[PATCH] Kbuild: Disable the -Wformat-security gcc flag

[PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Floris Kraak]

Some distributions have enabled the gcc flag -Wformat-security by default.*
This results in a number of warnings about format arguments to
functions, sometimes in cases where fixing the warning is not likely
to actually fix a bug.
Instead of hand patching a dozens of places (possibly more) that
produce warnings that get ignored anyway we just turn off the flag in
the Makefile.

Note: Regardless of any discussion surrounding the value of this
particular type of warning, having this show up in a few distributions
but not in the
vast majority of them means that this warning won't be seen by most of
the developers who introduce the new warnings in the first place. If
the
kernel decides it cares about format arguments it should do so
globally regardless of distribution. In which case I'd gladly whip up
a patch to do
the reverse thing and turn this thing on by default. However, such a
patch would have to produce a follow up patch(set) which fixes each
individual
warning.

See also:
http://kerneltrap.org/mailarchive/linux-kernel/2008/11/20/4215134

*) The ubuntu manpage for gcc states:

      -Wformat-security
          If -Wformat is specified, also warn about uses of format
functions that represent possible security problems.  At present, this
warns about
          calls to "printf" and "scanf" functions where the format
string is not a string literal and there are no format arguments, as
in "printf
          (foo);".  This may be a security hole if the format string
came from untrusted input and contains %n.  (This is currently a
subset of what
          -Wformat-nonliteral warns about, but in future warnings may
be added to -Wformat-security that are not included in
-Wformat-nonliteral.)

          NOTE: In Ubuntu 8.10 and later versions this option is
enabled by default for C, C++, ObjC, ObjC++.  To disable, use
-Wno-format-security, or
          disable all format warnings with -Wformat=0.  To make format
security warnings fatal, specify -Werror=format-security.

Signed-off-by: Floris Kraak <randakar@gmail.com>
---
diff --git a/Makefile b/Makefile
index 7715b2c..9ee766c 100644
--- a/Makefile
+++ b/Makefile
@@ -346,7 +346,8 @@ KBUILD_CPPFLAGS := -D__KERNEL__

 KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
 		   -fno-strict-aliasing -fno-common \
-		   -Werror-implicit-function-declaration
+		   -Werror-implicit-function-declaration \
+		   -Wno-format-security
 KBUILD_AFLAGS   := -D__ASSEMBLY__

 # Read KERNELRELEASE from include/config/kernel.release (if it exists)

---
"They that give up essential liberty to obtain temporary safety,
deserve neither liberty nor safety."
  -- Ben Franklin

"The course of history shows that as a government grows, liberty
decreases."
  -- Thomas Jefferson

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Sam Ravnborg]

On Wed, Feb 04, 2009 at 03:28:47PM +0100, Floris Kraak wrote:
> Some distributions have enabled the gcc flag -Wformat-security by default.*
> This results in a number of warnings about format arguments to
> functions, sometimes in cases where fixing the warning is not likely
> to actually fix a bug.
> Instead of hand patching a dozens of places (possibly more) that
> produce warnings that get ignored anyway we just turn off the flag in
> the Makefile.
> 
> Note: Regardless of any discussion surrounding the value of this
> particular type of warning, having this show up in a few distributions
> but not in the
> vast majority of them means that this warning won't be seen by most of
> the developers who introduce the new warnings in the first place. If
> the
> kernel decides it cares about format arguments it should do so
> globally regardless of distribution. In which case I'd gladly whip up
> a patch to do
> the reverse thing and turn this thing on by default. However, such a
> patch would have to produce a follow up patch(set) which fixes each
> individual
> warning.

Before judging on this patch could you please post what warning it
triggers and one or a few patches to fix some of them.

	Sam

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Roland Dreier]

 > Before judging on this patch could you please post what warning it
 > triggers and one or a few patches to fix some of them.

The warnings are things like:

    init/main.c: In function 'start_kernel':
    init/main.c:557: warning: format not a string literal and no format arguments

where the patch to fix this would be:

diff --git a/init/main.c b/init/main.c
index 8442094..78fc0d8 100644
--- a/init/main.c
+++ b/init/main.c
@@ -554,7 +554,7 @@ asmlinkage void __init start_kernel(void)
 	boot_cpu_init();
 	page_address_init();
 	printk(KERN_NOTICE);
-	printk(linux_banner);
+	printk("%s", linux_banner);
 	setup_arch(&command_line);
 	mm_init_owner(&init_mm, &init_task);
 	setup_command_line(command_line);

with the impact:

add/remove: 0/0 grow/shrink: 1/0 up/down: 7/0 (7)
function                                     old     new   delta
start_kernel                                 689     696      +7


There are also many warnings like:

    drivers/char/mem.c: In function 'chr_dev_init':
    drivers/char/mem.c:994: warning: format not a string literal and no format arguments

which require a similar fix but show that we can't just restrict things
to some trick with printk():

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 3586b3b..ba3bf43 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -991,7 +991,7 @@ static int __init chr_dev_init(void)
 	for (i = 0; i < ARRAY_SIZE(devlist); i++)
 		device_create(mem_class, NULL,
 			      MKDEV(MEM_MAJOR, devlist[i].minor), NULL,
-			      devlist[i].name);
+			      "%s", devlist[i].name);
 
 	return 0;
 }

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Robert Hancock]

Roland Dreier wrote:
>  > Before judging on this patch could you please post what warning it
>  > triggers and one or a few patches to fix some of them.
> 
> The warnings are things like:
> 
>     init/main.c: In function 'start_kernel':
>     init/main.c:557: warning: format not a string literal and no format arguments
> 
> where the patch to fix this would be:
> 
> diff --git a/init/main.c b/init/main.c
> index 8442094..78fc0d8 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -554,7 +554,7 @@ asmlinkage void __init start_kernel(void)
>  	boot_cpu_init();
>  	page_address_init();
>  	printk(KERN_NOTICE);
> -	printk(linux_banner);
> +	printk("%s", linux_banner);
>  	setup_arch(&command_line);
>  	mm_init_owner(&init_mm, &init_task);
>  	setup_command_line(command_line);
> 
> with the impact:
> 
> add/remove: 0/0 grow/shrink: 1/0 up/down: 7/0 (7)
> function                                     old     new   delta
> start_kernel                                 689     696      +7

Just how many of these warnings are showing up? In the cases you posted 
it's presumably no problem, but if the string could either a) be 
potentially set by a malicious user or b) accidentally contain printk 
format characters then this code has a risk that things could blow up..

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Roland Dreier]

 > Just how many of these warnings are showing up? In the cases you
 > posted it's presumably no problem, but if the string could either a)
 > be potentially set by a malicious user or b) accidentally contain
 > printk format characters then this code has a risk that things could
 > blow up..

I get ~150 of them on an x86 allyesconfig build here (see below).  Many
but not all are trivial; some at least appear to be passing in strings
that come from random hardware/firmware or DNS names etc (ie there's at
least a chance of a '%'); and I didn't exhaustively audit to make sure
none of them could print something from an unprivileged user.

init/main.c:557: warning: format not a string literal and no format arguments
init/initramfs.c:582: warning: format not a string literal and no format arguments
arch/x86/kernel/dumpstack.c:115: warning: format not a string literal and no format arguments
arch/x86/kernel/dumpstack.c:137: warning: format not a string literal and no format arguments
arch/x86/kernel/e820.c:1177: warning: format not a string literal and no format arguments
arch/x86/kernel/e820.c:1178: warning: format not a string literal and no format arguments
arch/x86/kernel/cpu/mcheck/mce_64.c:149: warning: format not a string literal and no format arguments
kernel/power/main.c:717: warning: format not a string literal and no format arguments
kernel/cpuset.c:2447: warning: format not a string literal and no format arguments
fs/gfs2/glock.c:901: warning: format not a string literal and no format arguments
fs/gfs2/locking.c:180: warning: format not a string literal and no format arguments
fs/lockd/svc.c:303: warning: format not a string literal and no format arguments
fs/nfs/nfs4proc.c:2929: warning: format not a string literal and no format arguments
fs/partitions/check.c:455: warning: format not a string literal and no format arguments
fs/reiserfs/prints.c:292: warning: format not a string literal and no format arguments
fs/ubifs/super.c:425: warning: format not a string literal and no format arguments
fs/ubifs/super.c:1204: warning: format not a string literal and no format arguments
fs/ubifs/super.c:1557: warning: format not a string literal and no format arguments
fs/dquot.c:175: warning: format not a string literal and no format arguments
fs/dquot.c:175: warning: format not a string literal and no format arguments
fs/dquot.c:175: warning: format not a string literal and no format arguments
crypto/api.c:218: warning: format not a string literal and no format arguments
crypto/algapi.c:427: warning: format not a string literal and no format arguments
crypto/cryptd.c:547: warning: format not a string literal and no format arguments
drivers/atm/iphase.c:982: warning: format not a string literal and no format arguments
drivers/base/core.c:1250: warning: format not a string literal and no format arguments
drivers/base/sys.c:140: warning: format not a string literal and no format arguments
drivers/base/platform.c:247: warning: format not a string literal and no format arguments
drivers/base/attribute_container.c:170: warning: format not a string literal and no format arguments
drivers/base/firmware_class.c:318: warning: format not a string literal and no format arguments
drivers/block/nbd.c:657: warning: format not a string literal and no format arguments
drivers/block/aoe/aoechr.c:289: warning: format not a string literal and no format arguments
drivers/cdrom/cdrom.c:3379: warning: format not a string literal and no format arguments
drivers/char/mem.c:994: warning: format not a string literal and no format arguments
drivers/char/tty_io.c:2850: warning: format not a string literal and no format arguments
drivers/char/hw_random/intel-rng.c:315: warning: format not a string literal and no format arguments
drivers/char/riscom8.c:1500: warning: format not a string literal and no format arguments
drivers/char/riscom8.c:1510: warning: format not a string literal and no format arguments
drivers/char/n_hdlc.c:945: warning: format not a string literal and no format arguments
drivers/char/n_hdlc.c:968: warning: format not a string literal and no format arguments
drivers/cpufreq/cpufreq.c:244: warning: format not a string literal and no format arguments
drivers/hwmon/adt7470.c:1294: warning: format not a string literal and no format arguments
drivers/ide/ide-probe.c:650: warning: format not a string literal and no format arguments
drivers/ide/ide-probe.c:664: warning: format not a string literal and no format arguments
drivers/infiniband/core/sysfs.c:781: warning: format not a string literal and no format arguments
drivers/infiniband/hw/ipath/ipath_file_ops.c:2452: warning: format not a string literal and no format arguments
drivers/infiniband/hw/ipath/ipath_file_ops.c:2462: warning: format not a string literal and no format arguments
drivers/input/mousedev.c:881: warning: format not a string literal and no format arguments
drivers/input/joydev.c:803: warning: format not a string literal and no format arguments
drivers/input/evdev.c:822: warning: format not a string literal and no format arguments
drivers/input/tablet/aiptek.c:1373: warning: format not a string literal and no format arguments
drivers/isdn/mISDN/dsp_pipeline.c:104: warning: format not a string literal and no format arguments
drivers/media/video/v4l2-common.c:558: warning: format not a string literal and no format arguments
drivers/media/video/v4l2-common.c:723: warning: format not a string literal and no format arguments
drivers/media/video/v4l2-common.c:741: warning: format not a string literal and no format arguments
drivers/media/video/pvrusb2/pvrusb2-hdw.c:1970: warning: format not a string literal and no format arguments
drivers/media/video/pvrusb2/pvrusb2-std.c:219: warning: format not a string literal and no format arguments
drivers/media/video/zoran/zoran_card.c:1421: warning: format not a string literal and no format arguments
drivers/media/video/zoran/zoran_card.c:1441: warning: format not a string literal and no format arguments
drivers/media/video/zoran/zoran_card.c:1465: warning: format not a string literal and no format arguments
drivers/media/video/zoran/zoran_card.c:1477: warning: format not a string literal and no format arguments
drivers/media/video/tvaudio.c:1916: warning: format not a string literal and no format arguments
drivers/media/video/cx2341x.c:474: warning: format not a string literal and no format arguments
drivers/misc/enclosure.c:122: warning: format not a string literal and no format arguments
drivers/misc/enclosure.c:259: warning: format not a string literal and no format arguments
drivers/mtd/chips/gen_probe.c:215: warning: format not a string literal and no format arguments
drivers/mtd/ubi/build.c:851: warning: format not a string literal and no format arguments
drivers/net/hamradio/mkiss.c:994: warning: format not a string literal and no format arguments
drivers/net/hamradio/mkiss.c:997: warning: format not a string literal and no format arguments
drivers/net/hamradio/6pack.c:800: warning: format not a string literal and no format arguments
drivers/net/hamradio/yam.c:1117: warning: format not a string literal and no format arguments
drivers/net/hamradio/bpqether.c:617: warning: format not a string literal and no format arguments
drivers/net/pcmcia/axnet_cs.c:1716: warning: format not a string literal and no format arguments
drivers/net/phy/mdio_bus.c:101: warning: format not a string literal and no format arguments
drivers/net/tulip/dmfe.c:378: warning: format not a string literal and no format arguments
drivers/net/tulip/dmfe.c:2191: warning: format not a string literal and no format arguments
drivers/net/tulip/winbond-840.c:1666: warning: format not a string literal and no format arguments
drivers/net/tulip/de4x5.c:1268: warning: format not a string literal and no format arguments
drivers/net/tulip/uli526x.c:277: warning: format not a string literal and no format arguments
drivers/net/tulip/uli526x.c:1819: warning: format not a string literal and no format arguments
drivers/net/wan/lapbether.c:441: warning: format not a string literal and no format arguments
drivers/net/wireless/b43/main.c:2008: warning: format not a string literal and no format arguments
drivers/net/wireless/b43/main.c:2010: warning: format not a string literal and no format arguments
drivers/net/wireless/hostap/hostap_ioctl.c:3272: warning: format not a string literal and no format arguments
drivers/net/wireless/ipw2x00/libipw_wx.c:611: warning: format not a string literal and no format arguments
drivers/net/wireless/airo.c:1887: warning: format not a string literal and no format arguments
drivers/net/rrunner.c:137: warning: format not a string literal and no format arguments
drivers/net/3c59x.c:1018: warning: format not a string literal and no format arguments
drivers/net/3c59x.c:2886: warning: format not a string literal and no format arguments
drivers/net/ne2k-pci.c:234: warning: format not a string literal and no format arguments
drivers/net/sis900.c:428: warning: format not a string literal and no format arguments
drivers/net/yellowfin.c:393: warning: format not a string literal and no format arguments
drivers/net/acenic.c:503: warning: format not a string literal and no format arguments
drivers/net/natsemi.c:816: warning: format not a string literal and no format arguments
drivers/net/fealnx.c:506: warning: format not a string literal and no format arguments
drivers/net/via-rhine.c:655: warning: format not a string literal and no format arguments
drivers/net/starfire.c:685: warning: format not a string literal and no format arguments
drivers/net/sundance.c:489: warning: format not a string literal and no format arguments
drivers/net/hamachi.c:604: warning: format not a string literal and no format arguments
drivers/net/forcedeth.c:926: warning: format not a string literal and no format arguments
drivers/net/defxx.c:534: warning: format not a string literal and no format arguments
drivers/net/eql.c:587: warning: format not a string literal and no format arguments
drivers/scsi/aacraid/commctrl.c:320: warning: format not a string literal and no format arguments
drivers/scsi/aacraid/commsup.c:1223: warning: format not a string literal and no format arguments
drivers/scsi/sd.c:1833: warning: format not a string literal and no format arguments
drivers/scsi/advansys.c:2899: warning: format not a string literal and no format arguments
drivers/scsi/sg.c:2540: warning: format not a string literal and no format arguments
drivers/serial/serial_core.c:1758: warning: format not a string literal and no format arguments
drivers/usb/atm/usbatm.c:1034: warning: format not a string literal and no format arguments
drivers/usb/atm/usbatm.c:1089: warning: format not a string literal and no format arguments
drivers/usb/storage/libusual.c:190: warning: format not a string literal and no format arguments
drivers/uwb/lc-dev.c:440: warning: format not a string literal and no format arguments
drivers/video/backlight/lcd.c:211: warning: format not a string literal and no format arguments
drivers/video/backlight/backlight.c:247: warning: format not a string literal and no format arguments
drivers/video/output.c:99: warning: format not a string literal and no format arguments
drivers/xen/xenbus/xenbus_probe.c:510: warning: format not a string literal and no format arguments
sound/sound_core.c:225: warning: format not a string literal and no format arguments
sound/core/sound.c:91: warning: format not a string literal and no format arguments
sound/core/seq/seq_clientmgr.c:2449: warning: format not a string literal and no format arguments
sound/drivers/opl3/opl3_seq.c:238: warning: format not a string literal and no format arguments
sound/pci/rme32.c:1473: warning: format not a string literal and no format arguments
sound/pci/rme96.c:1673: warning: format not a string literal and no format arguments
sound/pci/hda/hda_codec.c:600: warning: format not a string literal and no format arguments
sound/pci/korg1212/korg1212.c:2062: warning: format not a string literal and no format arguments
net/802/psnap.c:106: warning: format not a string literal and no format arguments
net/appletalk/ddp.c:1894: warning: format not a string literal and no format arguments
net/core/net-sysfs.c:499: warning: format not a string literal and no format arguments
net/decnet/af_decnet.c:2377: warning: format not a string literal and no format arguments
net/ipv4/ipip.c:836: warning: format not a string literal and no format arguments
net/ipx/af_ipx.c:2002: warning: format not a string literal and no format arguments
net/ipx/af_ipx.c:2008: warning: format not a string literal and no format arguments
net/ipx/af_ipx.c:2012: warning: format not a string literal and no format arguments
net/ipx/af_ipx.c:2016: warning: format not a string literal and no format arguments
net/llc/af_llc.c:1140: warning: format not a string literal and no format arguments
net/llc/af_llc.c:1145: warning: format not a string literal and no format arguments
net/llc/af_llc.c:1150: warning: format not a string literal and no format arguments
net/netfilter/nf_conntrack_proto_dccp.c:450: warning: format not a string literal and no format arguments
net/netfilter/nf_conntrack_proto_dccp.c:596: warning: format not a string literal and no format arguments
net/netfilter/ipvs/ip_vs_sync.c:876: warning: format not a string literal and no format arguments
net/sunrpc/svc.c:668: warning: format not a string literal and no format arguments
net/tipc/bcast.c:803: warning: format not a string literal and no format arguments
net/tipc/node.c:706: warning: format not a string literal and no format arguments
net/tipc/dbg.c:261: warning: format not a string literal and no format arguments
net/tipc/dbg.c:281: warning: format not a string literal and no format arguments
net/tipc/dbg.c:328: warning: format not a string literal and no format arguments
lib/kobject.c:797: warning: format not a string literal and no format arguments

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Floris Kraak]

On Thu, Feb 5, 2009 at 7:37 AM, Roland Dreier <rdreier@cisco.com> wrote:
>  > Just how many of these warnings are showing up? In the cases you
>  > posted it's presumably no problem, but if the string could either a)
>  > be potentially set by a malicious user or b) accidentally contain
>  > printk format characters then this code has a risk that things could
>  > blow up..
>
> I get ~150 of them on an x86 allyesconfig build here (see below).  Many
> but not all are trivial; some at least appear to be passing in strings
> that come from random hardware/firmware or DNS names etc (ie there's at
> least a chance of a '%'); and I didn't exhaustively audit to make sure
> none of them could print something from an unprivileged user.
>

There are probably some real bugs in there. On the other hand there is
some overhead to fixing the warnings. Kernel text size increase,
possibly some CPU overhead from parsing the format string. Hopefully
none of these calls are in really hot code paths ;-)
As I noted applying a patch that does the reverse and enables the
check instead is perfectly acceptable to me. Long term somebody
probably needs to go through all of them and fix (most of) them
anyway.

What remains an open question to me though is what to do with cases
where the warning not only can be ignored but literally should be. eg.
when there is zero chance of something unexpected getting passed in
and 'fixing' it would just bloat the kernel.
Can sparse be used to check this kind of thing for correctness?

Regards,
Floris
---
"They that give up essential liberty to obtain temporary safety,
deserve neither liberty nor safety."
  -- Ben Franklin

"The course of history shows that as a government grows, liberty
decreases."
  -- Thomas Jefferson

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Floris Kraak]

On Thu, Feb 5, 2009 at 9:26 AM, Floris Kraak <randakar@gmail.com> wrote:
> On Thu, Feb 5, 2009 at 7:37 AM, Roland Dreier <rdreier@cisco.com> wrote:
>>  > Just how many of these warnings are showing up? In the cases you
>>  > posted it's presumably no problem, but if the string could either a)
>>  > be potentially set by a malicious user or b) accidentally contain
>>  > printk format characters then this code has a risk that things could
>>  > blow up..
>>
>> I get ~150 of them on an x86 allyesconfig build here (see below).  Many
>> but not all are trivial; some at least appear to be passing in strings
>> that come from random hardware/firmware or DNS names etc (ie there's at
>> least a chance of a '%'); and I didn't exhaustively audit to make sure
>> none of them could print something from an unprivileged user.
>>
>
> There are probably some real bugs in there. On the other hand there is
> some overhead to fixing the warnings. Kernel text size increase,
> possibly some CPU overhead from parsing the format string. Hopefully
> none of these calls are in really hot code paths ;-)
> As I noted applying a patch that does the reverse and enables the
> check instead is perfectly acceptable to me. Long term somebody
> probably needs to go through all of them and fix (most of) them
> anyway.
>
> What remains an open question to me though is what to do with cases
> where the warning not only can be ignored but literally should be. eg.
> when there is zero chance of something unexpected getting passed in
> and 'fixing' it would just bloat the kernel.
> Can sparse be used to check this kind of thing for correctness?
>

Example:

kernel/power/main.c:717: warning: format not a string literal and no
format arguments

This complains about:
..
        if (!rtc) {
                printk(warn_no_rtc);
                goto done;
        }
..

So what is this "warn_no_rtc" thing?

        static char             warn_no_rtc[] __initdata =
                KERN_WARNING "PM: no wakealarm-capable RTC driver is ready\n";

That's pretty much GCC failing to recognize that the format is a
string literal and then complaining that it isn't.
How do we make the warning go away without growing the kernel text?
Given the use of __initdata flags I'm not even sure if doing the
obvious printk("%s", warn_no_rtc) isn't going to introduce a subtle
bug somehow..

Regards,
Floris
---
"They that give up essential liberty to obtain temporary safety,
deserve neither liberty nor safety."
  -- Ben Franklin

"The course of history shows that as a government grows, liberty
decreases."
  -- Thomas Jefferson

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Andreas Schwab]

Floris Kraak <randakar@gmail.com> writes:

> Example:
>
> kernel/power/main.c:717: warning: format not a string literal and no
> format arguments
>
> This complains about:
> ..
>         if (!rtc) {
>                 printk(warn_no_rtc);
>                 goto done;
>         }
> ..
>
> So what is this "warn_no_rtc" thing?
>
>         static char             warn_no_rtc[] __initdata =
>                 KERN_WARNING "PM: no wakealarm-capable RTC driver is ready\n";

Does it help to make it const?

> That's pretty much GCC failing to recognize that the format is a
> string literal and then complaining that it isn't.

Since it's a non-const variable it cannot be a string literal.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Floris Kraak]

On Thu, Feb 5, 2009 at 11:27 AM, Andreas Schwab <schwab@suse.de> wrote:
> Floris Kraak <randakar@gmail.com> writes:
>
>> Example:
>>
>> kernel/power/main.c:717: warning: format not a string literal and no
>> format arguments
>>
>> This complains about:
>> ..
>>         if (!rtc) {
>>                 printk(warn_no_rtc);
>>                 goto done;
>>         }
>> ..
>>
>> So what is this "warn_no_rtc" thing?
>>
>>         static char             warn_no_rtc[] __initdata =
>>                 KERN_WARNING "PM: no wakealarm-capable RTC driver is ready\n";
>
> Does it help to make it const?
>
>> That's pretty much GCC failing to recognize that the format is a
>> string literal and then complaining that it isn't.
>
> Since it's a non-const variable it cannot be a string literal.
>

kernel/power/main.c:698: error: warn_no_rtc causes a section type conflict

Nice try ;-)

Replacing __initdata with __devinitconst sounded like it might solve
things ( - http://mail.nl.linux.org/kernelnewbies/2008-02/msg00042.html,
interesting reading) but alas, the warning persists.

(apologies to Andreas for the resend)

Regards,
Floris
---
"They that give up essential liberty to obtain temporary safety,
deserve neither liberty nor safety."
  -- Ben Franklin

"The course of history shows that as a government grows, liberty
decreases."
  -- Thomas Jefferson

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Roland Dreier]

 > > So what is this "warn_no_rtc" thing?
 > >
 > >         static char             warn_no_rtc[] __initdata =
 > >                 KERN_WARNING "PM: no wakealarm-capable RTC driver is ready\n";
 > 
 > Does it help to make it const?

You can't make __initdata const, because the initdata section is not
read-only.

 - R.

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Kyle Moffett]

On Thu, Feb 5, 2009 at 3:26 AM, Floris Kraak <randakar@gmail.com> wrote:
> There are probably some real bugs in there. On the other hand there is
> some overhead to fixing the warnings. Kernel text size increase,
> possibly some CPU overhead from parsing the format string. Hopefully
> none of these calls are in really hot code paths ;-)

Actually, I would really suspect that CPU time would *decrease* for
most of these, because instead of groveling through the whole argument
string looking for '%' symbols we would do a single "%s" lookup
followed by a basic strncpy(dmesg_buffer + offset, arg,
dmesg_buffer_size - offset);

Cheers,
Kyle Moffett

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Floris Kraak]

On Tue, Feb 10, 2009 at 10:11 PM, Kyle Moffett <kyle@moffetthome.net> wrote:
> On Thu, Feb 5, 2009 at 3:26 AM, Floris Kraak <randakar@gmail.com> wrote:
>> There are probably some real bugs in there. On the other hand there is
>> some overhead to fixing the warnings. Kernel text size increase,
>> possibly some CPU overhead from parsing the format string. Hopefully
>> none of these calls are in really hot code paths ;-)
>
> Actually, I would really suspect that CPU time would *decrease* for
> most of these, because instead of groveling through the whole argument
> string looking for '%' symbols we would do a single "%s" lookup
> followed by a basic strncpy(dmesg_buffer + offset, arg,
> dmesg_buffer_size - offset);
>

True enough.
Still increasing kernel text in a number of cases.

I've been thinking that in a lot of them (most of the 'banner' and
'version' substitutions, basically) I can probably get away with
moving the string argument into the function that uses it without
losing the __initdata section advantage - it just gets moved from an
__initdata segment into an __init declared function anyway.. or
something that should be declared __init at any rate since what the
hell is it using __initdata segment data for otherwise?

Unless some expert on these section markers can dispute that line of
reasoning, anyway ;-)
It means redoing most - possibly all - of the trivial patch though.

Getting rid if a few warnings most people don't even get is more work
than I thought. And to think I was considering looking into other
types of warnings as well ..
I should consider it a nice exercise in learning kernel development
practices I guess ..

Regards,
Floris
---
"They that give up essential liberty to obtain temporary safety,
deserve neither liberty nor safety."
  -- Ben Franklin

"The course of history shows that as a government grows, liberty
decreases."
  -- Thomas Jefferson

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Pavel Machek]

On Wed 2009-02-04 14:26:45, Roland Dreier wrote:
>  > Before judging on this patch could you please post what warning it
>  > triggers and one or a few patches to fix some of them.
> 
> The warnings are things like:
> 
>     init/main.c: In function 'start_kernel':
>     init/main.c:557: warning: format not a string literal and no format arguments
> 

Would it be possible to declare linux_banner const or something?

> diff --git a/init/main.c b/init/main.c
> index 8442094..78fc0d8 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -554,7 +554,7 @@ asmlinkage void __init start_kernel(void)
>  	boot_cpu_init();
>  	page_address_init();
>  	printk(KERN_NOTICE);
> -	printk(linux_banner);
> +	printk("%s", linux_banner);

Actually 
	 printk(KERN_NOTICE "%s", linux_banner);

is a better fix that will save some code, too.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Floris Kraak]

On Tue, Feb 10, 2009 at 9:24 PM, Pavel Machek <pavel@suse.cz> wrote:
> On Wed 2009-02-04 14:26:45, Roland Dreier wrote:
>>  > Before judging on this patch could you please post what warning it
>>  > triggers and one or a few patches to fix some of them.
>>
>> The warnings are things like:
>>
>>     init/main.c: In function 'start_kernel':
>>     init/main.c:557: warning: format not a string literal and no format arguments
>>
>
> Would it be possible to declare linux_banner const or something?
>
>> diff --git a/init/main.c b/init/main.c
>> index 8442094..78fc0d8 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -554,7 +554,7 @@ asmlinkage void __init start_kernel(void)
>>       boot_cpu_init();
>>       page_address_init();
>>       printk(KERN_NOTICE);
>> -     printk(linux_banner);
>> +     printk("%s", linux_banner);
>
> Actually
>         printk(KERN_NOTICE "%s", linux_banner);
>
> is a better fix that will save some code, too.
>

Noted. Adding that to the patch tomorrow.

Regards,
Floris
---
"They that give up essential liberty to obtain temporary safety,
deserve neither liberty nor safety."
  -- Ben Franklin

"The course of history shows that as a government grows, liberty
decreases."
  -- Thomas Jefferson

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Pekka Enberg]

Hi!

On Wed, Feb 4, 2009 at 5:28 PM, Floris Kraak <randakar@gmail.com> wrote:
> Some distributions have enabled the gcc flag -Wformat-security by default.*
> This results in a number of warnings about format arguments to
> functions, sometimes in cases where fixing the warning is not likely
> to actually fix a bug.
> Instead of hand patching a dozens of places (possibly more) that
> produce warnings that get ignored anyway we just turn off the flag in
> the Makefile.
>
> Note: Regardless of any discussion surrounding the value of this
> particular type of warning, having this show up in a few distributions
> but not in the
> vast majority of them means that this warning won't be seen by most of
> the developers who introduce the new warnings in the first place. If
> the
> kernel decides it cares about format arguments it should do so
> globally regardless of distribution. In which case I'd gladly whip up
> a patch to do
> the reverse thing and turn this thing on by default. However, such a
> patch would have to produce a follow up patch(set) which fixes each
> individual
> warning.
>
> See also:
> http://kerneltrap.org/mailarchive/linux-kernel/2008/11/20/4215134
>
> *) The ubuntu manpage for gcc states:
>
>      -Wformat-security
>          If -Wformat is specified, also warn about uses of format
> functions that represent possible security problems.  At present, this
> warns about
>          calls to "printf" and "scanf" functions where the format
> string is not a string literal and there are no format arguments, as
> in "printf
>          (foo);".  This may be a security hole if the format string
> came from untrusted input and contains %n.  (This is currently a
> subset of what
>          -Wformat-nonliteral warns about, but in future warnings may
> be added to -Wformat-security that are not included in
> -Wformat-nonliteral.)
>
>          NOTE: In Ubuntu 8.10 and later versions this option is
> enabled by default for C, C++, ObjC, ObjC++.  To disable, use
> -Wno-format-security, or
>          disable all format warnings with -Wformat=0.  To make format
> security warnings fatal, specify -Werror=format-security.
>
> Signed-off-by: Floris Kraak <randakar@gmail.com>
> ---
> diff --git a/Makefile b/Makefile
> index 7715b2c..9ee766c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -346,7 +346,8 @@ KBUILD_CPPFLAGS := -D__KERNEL__
>
>  KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
>                   -fno-strict-aliasing -fno-common \
> -                  -Werror-implicit-function-declaration
> +                  -Werror-implicit-function-declaration \
> +                  -Wno-format-security
>  KBUILD_AFLAGS   := -D__ASSEMBLY__
>
>  # Read KERNELRELEASE from include/config/kernel.release (if it exists)

Is there a reason this patch was not merged? Yes, it's clearly a
distro problem but apparently there's no easy way to turn it off.

                                           Pekka

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Floris Kraak]

On 5/15/09, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>
> On Wed, Feb 4, 2009 at 5:28 PM, Floris Kraak <randakar@gmail.com> wrote:
> > Some distributions have enabled the gcc flag -Wformat-security by default.*
> > This results in a number of warnings about format arguments to
> > functions, sometimes in cases where fixing the warning is not likely
> > to actually fix a bug.
> > Instead of hand patching a dozens of places (possibly more) that
> > produce warnings that get ignored anyway we just turn off the flag in
> > the Makefile.
> >
>
> Is there a reason this patch was not merged? Yes, it's clearly a
> distro problem but apparently there's no easy way to turn it off.
>

Well, I posted a few follow up patches that turned this one on his
head - instead of disabling the feature in GCC I attempted to hand
patch every location that caused the warning instead.

However, that is quite a large job for fixing a mere 'minor annoyance'
- there are a number of obvious places where merely changing the
definition of a 'char* foo' variable into a 'char foo[]' variable
makes the warning go away (hence easily done)but getting rid of all of
them requires some real code changes here and there. In theory all of
them are harmless but it adds up to well over 130 patches. (When
split.)

I was still in the process of triaging the whole thing into a
mergeable form when some assignment came along that caused me to drop
the whole thing on the floor. I can dig them up and repost them if you
like ;-)

Tellingly enough I didn't find any place where the warning was
actually warning about anything harmful. Maybe I just need better
glasses though ;-)

Quite honestly I still believe just disabling this check is the best
thing to do.

It would be *really* nice if printk could just check instead how many
arguments it has and refrain from parsing the format string if there
aren't any. Unfortunately that's seemingly impossible - or at least,
well beyond my abilities ;-)

Regards,
Floris
---
'Or lawyers may say, “But if I decline, someone else will do it. So
what is gained?” My reply: “Let someone else do it. But not you. Honor
is personal. Worry about yourself.  You don’t get a pass from moral
responsibility because you acted for a client.”

That’s the first lesson I would offer, aimed at lawyers. A second
lesson, aimed at all, is this: Keep ready your capacity for outrage.
This is very important. Next to the vote, outrage is the one response
each of us can contribute. Outrage is how honor must confront
dishonor. If we lose the capacity for outrage, we are in serious
trouble. '
   --- Stephen Gillers

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Pekka Enberg]

On Fri, May 15, 2009 at 4:28 PM, Floris Kraak <randakar@gmail.com> wrote:
> Quite honestly I still believe just disabling this check is the best
> thing to do.

Agreed. The proposed "fixes" to silence the warning don't improve the
code and apparently in some cases they even increase kernel text.

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Floris Kraak]

On Fri, May 15, 2009 at 10:42 PM, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> On Fri, May 15, 2009 at 4:28 PM, Floris Kraak <randakar@gmail.com> wrote:
>> Quite honestly I still believe just disabling this check is the best
>> thing to do.
>
> Agreed. The proposed "fixes" to silence the warning don't improve the
> code and apparently in some cases they even increase kernel text.
>

The exercise needed to be done to prove the point though ;-)

Regards,
Floris
---
'Or lawyers may say, “But if I decline, someone else will do it. So
what is gained?” My reply: “Let someone else do it. But not you. Honor
is personal. Worry about yourself.  You don’t get a pass from moral
responsibility because you acted for a client.”

That’s the first lesson I would offer, aimed at lawyers. A second
lesson, aimed at all, is this: Keep ready your capacity for outrage.
This is very important. Next to the vote, outrage is the one response
each of us can contribute. Outrage is how honor must confront
dishonor. If we lose the capacity for outrage, we are in serious
trouble. '
   --- Stephen Gillers

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Sam Ravnborg]

On Fri, May 15, 2009 at 03:28:18PM +0200, Floris Kraak wrote:
> On 5/15/09, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> >
> > On Wed, Feb 4, 2009 at 5:28 PM, Floris Kraak <randakar@gmail.com> wrote:
> > > Some distributions have enabled the gcc flag -Wformat-security by default.*
> > > This results in a number of warnings about format arguments to
> > > functions, sometimes in cases where fixing the warning is not likely
> > > to actually fix a bug.
> > > Instead of hand patching a dozens of places (possibly more) that
> > > produce warnings that get ignored anyway we just turn off the flag in
> > > the Makefile.
> > >
> >
> > Is there a reason this patch was not merged? Yes, it's clearly a
> > distro problem but apparently there's no easy way to turn it off.
> >

I have lost the original patch to disable this check.
Could you please resend.

Thanks,
	Sam

[PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Pekka J Enberg]

From: Floris Kraak <randakar@gmail.com>

Some distributions have enabled the gcc flag -Wformat-security by default.
This results in a number of warnings about format arguments to functions,
sometimes in cases where fixing the warning is not likely to actually fix a
bug.  Instead of hand patching a dozens of places (possibly more) that produce
warnings that get ignored anyway we just turn off the flag in the Makefile.

Signed-off-by: Floris Kraak <randakar@gmail.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 Makefile |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 03373bb..5ef1f72 100644
--- a/Makefile
+++ b/Makefile
@@ -351,7 +351,8 @@ KBUILD_CPPFLAGS := -D__KERNEL__
 
 KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
 		   -fno-strict-aliasing -fno-common \
-		   -Werror-implicit-function-declaration
+		   -Werror-implicit-function-declaration \
+		   -Wno-format-security
 KBUILD_AFLAGS   := -D__ASSEMBLY__
 
 # Read KERNELRELEASE from include/config/kernel.release (if it exists)
-- 
1.6.0.4

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Floris Kraak]

On Mon, Jun 15, 2009 at 7:54 AM, Pekka J Enberg<penberg@cs.helsinki.fi> wrote:
> From: Floris Kraak <randakar@gmail.com>
>

[snip]

Thanks Pekka :-)

Regards,
Floris
---
'Or lawyers may say, “But if I decline, someone else will do it. So
what is gained?” My reply: “Let someone else do it. But not you. Honor
is personal. Worry about yourself.  You don’t get a pass from moral
responsibility because you acted for a client.”

That’s the first lesson I would offer, aimed at lawyers. A second
lesson, aimed at all, is this: Keep ready your capacity for outrage.
This is very important. Next to the vote, outrage is the one response
each of us can contribute. Outrage is how honor must confront
dishonor. If we lose the capacity for outrage, we are in serious
trouble. '
   --- Stephen Gillers

Re: [PATCH] Kbuild: Disable the -Wformat-security gcc flag

[Sam Ravnborg]

On Mon, Jun 15, 2009 at 08:54:02AM +0300, Pekka J Enberg wrote:
> From: Floris Kraak <randakar@gmail.com>
> 
> Some distributions have enabled the gcc flag -Wformat-security by default.
> This results in a number of warnings about format arguments to functions,
> sometimes in cases where fixing the warning is not likely to actually fix a
> bug.  Instead of hand patching a dozens of places (possibly more) that produce
> warnings that get ignored anyway we just turn off the flag in the Makefile.
> 
> Signed-off-by: Floris Kraak <randakar@gmail.com>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>

Applied.

	Sam