2008-12-11 03:35:33

by Nguyen Anh Quynh

[permalink] [raw]
Subject: [PATCH] fix calls to request_module()

Hi,

The request_module() function should always have the 1st param as a
format argument. So for example, request_module("i2c-powermac") should
be called as request_module("%s", "i2c-powermac"). Otherwise, new gcc
like gcc 4.3.2 on Ubuntu 8.10 would spit out a lot of warnings. This
patch fixes them all in linus-git tree.

Signed-off-by: Nguyen Anh Quynh <[email protected]>

#diffstat fix-request_module-call.linus_git_tree.patch
arch/arm/mach-pxa/am200epd.c | 2 +-
crypto/api.c | 2 +-
drivers/block/paride/paride.c | 2 +-
drivers/hid/hid-core.c | 2 +-
drivers/ide/ide-probe.c | 10 +++++-----
drivers/macintosh/therm_adt746x.c | 2 +-
drivers/macintosh/windfarm_pm112.c | 12 ++++++------
drivers/macintosh/windfarm_pm121.c | 12 ++++++------
drivers/macintosh/windfarm_pm81.c | 8 ++++----
drivers/macintosh/windfarm_pm91.c | 8 ++++----
drivers/media/video/bt8xx/bttv-cards.c | 12 ++++++------
drivers/media/video/cafe_ccic.c | 2 +-
drivers/media/video/cx18/cx18-driver.c | 2 +-
drivers/media/video/cx23885/cx23885-cards.c | 4 ++--
drivers/media/video/cx88/cx88-cards.c | 2 +-
drivers/media/video/cx88/cx88-mpeg.c | 4 ++--
drivers/media/video/cx88/cx88-video.c | 6 +++---
drivers/media/video/em28xx/em28xx-cards.c | 12 ++++++------
drivers/media/video/em28xx/em28xx-video.c | 6 +++---
drivers/media/video/ivtv/ivtv-driver.c | 2 +-
drivers/media/video/mxb.c | 10 +++++-----
drivers/media/video/pvrusb2/pvrusb2-hdw.c | 2 +-
drivers/media/video/saa7134/saa7134-core.c | 14 +++++++-------
drivers/media/video/usbvision/usbvision-i2c.c | 6 +++---
drivers/media/video/vino.c | 4 ++--
drivers/media/video/w9968cf.c | 2 +-
drivers/media/video/zoran/zoran_card.c | 8 ++++----
drivers/mtd/chips/gen_probe.c | 2 +-
drivers/net/wireless/b43/rfkill.c | 2 +-
drivers/net/wireless/b43legacy/rfkill.c | 2 +-
drivers/net/wireless/hostap/hostap_ioctl.c | 10 +++++-----
drivers/of/of_spi.c | 2 +-
drivers/parport/share.c | 2 +-
drivers/usb/misc/ftdi-elan.c | 2 +-
drivers/usb/storage/libusual.c | 2 +-
drivers/video/n411.c | 2 +-
fs/dquot.c | 2 +-
fs/gfs2/locking.c | 2 +-
net/atm/ioctl.c | 10 +++++-----
net/ieee80211/ieee80211_wx.c | 4 ++--
net/socket.c | 6 +++---
sound/core/sound.c | 2 +-
sound/oss/sequencer.c | 2 +-
sound/ppc/daca.c | 2 +-
sound/ppc/tumbler.c | 2 +-
45 files changed, 108 insertions(+), 108 deletions(-)


Attachments:
(No filename) (3.10 kB)
fix-request_module-call.linus_git_tree.patch (30.26 kB)
Download all attachments

2008-12-11 04:01:34

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Thu, Dec 11, 2008 at 12:35:21PM +0900, Nguyen Anh Quynh wrote:
> Hi,
>
> The request_module() function should always have the 1st param as a
> format argument. So for example, request_module("i2c-powermac") should
> be called as request_module("%s", "i2c-powermac"). Otherwise, new gcc
> like gcc 4.3.2 on Ubuntu 8.10 would spit out a lot of warnings. This
> patch fixes them all in linus-git tree.

... and it doesn't address the underlying problems at all. A string literal
without a single % in it is a perfectly sane and valid format. _Why_ are
we getting these warning?

2008-12-11 04:14:18

by Nguyen Anh Quynh

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Thu, Dec 11, 2008 at 1:01 PM, Al Viro <[email protected]> wrote:
> On Thu, Dec 11, 2008 at 12:35:21PM +0900, Nguyen Anh Quynh wrote:
>> Hi,
>>
>> The request_module() function should always have the 1st param as a
>> format argument. So for example, request_module("i2c-powermac") should
>> be called as request_module("%s", "i2c-powermac"). Otherwise, new gcc
>> like gcc 4.3.2 on Ubuntu 8.10 would spit out a lot of warnings. This
>> patch fixes them all in linus-git tree.
>
> ... and it doesn't address the underlying problems at all. A string literal
> without a single % in it is a perfectly sane and valid format. _Why_ are
> we getting these warning?

Hmm, I checked again, and the warnings actually lie elsewhere. I got
mistake while work with several versions at the same time. So please
ignore this patch.

Thanks,
Quynh

2008-12-11 04:15:17

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Thu, 11 Dec 2008 04:01:18 +0000 Al Viro <[email protected]> wrote:

> On Thu, Dec 11, 2008 at 12:35:21PM +0900, Nguyen Anh Quynh wrote:
> > Hi,
> >
> > The request_module() function should always have the 1st param as a
> > format argument. So for example, request_module("i2c-powermac") should
> > be called as request_module("%s", "i2c-powermac"). Otherwise, new gcc
> > like gcc 4.3.2 on Ubuntu 8.10 would spit out a lot of warnings. This
> > patch fixes them all in linus-git tree.
>
> ... and it doesn't address the underlying problems at all. A string literal
> without a single % in it is a perfectly sane and valid format. _Why_ are
> we getting these warning?

extern int request_module(const char * name, ...) __attribute__ ((format (printf, 1, 2)));

?

2008-12-11 04:23:48

by Nguyen Anh Quynh

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Thu, Dec 11, 2008 at 1:14 PM, Andrew Morton
<[email protected]> wrote:
> On Thu, 11 Dec 2008 04:01:18 +0000 Al Viro <[email protected]> wrote:
>
>> On Thu, Dec 11, 2008 at 12:35:21PM +0900, Nguyen Anh Quynh wrote:
>> > Hi,
>> >
>> > The request_module() function should always have the 1st param as a
>> > format argument. So for example, request_module("i2c-powermac") should
>> > be called as request_module("%s", "i2c-powermac"). Otherwise, new gcc
>> > like gcc 4.3.2 on Ubuntu 8.10 would spit out a lot of warnings. This
>> > patch fixes them all in linus-git tree.
>>
>> ... and it doesn't address the underlying problems at all. A string literal
>> without a single % in it is a perfectly sane and valid format. _Why_ are
>> we getting these warning?
>
> extern int request_module(const char * name, ...) __attribute__ ((format (printf, 1, 2)));
>

Sorry that after the mail of Viro, I checked again by recover a code
and recompile, but got no warning. But actually that code should not
be compiled at all.

So I checked again by fixing the code that should be compiled
(sound/core/sound.c), and can confirm that without the patch we got
warning like below:

sound/core/sound.c: In function 'snd_request_other':
sound/core/sound.c:91: warning: format not a string literal and no
format arguments

And with the patch, the warnings are all gone.

So Andrew, please consider taking the patch.

Thanks,
Quynh

2008-12-11 04:40:35

by Amos Kong

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()


* Nguyen Anh Quynh wrote:

>On Thu, Dec 11, 2008 at 1:14 PM, Andrew Morton
><[email protected]> wrote:
>> On Thu, 11 Dec 2008 04:01:18 +0000 Al Viro <[email protected]> wrote:

>>> > The request_module() function should always have the 1st param as a
>>> > format argument. So for example, request_module("i2c-powermac") should
>>> > be called as request_module("%s", "i2c-powermac"). Otherwise, new gcc
>>> > like gcc 4.3.2 on Ubuntu 8.10 would spit out a lot of warnings. This
>>> > patch fixes them all in linus-git tree.
>>>
>>> ... and it doesn't address the underlying problems at all. A string literal
>>> without a single % in it is a perfectly sane and valid format. _Why_ are
>>> we getting these warning?
>>
>> extern int request_module(const char * name, ...) __attribute__ ((format (printf, 1, 2)));
>>
>
>Sorry that after the mail of Viro, I checked again by recover a code
>and recompile, but got no warning. But actually that code should not
>be compiled at all.
>
>So I checked again by fixing the code that should be compiled
>(sound/core/sound.c), and can confirm that without the patch we got
>warning like below:
>
>sound/core/sound.c: In function 'snd_request_other':
>sound/core/sound.c:91: warning: format not a string literal and no
>format arguments

Hi,all
I also use Ubuntu 8.10, gcc version 4.3.2 (Ubuntu 4.3.2-1ubuntu11)

When I compile the latest kernel, there are more warning. Like this:

scripts/genksyms/lex.c: In function ‘yylex1’:
scripts/genksyms/lex.l:97: warning: ignoring return value of ‘fwrite’, declared with attribute warn_unused_result
scripts/mod/modpost.c: In function ‘get_markers’:
scripts/mod/modpost.c:1542: warning: ignoring return value of ‘asprintf’, declared with attribute warn_unused_result
scripts/mod/modpost.c: In function ‘add_marker’:
scripts/mod/modpost.c:1962: warning: ignoring return value of ‘asprintf’, declared with attribute warn_unused_result
scripts/kallsyms.c: In function ‘read_symbol’:
scripts/kallsyms.c:74: warning: ignoring return value of ‘fgets’, declared with attribute warn_unused_result
init/main.c: In function ‘start_kernel’:
init/main.c:571: warning: format not a string literal and no format arguments
init/initramfs.c: In function ‘populate_rootfs’:
init/initramfs.c:581: warning: format not a string literal and no format arguments
usr/gen_init_cpio.c: In function ‘cpio_mkfile’:
usr/gen_init_cpio.c:357: warning: ignoring return value of ‘fwrite’, declared with attribute warn_unused_result
arch/x86/kernel/dumpstack_32.c: In function ‘print_trace_warning_symbol’:
arch/x86/kernel/dumpstack_32.c:125: warning: format not a string literal and no format arguments
arch/x86/kernel/dumpstack_32.c: In function ‘print_trace_address’:
arch/x86/kernel/dumpstack_32.c:147: warning: format not a string literal and no format arguments
arch/x86/kernel/e820.c: In function ‘early_panic’:
arch/x86/kernel/e820.c:1172: warning: format not a string literal and no format arguments
arch/x86/kernel/e820.c:1173: warning: format not a string literal and no format arguments
kernel/power/main.c: In function ‘test_suspend’:
kernel/power/main.c:720: warning: format not a string literal and no format arguments
fs/dquot.c: In function ‘find_quota_format’:
fs/dquot.c:170: warning: format not a string literal and no format arguments
crypto/api.c: In function ‘crypto_larval_lookup’:
crypto/api.c:218: warning: format not a string literal and no format arguments
crypto/algapi.c: In function ‘crypto_lookup_template’:
crypto/algapi.c:427: warning: format not a string literal and no format arguments
...
...

--
Jianjun Kong |Happy Hacking
Homepage: http://kongove.cn
Gtalk:[email protected]

2008-12-11 04:44:12

by Nguyen Anh Quynh

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Thu, Dec 11, 2008 at 1:40 PM, Jianjun Kong <[email protected]> wrote:
>
> * Nguyen Anh Quynh wrote:
>
>>On Thu, Dec 11, 2008 at 1:14 PM, Andrew Morton
>><[email protected]> wrote:
>>> On Thu, 11 Dec 2008 04:01:18 +0000 Al Viro <[email protected]> wrote:
>
>>>> > The request_module() function should always have the 1st param as a
>>>> > format argument. So for example, request_module("i2c-powermac") should
>>>> > be called as request_module("%s", "i2c-powermac"). Otherwise, new gcc
>>>> > like gcc 4.3.2 on Ubuntu 8.10 would spit out a lot of warnings. This
>>>> > patch fixes them all in linus-git tree.
>>>>
>>>> ... and it doesn't address the underlying problems at all. A string literal
>>>> without a single % in it is a perfectly sane and valid format. _Why_ are
>>>> we getting these warning?
>>>
>>> extern int request_module(const char * name, ...) __attribute__ ((format (printf, 1, 2)));
>>>
>>
>>Sorry that after the mail of Viro, I checked again by recover a code
>>and recompile, but got no warning. But actually that code should not
>>be compiled at all.
>>
>>So I checked again by fixing the code that should be compiled
>>(sound/core/sound.c), and can confirm that without the patch we got
>>warning like below:
>>
>>sound/core/sound.c: In function 'snd_request_other':
>>sound/core/sound.c:91: warning: format not a string literal and no
>>format arguments
>
> Hi,all
> I also use Ubuntu 8.10, gcc version 4.3.2 (Ubuntu 4.3.2-1ubuntu11)
>
> When I compile the latest kernel, there are more warning. Like this:
>
> scripts/genksyms/lex.c: In function 'yylex1':
> scripts/genksyms/lex.l:97: warning: ignoring return value of 'fwrite', declared with attribute warn_unused_result
> scripts/mod/modpost.c: In function 'get_markers':
> scripts/mod/modpost.c:1542: warning: ignoring return value of 'asprintf', declared with attribute warn_unused_result
> scripts/mod/modpost.c: In function 'add_marker':
> scripts/mod/modpost.c:1962: warning: ignoring return value of 'asprintf', declared with attribute warn_unused_result
> scripts/kallsyms.c: In function 'read_symbol':
> scripts/kallsyms.c:74: warning: ignoring return value of 'fgets', declared with attribute warn_unused_result
> init/main.c: In function 'start_kernel':
> init/main.c:571: warning: format not a string literal and no format arguments
> init/initramfs.c: In function 'populate_rootfs':
> init/initramfs.c:581: warning: format not a string literal and no format arguments
> usr/gen_init_cpio.c: In function 'cpio_mkfile':
> usr/gen_init_cpio.c:357: warning: ignoring return value of 'fwrite', declared with attribute warn_unused_result
> arch/x86/kernel/dumpstack_32.c: In function 'print_trace_warning_symbol':
> arch/x86/kernel/dumpstack_32.c:125: warning: format not a string literal and no format arguments
> arch/x86/kernel/dumpstack_32.c: In function 'print_trace_address':
> arch/x86/kernel/dumpstack_32.c:147: warning: format not a string literal and no format arguments
> arch/x86/kernel/e820.c: In function 'early_panic':
> arch/x86/kernel/e820.c:1172: warning: format not a string literal and no format arguments
> arch/x86/kernel/e820.c:1173: warning: format not a string literal and no format arguments
> kernel/power/main.c: In function 'test_suspend':
> kernel/power/main.c:720: warning: format not a string literal and no format arguments
> fs/dquot.c: In function 'find_quota_format':
> fs/dquot.c:170: warning: format not a string literal and no format arguments
> crypto/api.c: In function 'crypto_larval_lookup':
> crypto/api.c:218: warning: format not a string literal and no format arguments
> crypto/algapi.c: In function 'crypto_lookup_template':
> crypto/algapi.c:427: warning: format not a string literal and no format arguments
> ...

I saw the same thing. My patch only fixed part of them.

So please submit your patch to fix the remainig :-)

Q

2008-12-11 04:49:18

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Wed, Dec 10, 2008 at 08:14:55PM -0800, Andrew Morton wrote:
> On Thu, 11 Dec 2008 04:01:18 +0000 Al Viro <[email protected]> wrote:
>
> > On Thu, Dec 11, 2008 at 12:35:21PM +0900, Nguyen Anh Quynh wrote:
> > > Hi,
> > >
> > > The request_module() function should always have the 1st param as a
> > > format argument. So for example, request_module("i2c-powermac") should
> > > be called as request_module("%s", "i2c-powermac"). Otherwise, new gcc
> > > like gcc 4.3.2 on Ubuntu 8.10 would spit out a lot of warnings. This
> > > patch fixes them all in linus-git tree.
> >
> > ... and it doesn't address the underlying problems at all. A string literal
> > without a single % in it is a perfectly sane and valid format. _Why_ are
> > we getting these warning?
>
> extern int request_module(const char * name, ...) __attribute__ ((format (printf, 1, 2)));
>
> ?

... and request_module("i2c-powermac") should be perfectly valid, shouldn't it?
I mean, I do not believe that any gcc version would start spewing warnings
of
printf("-- \n");
and its ilk...

2008-12-11 05:00:35

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Thu, Dec 11, 2008 at 01:23:36PM +0900, Nguyen Anh Quynh wrote:

> So I checked again by fixing the code that should be compiled
> (sound/core/sound.c), and can confirm that without the patch we got
> warning like below:
>
> sound/core/sound.c: In function 'snd_request_other':
> sound/core/sound.c:91: warning: format not a string literal and no
> format arguments

Ah, but that's different. Take a look at that warning and think _why_
it is given and what is it about. Getting an untrusted string as
format argument is a real security hole, but it has nothing to do
with a pile of cases in your patch.

FWIW, even in this case (where the warning is a false positive, BTW -
the string passed there can have one of two values and both are safe)
I would simply do
switch(...) {
case .... : request_module("snd-seq"); break;
case .... : request_module("snd-timer"); break;
}
and that's it.

Slapping "%s" here actually obfuscates the code, without making it safer.

2008-12-11 05:02:21

by Amos Kong

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()


* Nguyen Anh Quynh wrote:

>On Thu, Dec 11, 2008 at 1:40 PM, Jianjun Kong <[email protected]> wrote:
>> * Nguyen Anh Quynh wrote:
>>>>> > The request_module() function should always have the 1st param as a
>>>>> > format argument. So for example, request_module("i2c-powermac") should
>>>>> > be called as request_module("%s", "i2c-powermac"). Otherwise, new gcc
>>>>> > like gcc 4.3.2 on Ubuntu 8.10 would spit out a lot of warnings. This
>>>>> > patch fixes them all in linus-git tree.

<sign>
>> Hi,all
>> I also use Ubuntu 8.10, gcc version 4.3.2 (Ubuntu 4.3.2-1ubuntu11)
>>
>> When I compile the latest kernel, there are more warning. Like this:

<sign>
>> arch/x86/kernel/e820.c: In function 'early_panic':
>> arch/x86/kernel/e820.c:1172: warning: format not a string literal and no format arguments
>> arch/x86/kernel/e820.c:1173: warning: format not a string literal and no format arguments
>> kernel/power/main.c: In function 'test_suspend':
>> kernel/power/main.c:720: warning: format not a string literal and no format arguments
>> fs/dquot.c: In function 'find_quota_format':
>> fs/dquot.c:170: warning: format not a string literal and no format arguments
>> crypto/api.c: In function 'crypto_larval_lookup':
>> crypto/api.c:218: warning: format not a string literal and no format arguments
>> crypto/algapi.c: In function 'crypto_lookup_template':
>> crypto/algapi.c:427: warning: format not a string literal and no format arguments
>> ...
>
>I saw the same thing. My patch only fixed part of them.
>
>So please submit your patch to fix the remainig :-)

But there is no this kind of warns at Fedora 10, gcc version 4.3.2 20081105 (Red Hat 4.3.2-7)

We should make clean whether your patch is correct first.
There are many more that kind of warns. I just list little of them.


Jianjun
--
Jianjun Kong |Happy Hacking
Homepage: http://kongove.cn
Gtalk:[email protected]

2008-12-11 05:03:50

by Roland Dreier

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

> I mean, I do not believe that any gcc version would start spewing warnings
> of
> printf("-- \n");
> and its ilk...

No, I haven't seen gcc warn about anything that crazy (ie where it can
see the format string and prove it's safe).

I do see warnings from the Ubuntu gcc with code like:

#include <stdio.h>

extern char *a;

void foo()
{
printf(a);
}

which produces

a.c: In function 'foo':
a.c:7: warning: format not a string literal and no format arguments


The kernel has such code eg in init/main.c, which does

printk(linux_banner);

when linux_banner is only visible to the compiler as

extern const char linux_banner[];

however the trivial fix

diff --git a/init/main.c b/init/main.c
index 7e117a2..e471598 100644
--- a/init/main.c
+++ b/init/main.c
@@ -568,7 +568,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);

doesn't seem that appealing, since it bloats the object code for a
non-bug -- 7 bytes for me on x86_64:

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

given the number of such warnings I see in a typical compile, this would
be a fairly hefty amount of bloat just to shut up gcc.

On the other hand, gcc warning on such code (untrusted format string
passed into a printf-like function) seems quite legitimate as well.

So I dunno.

- Roland

2008-12-11 05:42:03

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Wed, 10 Dec 2008 21:03:37 -0800 Roland Dreier <[email protected]> wrote:

> The kernel has such code eg in init/main.c, which does
>
> printk(linux_banner);
>
> when linux_banner is only visible to the compiler as
>
> extern const char linux_banner[];
>
> however the trivial fix
>
> diff --git a/init/main.c b/init/main.c
> index 7e117a2..e471598 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -568,7 +568,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);
>
> doesn't seem that appealing, since it bloats the object code for a
> non-bug -- 7 bytes for me on x86_64:
>
> add/remove: 0/0 grow/shrink: 1/0 up/down: 7/0 (7)
> function old new delta
> start_kernel 680 687 +7
>
> given the number of such warnings I see in a typical compile, this would
> be a fairly hefty amount of bloat just to shut up gcc.

yes, that would suck. otoh, our current warning spew actually causes bugs.

I wonder if we could add a printk_stfu() which isn't declared
attribute(printf) and which simply calls printk. We might still get a
single warning at the interface point.

> On the other hand, gcc warning on such code (untrusted format string
> passed into a printf-like function) seems quite legitimate as well.

Yes, we've had actual bugs in the kernel from this, where the control
string was user-provided. root-only user, fortunately.

2008-12-11 05:42:39

by Nguyen Anh Quynh

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Thu, Dec 11, 2008 at 2:00 PM, Al Viro <[email protected]> wrote:
> On Thu, Dec 11, 2008 at 01:23:36PM +0900, Nguyen Anh Quynh wrote:
>
>> So I checked again by fixing the code that should be compiled
>> (sound/core/sound.c), and can confirm that without the patch we got
>> warning like below:
>>
>> sound/core/sound.c: In function 'snd_request_other':
>> sound/core/sound.c:91: warning: format not a string literal and no
>> format arguments
>
> Ah, but that's different. Take a look at that warning and think _why_
> it is given and what is it about. Getting an untrusted string as
> format argument is a real security hole, but it has nothing to do
> with a pile of cases in your patch.

Yes, clearly the warning is to warn us about potential format string
bugs. But I agree that there are a lot of false possitives.

My patch is mainly to make gcc happy.

Thanks,
Q

2008-12-11 06:00:53

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Thu, Dec 11, 2008 at 02:42:28PM +0900, Nguyen Anh Quynh wrote:
> > Ah, but that's different. Take a look at that warning and think _why_
> > it is given and what is it about. Getting an untrusted string as
> > format argument is a real security hole, but it has nothing to do
> > with a pile of cases in your patch.
>
> Yes, clearly the warning is to warn us about potential format string
> bugs. But I agree that there are a lot of false possitives.
>
> My patch is mainly to make gcc happy.

Your patch is mostly obfuscating the places gcc does *not* warn about...

Looking through it, only
drivers/media/video/cx18/cx18-driver.c
drivers/media/video/ivtv/ivtv-driver.c
drivers/media/video/pvrusb2/pvrusb2-hdw.c
drivers/media/video/zoran/zoran_card.c
drivers/mtd/chips/gen_probe.c
drivers/net/wireless/hostap/hostap_ioctl.c
drivers/of/of_spi.c
drivers/usb/storage/libusual.c
fs/dquot.c
fs/gfs2/locking.c
net/ieee80211/ieee80211_wx.c
sound/core/sound.c

are not of <string literal> -> "%s", <string literal> variety. Everything
else has never generated a format warning. At least trim the patch,
removing the obviously useless parts.

2008-12-11 06:29:19

by Nguyen Anh Quynh

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

On Thu, Dec 11, 2008 at 3:00 PM, Al Viro <[email protected]> wrote:
> On Thu, Dec 11, 2008 at 02:42:28PM +0900, Nguyen Anh Quynh wrote:
>> > Ah, but that's different. Take a look at that warning and think _why_
>> > it is given and what is it about. Getting an untrusted string as
>> > format argument is a real security hole, but it has nothing to do
>> > with a pile of cases in your patch.
>>
>> Yes, clearly the warning is to warn us about potential format string
>> bugs. But I agree that there are a lot of false possitives.
>>
>> My patch is mainly to make gcc happy.
>
> Your patch is mostly obfuscating the places gcc does *not* warn about...
>
> Looking through it, only
> drivers/media/video/cx18/cx18-driver.c
> drivers/media/video/ivtv/ivtv-driver.c
> drivers/media/video/pvrusb2/pvrusb2-hdw.c
> drivers/media/video/zoran/zoran_card.c
> drivers/mtd/chips/gen_probe.c
> drivers/net/wireless/hostap/hostap_ioctl.c
> drivers/of/of_spi.c
> drivers/usb/storage/libusual.c
> fs/dquot.c
> fs/gfs2/locking.c
> net/ieee80211/ieee80211_wx.c
> sound/core/sound.c
>
> are not of <string literal> -> "%s", <string literal> variety. Everything
> else has never generated a format warning. At least trim the patch,
> removing the obviously useless parts.

Yes, initially I misunderstood these warnings.

So here is the patch, again, with only the "correct" warning fixed.

Signed-off-by: Nguyen Anh Quynh <[email protected]>

Thanks,
Quynh


# diffstat fix-request_module-call.linus_git_tree.patch
drivers/media/video/cx18/cx18-driver.c | 2 +-
drivers/media/video/ivtv/ivtv-driver.c | 2 +-
drivers/media/video/pvrusb2/pvrusb2-hdw.c | 2 +-
drivers/media/video/zoran/zoran_card.c | 8 ++++----
drivers/mtd/chips/gen_probe.c | 2 +-
drivers/net/wireless/hostap/hostap_ioctl.c | 2 +-
drivers/of/of_spi.c | 2 +-
drivers/usb/storage/libusual.c | 2 +-
fs/dquot.c | 2 +-
fs/gfs2/locking.c | 2 +-
net/ieee80211/ieee80211_wx.c | 2 +-
sound/core/sound.c | 2 +-
12 files changed, 15 insertions(+), 15 deletions(-)


Attachments:
(No filename) (2.16 kB)
fix-request_module-call.linus_git_tree.patch (6.52 kB)
Download all attachments

2008-12-11 23:25:20

by Roland Dreier

[permalink] [raw]
Subject: Re: [PATCH] fix calls to request_module()

> I wonder if we could add a printk_stfu() which isn't declared
> attribute(printf) and which simply calls printk. We might still get a
> single warning at the interface point.

That would fix the class of warnings like

printk(char_pointer_gcc_doesnt_know_is_safe);

but therre are alsofunctions like device_create() etc etc where someone
could easily and legitimately do the same thing.

- R.