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: <Z_fTOc5JCTcDzzv4@pathway.suse.cz>
Date: Thu, 10 Apr 2025 16:18:33 +0200
From: Petr Mladek <pmladek@...e.com>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
	David Laight <david.laight.linux@...il.com>,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	Nathan Chancellor <nathan@...nel.org>,
	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 Mon 2025-04-07 09:31:28, Rasmus Villemoes wrote:
> 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.

I think that we all agree that this patchset is better than the
current state.

I have added Andy's Tested-by from
https://lore.kernel.org/r/Z-557YrwVr8bONq4@smile.fi.intel.com

Link to the previous thread, see
https://lore.kernel.org/r/CAHk-=wgfX9nBGE0Ap9GjhOy7Mn=RSy=rx0MvqfYFFDx31KJXqQ@mail.gmail.com

and pushed this into printk/linux.git, branch for-6.15-printf-attribute.
It was the branch with the already pulled code, see
https://web.git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git/log/?h=for-6.15-printf-attribute

I am going to give it few days in linux-next and create another
pull request to have this sorted in 6.15 where it stated.

Best Regards,
Petr