| Message ID | 20220609221702.347522-1-morbo@google.com |
|---|---|
| Headers | show |
| Series | Clang -Wformat warning fixes | expand |
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <morbo@google.com> wrote: > This patch set fixes some clang warnings when -Wformat is enabled. > tldr: - printk(msg); + printk("%s", msg); the only reason to make this change is where `msg' could contain a `%'. Generally, it came from userspace. Otherwise these changes are a useless consumer of runtime resources. I think it would be better to quieten clang in some fashion.
On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <morbo@google.com> wrote: > > > This patch set fixes some clang warnings when -Wformat is enabled. > > > > tldr: > > - printk(msg); > + printk("%s", msg); > > the only reason to make this change is where `msg' could contain a `%'. > Generally, it came from userspace. It helps kernel developers not accidentally to insert an unescaped '%' in their messages, potentially exposing their code to an attack vector. > Otherwise these changes are a > useless consumer of runtime resources. Calling a "printf" style function is already insanely expensive. :-) I understand that it's not okay blithely to increase runtime resources simply because it's already slow, but in this case it's worthwhile. > I think it would be better to quieten clang in some fashion. The "printk" and similar functions all have the "__printf" attribute. I don't know of a modification to that attribute which can turn off this type of check. -bw
On 6/9/22 15:16, Bill Wendling wrote: > From: Bill Wendling <isanbard@gmail.com> > > When compiling with -Wformat, clang emits the following warnings: > > arch/x86/kernel/cpu/mce/core.c:295:9: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] > panic(msg); > ^~~ > > Use a string literal for the format string. > > Link: https://github.com/ClangBuiltLinux/linux/issues/378 > Signed-off-by: Bill Wendling <isanbard@gmail.com> > --- > arch/x86/kernel/cpu/mce/core.c | 2 +- > scripts/Makefile.extrawarn | 4 ++-- Where is the scripts/ change? > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > index 2c8ec5c71712..3d411b7c85ad 100644 > --- a/arch/x86/kernel/cpu/mce/core.c > +++ b/arch/x86/kernel/cpu/mce/core.c > @@ -292,7 +292,7 @@ static noinstr void mce_panic(const char *msg, struct mce *final, char *exp) > if (!fake_panic) { > if (panic_timeout == 0) > panic_timeout = mca_cfg.panic_timeout; > - panic(msg); > + panic("%s", msg); > } else > pr_emerg(HW_ERR "Fake kernel panic: %s\n", msg); >
On Thu, Jun 9, 2022 at 4:15 PM Randy Dunlap <rdunlap@infradead.org> wrote: > On 6/9/22 15:16, Bill Wendling wrote: > > From: Bill Wendling <isanbard@gmail.com> > > > > When compiling with -Wformat, clang emits the following warnings: > > > > arch/x86/kernel/cpu/mce/core.c:295:9: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] > > panic(msg); > > ^~~ > > > > Use a string literal for the format string. > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/378 > > Signed-off-by: Bill Wendling <isanbard@gmail.com> > > --- > > arch/x86/kernel/cpu/mce/core.c | 2 +- > > scripts/Makefile.extrawarn | 4 ++-- > > Where is the scripts/ change? > I'm sorry about this. The change in that file was a mistake that I reverted, but I forgot to change this part. -bw > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > > index 2c8ec5c71712..3d411b7c85ad 100644 > > --- a/arch/x86/kernel/cpu/mce/core.c > > +++ b/arch/x86/kernel/cpu/mce/core.c > > @@ -292,7 +292,7 @@ static noinstr void mce_panic(const char *msg, struct mce *final, char *exp) > > if (!fake_panic) { > > if (panic_timeout == 0) > > panic_timeout = mca_cfg.panic_timeout; > > - panic(msg); > > + panic("%s", msg); > > } else > > pr_emerg(HW_ERR "Fake kernel panic: %s\n", msg); > > > > -- > ~Randy
On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <morbo@google.com> wrote: > > > This patch set fixes some clang warnings when -Wformat is enabled. It looks like this series fixes -Wformat-security, which while being a member of the -Wformat group, is intentionally disabled in the kernel and somewhat orthogonal to enabling -Wformat with Clang. -Wformat is a group flag (like -Wall) that enables multiple other flags implicitly. Reading through clang/include/clang/Basic/DiagnosticGroups.td in clang's sources, it looks like: 1. -Wformat is a group flag. 2. -Wformat-security is a member of the -Wformat group; enabling -Wformat will enable -Wformat-security. 3. -Wformat itself is a member of -Wmost (never heard of -Wmost, but w/e). So -Wmost will enable -Wformat will enable -Wformat-security. 4. -Wmost is itself a member of -Wall. -Wall enables -Wmost enables -Wformat enables -Wformat security. Looking now at Kbuild: 1. Makefile:523 adds -Wall to KBUILD_CFLAGS. 2. The same assignment expression but on line 526 immediately disables -Wformat-security via -Wno-format-security. 3. scripts/Makefile.extrawarn disables -Wformat via -Wno-format only for clang (via guard of CONFIG_CC_IS_CLANG). We _want_ -Wformat enabled for clang so that developers aren't sending patches that trigger -Wformat with GCC (if they didn't happen to test their code with both). It's disabled for clang until we can build the kernel cleanly with it enabled, which we'd like to do. I don't think that we need to enable -Wformat-security to build with -Wformat for clang. I suspect based on Randy's comment on patch 1/12 that perhaps -Wformat was _added_ to KBUILD_CFLAGS in scripts/Makefile.extrawarn rather than -Wno-format being _removed_. The former would re-enable -Wformat-security due to the grouping logic described above. The latter is probably closer to our ultimate goal of enabling -Wformat coverage for clang (or rather not disabling the coverage via -Wno-format; a double negative). I'm pretty sure the kernel doesn't support %n in format strings...see the comment above vsnprintf in lib/vsprintf.c. Are there other attacks other than %n that -Wformat-security guards against? Maybe there's some context on the commit that added -Wno-format-security to the kernel? Regardless, I don't think enabling -Wformat-security is a blocker for enabling -Wformat (or...disabling -Wno-format...two sides of the same coin) for clang. > > > > tldr: > > - printk(msg); > + printk("%s", msg); > > the only reason to make this change is where `msg' could contain a `%'. > Generally, it came from userspace. Otherwise these changes are a > useless consumer of runtime resources. > > I think it would be better to quieten clang in some fashion.
On Thu, 9 Jun 2022 16:16:16 -0700 Bill Wendling <morbo@google.com> wrote: > On Thu, Jun 9, 2022 at 4:03 PM Jan Engelhardt <jengelh@inai.de> wrote: > > On Friday 2022-06-10 00:49, Bill Wendling wrote: > > >On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > >> On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <morbo@google.com> wrote: > > >> > > >> > This patch set fixes some clang warnings when -Wformat is enabled. > > >> > > >> tldr: > > >> > > >> - printk(msg); > > >> + printk("%s", msg); > > >> > > >> Otherwise these changes are a > > >> useless consumer of runtime resources. > > > > > >Calling a "printf" style function is already insanely expensive. > > >[...] > > >The "printk" and similar functions all have the "__printf" attribute. > > >I don't know of a modification to that attribute which can turn off > > >this type of check. > > > > Perhaps you can split vprintk_store in the middle (after the call to > > vsnprintf), and offer the second half as a function of its own (e.g. > > "puts"). Then the tldr could be > > > > - printk(msg); > > + puts(msg); > > That might be a nice compromise. Andrew, what do you think? > Sure. It's surprising that we don't already have a puts().
On Thu, Jun 09, 2022 at 10:16:26PM +0000, Bill Wendling wrote: > From: Bill Wendling <isanbard@gmail.com> Why isn't that matching your From: line in the email? > > When compiling with -Wformat, clang emits the following warnings: Is that ever a default build option for the kernel? > > drivers/char/mem.c:775:16: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] > NULL, devlist[minor].name); > ^~~~~~~~~~~~~~~~~~~ > > Use a string literal for the format string. > > Link: https://github.com/ClangBuiltLinux/linux/issues/378 > Signed-off-by: Bill Wendling <isanbard@gmail.com> > --- > drivers/char/mem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/char/mem.c b/drivers/char/mem.c > index 84ca98ed1dad..32d821ba9e4d 100644 > --- a/drivers/char/mem.c > +++ b/drivers/char/mem.c > @@ -772,7 +772,7 @@ static int __init chr_dev_init(void) > continue; > > device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor), > - NULL, devlist[minor].name); > + NULL, "%s", devlist[minor].name); Please explain how this static string can ever be user controlled. thanks, greg k-h
On Thu, Jun 09, 2022 at 04:16:16PM -0700, Bill Wendling wrote: > On Thu, Jun 9, 2022 at 4:03 PM Jan Engelhardt <jengelh@inai.de> wrote: > > On Friday 2022-06-10 00:49, Bill Wendling wrote: > > >On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > >> On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <morbo@google.com> wrote: > > >> > > >> > This patch set fixes some clang warnings when -Wformat is enabled. > > >> > > >> tldr: > > >> > > >> - printk(msg); > > >> + printk("%s", msg); > > >> > > >> Otherwise these changes are a > > >> useless consumer of runtime resources. > > > > > >Calling a "printf" style function is already insanely expensive. > > >[...] > > >The "printk" and similar functions all have the "__printf" attribute. > > >I don't know of a modification to that attribute which can turn off > > >this type of check. > > > > Perhaps you can split vprintk_store in the middle (after the call to > > vsnprintf), and offer the second half as a function of its own (e.g. > > "puts"). Then the tldr could be > > > > - printk(msg); > > + puts(msg); > > That might be a nice compromise. Andrew, what do you think? You would need to do that for all of the dev_printk() variants, so I doubt that would ever be all that useful as almost no one should be using a "raw" printk() these days. thanks, greg k-h
From: Bill Wendling > Sent: 09 June 2022 23:49 > > On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <morbo@google.com> wrote: > > > > > This patch set fixes some clang warnings when -Wformat is enabled. > > > > > > > tldr: > > > > - printk(msg); > > + printk("%s", msg); > > > > the only reason to make this change is where `msg' could contain a `%'. > > Generally, it came from userspace. > > It helps kernel developers not accidentally to insert an unescaped '%' > in their messages, potentially exposing their code to an attack > vector. > > > Otherwise these changes are a > > useless consumer of runtime resources. > > Calling a "printf" style function is already insanely expensive. :-) I > understand that it's not okay blithely to increase runtime resources > simply because it's already slow, but in this case it's worthwhile. Yep, IMHO definitely should be fixed. It is even possible that using "%s" is faster because the printf code doesn't have to scan the string for format effectors. > > I think it would be better to quieten clang in some fashion. > > The "printk" and similar functions all have the "__printf" attribute. > I don't know of a modification to that attribute which can turn off > this type of check. And you wouldn't want to for these cases. The only problems arise when the format is calculated (or passed in from a caller). But that is likely to be dangerous - reading formats from files (eg for language translation) isn't a good idea at all. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Friday 2022-06-10 10:17, David Laight wrote: >> >> Calling a "printf" style function is already insanely expensive. :-) I >> understand that it's not okay blithely to increase runtime resources >> simply because it's already slow, but in this case it's worthwhile. > >Yep, IMHO definitely should be fixed. >It is even possible that using "%s" is faster because the printf >code doesn't have to scan the string for format effectors. I see no special handling; the vsnprintf function just loops over fmt as usual and I see no special casing of fmt by e.g. strcmp(fmt, "%s") == 0 to take a shortcut.
From: Jan Engelhardt > Sent: 10 June 2022 09:32 > > > On Friday 2022-06-10 10:17, David Laight wrote: > >> > >> Calling a "printf" style function is already insanely expensive. :-) I > >> understand that it's not okay blithely to increase runtime resources > >> simply because it's already slow, but in this case it's worthwhile. > > > >Yep, IMHO definitely should be fixed. > >It is even possible that using "%s" is faster because the printf > >code doesn't have to scan the string for format effectors. > > I see no special handling; the vsnprintf function just loops > over fmt as usual and I see no special casing of fmt by > e.g. strcmp(fmt, "%s") == 0 to take a shortcut. Consider the difference between: printf("fubar"); and printf("%s", "fubar"); In the former all of "fubar" is checked for '%'. In the latter only the length of "fubar" has to be counted. FWIW the patch description should probably by: use "%s" when formatting a single string. (or something to that effect). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Friday 2022-06-10 11:14, David Laight wrote: >> >Yep, IMHO definitely should be fixed. >> >It is even possible that using "%s" is faster because the printf >> >code doesn't have to scan the string for format effectors. >> >> I see no special handling; the vsnprintf function just loops >> over fmt as usual and I see no special casing of fmt by >> e.g. strcmp(fmt, "%s") == 0 to take a shortcut. > >Consider the difference between: > printf("fubar"); >and > printf("%s", "fubar"); >In the former all of "fubar" is checked for '%'. >In the latter only the length of "fubar" has to be counted. To check the length of "fubar", printf first needs to know that there even is an argument to be pulled from the stack, which it does by evaluating the format string. So, in fairness, it's more like: >> In the latter, all of "%s" is checked for '%'.
On Fri, 2022-06-10 at 07:20 +0200, Greg Kroah-Hartman wrote: > On Thu, Jun 09, 2022 at 04:16:16PM -0700, Bill Wendling wrote: > > On Thu, Jun 9, 2022 at 4:03 PM Jan Engelhardt <jengelh@inai.de> wrote: > > > On Friday 2022-06-10 00:49, Bill Wendling wrote: > > > > On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <morbo@google.com> wrote: > > > > > > > > > > > This patch set fixes some clang warnings when -Wformat is enabled. > > > > > > > > > > tldr: > > > > > > > > > > - printk(msg); > > > > > + printk("%s", msg); > > > > > > > > > > Otherwise these changes are a > > > > > useless consumer of runtime resources. > > > > Calling a "printf" style function is already insanely expensive. I expect the printk code itself dominates, not the % scan cost. > > > Perhaps you can split vprintk_store in the middle (after the call to > > > vsnprintf), and offer the second half as a function of its own (e.g. > > > "puts"). Then the tldr could be > > > > > > - printk(msg); > > > + puts(msg); > > > > That might be a nice compromise. Andrew, what do you think? > > You would need to do that for all of the dev_printk() variants, so I > doubt that would ever be all that useful as almost no one should be > using a "raw" printk() these days. True. The kernel has ~20K variants like that. $ git grep -P '\b(?:(?:\w+_){1,3}(?:alert|emerg|crit|err|warn|notice|info|cont|debug|dbg)|printk)\s*\(".*"\s*\)\s*;' | wc -l 21160 That doesn't include the ~3K uses like #define foo "bar" printk(foo); $ git grep -P '\b(?:(?:\w+_){1,3}(?:alert|emerg|crit|err|warn|info|notice|debug|dbg|cont)|printk)\s*\((?:\s*\w+){1,3}\s*\)\s*;'|wc -l 2922 There are apparently only a few hundred uses of variants like: printk("%s", foo) $ git grep -P '\b(?:(?:\w+_){1,3}(?:alert|emerg|crit|err|warn|info|notice|debug|dbg|cont)|printk)\s*\(\s*"%s(?:\\n)?"\s*,\s*(?:".*"|\w+)\s*\)\s*;' | wc -l 305 unless I screwed up my greps (which of course is quite possible)
On Thu, Jun 9, 2022 at 10:18 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Jun 09, 2022 at 10:16:26PM +0000, Bill Wendling wrote: > > From: Bill Wendling <isanbard@gmail.com> > > Why isn't that matching your From: line in the email? > There must be something wrong with my .gitconfig file. I"ll check into it. > > > > When compiling with -Wformat, clang emits the following warnings: > > Is that ever a default build option for the kernel? > We want to enable -Wformat for clang. I believe that these specific warnings have been disabled, but I'm confused as to why, because they're valid warnings. When I compiled with the warning enabled, there were only a few (12) places that needed changes, so thought that patches would be a nice cleanup, even though the warning itself is disabled. > > drivers/char/mem.c:775:16: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] > > NULL, devlist[minor].name); > > ^~~~~~~~~~~~~~~~~~~ > > > > Use a string literal for the format string. > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/378 > > Signed-off-by: Bill Wendling <isanbard@gmail.com> > > --- > > drivers/char/mem.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/char/mem.c b/drivers/char/mem.c > > index 84ca98ed1dad..32d821ba9e4d 100644 > > --- a/drivers/char/mem.c > > +++ b/drivers/char/mem.c > > @@ -772,7 +772,7 @@ static int __init chr_dev_init(void) > > continue; > > > > device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor), > > - NULL, devlist[minor].name); > > + NULL, "%s", devlist[minor].name); > > Please explain how this static string can ever be user controlled. > All someone would need to do is accidentally insert an errant '%' in one of the strings for this function call to perform unexpected actions---at the very least reading memory that's not allocated and may contain garbage, thereby decreasing performance and possibly overrunning some buffer. Perhaps in this specific scenario it's unlikely, but "device_create()" is used in a lot more places than here. This patch is a general code cleanup. -bw