linux-kernel - Re: [PATCH 0/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87zfgs5sxb.fsf@prevas.dk>
Date: Mon, 07 Apr 2025 09:31:28 +0200
From: Rasmus Villemoes <linux@...musvillemoes.dk>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: David Laight <david.laight.linux@...il.com>,  Andy Shevchenko
 <andy.shevchenko@...il.com>,  Nathan Chancellor <nathan@...nel.org>,  Petr
 Mladek <pmladek@...e.com>,  Steven Rostedt <rostedt@...dmis.org>,  Sergey
 Senozhatsky <senozhatsky@...omium.org>,  linux-kernel@...r.kernel.org,
  llvm@...ts.linux.dev
Subject: Re: [PATCH 0/2] vsprintf: Use __diag macros to disable
 '-Wsuggest-attribute=format'

On Sat, Apr 05 2025, Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Sat, 5 Apr 2025 at 02:11, David Laight <david.laight.linux@...il.com> wrote:
>>
>> Perhaps the compilers ought to support __attribute__((format(none)))
>> to disable the warning.
>
> D'oh, that's a good idea.
>
> And gcc already supports it, even if we have to hack it up.
>
> So let's remove this whole horrible garbage entirely, and replace it
> with __printf(1,0) which should do exactly that.
>
> The 1 is for the format string argument number, and we're just *lying*
> about it. But there is not format string argument, and gcc just checks
> for 'is it a char pointer).
>
> The real format string argument is va_fmt->fmt, but there's no way to
> tell gcc that.
>
> And the 0 is is to tell gcc that there's nothing to verify.
>
> Then, if you do that, gcc will say "oh, maybe you need to do the same
> for the 'pointer()' function". That one has a real 'fmt' thing, but
> again nothing to be checked, so we do the same '__printf(1,0)' there
> too.
>
> There it makes more sense, because argument 1 _is_ actually a format
> string, so we're not lying about it.
>
> IOW, something like this:
>
>   --- a/lib/vsprintf.c
>   +++ b/lib/vsprintf.c
>   @@ -1700,9 +1700,10 @@ char *escaped_string(...
>    }
>
>   -#pragma GCC diagnostic push
>   -#ifndef __clang__
>   -#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
>   -#endif
>   -static char *va_format(char *buf, char *end, struct va_format *va_fmt,
>   +/*
>   + * The '__printf(1,0)' thing is a hack make gcc not ask us to use a
>   + * a format attribute. 'buf' is *not* the format, 'va_fmt->fmt' is.
>   + */
>   +static __printf(1,0)
>   +char *va_format(char *buf, char *end, struct va_format *va_fmt,
>                        struct printf_spec spec)
>    {
>   @@ -1718,5 +1719,4 @@ static char *va_format(...
>         return buf;
>    }
>   -#pragma GCC diagnostic pop
>
>    static noinline_for_stack
>   @@ -2429,5 +2429,5 @@ early_param(...
>     * See rust/kernel/print.rs for details.
>     */
>   -static noinline_for_stack
>   +static noinline_for_stack __printf(1,0)
>    char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>               struct printf_spec spec)
>
> Does that work for people who see this warning?

IMHO, this is much worse.

Yes, as I also said in the previous thread, I consider the
warning/suggestion here a gcc bug, as it shouldn't make that suggestion
when one doesn't pass any of the function's arguments as the fmt
argument to another __format__(()) annotated-function.

But we have this __diag infrastructure exactly to silence special cases
(and sorry I forgot about that when suggesting the #pragma approach to
Andy), and this is very much a special case: It's the only place in the
whole codebase that has any reason to dereference that va_fmt, and any
other function anywhere calling a vsprintf()-like really should have
gotten the format string that goes along with the varargs from its
caller.

As this is apparently some newer gcc that has started doing this, you
just risk the next version turning the wrongness to 11 and complaining
that "buf" or "fmt" is not passed to a vsprintf-like function. Let's not
do "a hack make gcc not ask us to use a format attribute" when we have
a proper way to selectively silence such false-positives. If this was
something happening all over, we'd do -Wno-suggest-attribute=format, not
spread these annotations. But this really is a special case in the guts
of our printf implementation.

So, FWIW, ack on Nathan's fixups, nak on this one.

Rasmus