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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Vd--N0QjGrAW3TJRVCN0-LG31rVeHTGyb0awgO0sc_aXA@mail.gmail.com>
Date: Sat, 5 Apr 2025 21:54:14 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: David Laight <david.laight.linux@...il.com>, Nathan Chancellor <nathan@...nel.org>, 
	Petr Mladek <pmladek@...e.com>, Steven Rostedt <rostedt@...dmis.org>, 
	Rasmus Villemoes <linux@...musvillemoes.dk>, 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 5, 2025 at 8:27 PM 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?

This is quite similar to my initial approach [1] which Rasmus was
against (okay, I did the nasty castings on top of the printf() there,
but still). TL;DR: I assume it will work, but let others comment on
this.

[1]: https://lore.kernel.org/lkml/20250320180926.4002817-7-andriy.shevchenko@linux.intel.com/

-- 
With Best Regards,
Andy Shevchenko