Re: [PATCH] modpost: detect unterminated device id lists
Hi Andrew,
Em Seg, 2007-09-17 às 14:50 -0700, Andrew Morton escreveu:
> On Tue, 18 Sep 2007 03:15:14 +0530 (IST)
> Satyam Sharma <[EMAIL PROTECTED]> wrote:
>
> >
> >
> > On Sun, 16 Sep 2007, Andrew Morton wrote:
> >
> > > On Mon, 17 Sep 2007 05:54:45 +0530 "Satyam Sharma" <[EMAIL PROTECTED]>
> > > wrote:
> > >
> > > > On 9/17/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
> > > > >
> > > > > I'm getting this:
> > > > >
> > > > > rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
> > > > > 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> > > > > 0x00
> > > > > 0x00 0x00 0x00 0x00 0x00
> > > > > FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is
> > > > > not terminated
> > > > > with a NULL entry!
> > > > >
> > > > > ("rusb2/pvrusb2" ??)
> > > >
> > > > Hmm? Are you sure you didn't see any "drivers/media/video/pv" before the
> > > > "rusb2/pvrusb2" bit?
> > >
> > > Fairly. I looked twice.
> >
> > "drivers/media/video/pvrusb2/pvrusb2" comes out correctly here ...
> >
> >
> > > > Looking at Kees' patch (and the existing code), I've no
> > > > clue how/why this should happen ... will try to reproduce here ...
> > > >
> > > >
> > > > > but:
> > > > >
> > > > > struct usb_device_id pvr2_device_table[] = {
> > > > > [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
> > > > > [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> > > > > { USB_DEVICE(0, 0) },
> > > > > };
> > > > >
> > > > > looks OK?
> > > > >
> > > > > Using plain old "{ }" shut the warning up.
> > > >
> > > > USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
> > > > a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
> > > > assigns "0x03 0x00" (in little endian) to usb_device_id.match_flags. And
> > > > I don't think the USB code treats such an entry as an empty entry (?)
> > > >
> > > > Interestingly, the "USB_DEVICE(0, 0)" thing is absent from latest -git
> > > > tree and also in my copy of 23-rc4-mm1 -- so this looks like something
> > > > you must've merged recently.
> > >
> > > git-dvb very carefully does
> > >
> > > --- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb
> > > +++ a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> > > @@ -44,7 +44,7 @@
> > > struct usb_device_id pvr2_device_table[] = {
> > > [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
> > > [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> > > - { }
> > > + { USB_DEVICE(0, 0) },
> > > };
> > >
> > > MODULE_DEVICE_TABLE(usb, pvr2_device_table);
> >
> > Ok, this is a false positive indeed, the core USB code does in fact
> > treat such an entry as an empty entry (usb_match_id() tests only the
> > .idVendor, .bDeviceClass, .bInterfaceClass and .driver_info members
> > for non-zero and not the .match_flags member).
> >
> > However, a quick-grep-and-glance tells us that none of the other 2213
> > occurrences of USB_DEVICE() in the tree ever do this "(0,0)" thing,
> > so it does make sense to change this one to a simple "{ }" as well --
> > that's clearer style anyway, and the "standard" way to empty-terminate
> > in the rest of the tree, if nothing else.
> >
>
> yeah, I think so. Mauro, could you please drop that change?
Patch dropped from my tree.
Cheers,
Mauro.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Tue, 18 Sep 2007 03:15:14 +0530 (IST)
Satyam Sharma <[EMAIL PROTECTED]> wrote:
>
>
> On Sun, 16 Sep 2007, Andrew Morton wrote:
>
> > On Mon, 17 Sep 2007 05:54:45 +0530 "Satyam Sharma" <[EMAIL PROTECTED]>
> > wrote:
> >
> > > On 9/17/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
> > > >
> > > > I'm getting this:
> > > >
> > > > rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
> > > > 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> > > > 0x00
> > > > 0x00 0x00 0x00 0x00 0x00
> > > > FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not
> > > > terminated
> > > > with a NULL entry!
> > > >
> > > > ("rusb2/pvrusb2" ??)
> > >
> > > Hmm? Are you sure you didn't see any "drivers/media/video/pv" before the
> > > "rusb2/pvrusb2" bit?
> >
> > Fairly. I looked twice.
>
> "drivers/media/video/pvrusb2/pvrusb2" comes out correctly here ...
>
>
> > > Looking at Kees' patch (and the existing code), I've no
> > > clue how/why this should happen ... will try to reproduce here ...
> > >
> > >
> > > > but:
> > > >
> > > > struct usb_device_id pvr2_device_table[] = {
> > > > [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
> > > > [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> > > > { USB_DEVICE(0, 0) },
> > > > };
> > > >
> > > > looks OK?
> > > >
> > > > Using plain old "{ }" shut the warning up.
> > >
> > > USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
> > > a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
> > > assigns "0x03 0x00" (in little endian) to usb_device_id.match_flags. And
> > > I don't think the USB code treats such an entry as an empty entry (?)
> > >
> > > Interestingly, the "USB_DEVICE(0, 0)" thing is absent from latest -git
> > > tree and also in my copy of 23-rc4-mm1 -- so this looks like something
> > > you must've merged recently.
> >
> > git-dvb very carefully does
> >
> > --- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb
> > +++ a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> > @@ -44,7 +44,7 @@
> > struct usb_device_id pvr2_device_table[] = {
> > [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
> > [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> > - { }
> > + { USB_DEVICE(0, 0) },
> > };
> >
> > MODULE_DEVICE_TABLE(usb, pvr2_device_table);
>
> Ok, this is a false positive indeed, the core USB code does in fact
> treat such an entry as an empty entry (usb_match_id() tests only the
> .idVendor, .bDeviceClass, .bInterfaceClass and .driver_info members
> for non-zero and not the .match_flags member).
>
> However, a quick-grep-and-glance tells us that none of the other 2213
> occurrences of USB_DEVICE() in the tree ever do this "(0,0)" thing,
> so it does make sense to change this one to a simple "{ }" as well --
> that's clearer style anyway, and the "standard" way to empty-terminate
> in the rest of the tree, if nothing else.
>
yeah, I think so. Mauro, could you please drop that change?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Sun, 16 Sep 2007, Andrew Morton wrote:
> On Mon, 17 Sep 2007 05:54:45 +0530 "Satyam Sharma" <[EMAIL PROTECTED]> wrote:
>
> > On 9/17/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
> > >
> > > I'm getting this:
> > >
> > > rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
> > > 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> > > 0x00 0x00 0x00 0x00 0x00
> > > FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not
> > > terminated
> > > with a NULL entry!
> > >
> > > ("rusb2/pvrusb2" ??)
> >
> > Hmm? Are you sure you didn't see any "drivers/media/video/pv" before the
> > "rusb2/pvrusb2" bit?
>
> Fairly. I looked twice.
"drivers/media/video/pvrusb2/pvrusb2" comes out correctly here ...
> > Looking at Kees' patch (and the existing code), I've no
> > clue how/why this should happen ... will try to reproduce here ...
> >
> >
> > > but:
> > >
> > > struct usb_device_id pvr2_device_table[] = {
> > > [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
> > > [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> > > { USB_DEVICE(0, 0) },
> > > };
> > >
> > > looks OK?
> > >
> > > Using plain old "{ }" shut the warning up.
> >
> > USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
> > a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
> > assigns "0x03 0x00" (in little endian) to usb_device_id.match_flags. And
> > I don't think the USB code treats such an entry as an empty entry (?)
> >
> > Interestingly, the "USB_DEVICE(0, 0)" thing is absent from latest -git
> > tree and also in my copy of 23-rc4-mm1 -- so this looks like something
> > you must've merged recently.
>
> git-dvb very carefully does
>
> --- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb
> +++ a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> @@ -44,7 +44,7 @@
> struct usb_device_id pvr2_device_table[] = {
> [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
> [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> - { }
> + { USB_DEVICE(0, 0) },
> };
>
> MODULE_DEVICE_TABLE(usb, pvr2_device_table);
Ok, this is a false positive indeed, the core USB code does in fact
treat such an entry as an empty entry (usb_match_id() tests only the
.idVendor, .bDeviceClass, .bInterfaceClass and .driver_info members
for non-zero and not the .match_flags member).
However, a quick-grep-and-glance tells us that none of the other 2213
occurrences of USB_DEVICE() in the tree ever do this "(0,0)" thing,
so it does make sense to change this one to a simple "{ }" as well --
that's clearer style anyway, and the "standard" way to empty-terminate
in the rest of the tree, if nothing else.
Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Sun, 16 Sep 2007 20:45:54 -0700 Kees Cook <[EMAIL PROTECTED]> wrote: > The cleanups you suggested, who should I send those to? Or will you (or > Sam?) make them directly to the kbuild.git tree? (I've never poked at > this part of the kernel source before... I'm unclear on the processes > surrounding it maintainership.) If they hit my inbox they won't get lost.. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Mon, 17 Sep 2007 05:54:45 +0530 "Satyam Sharma" <[EMAIL PROTECTED]> wrote:
> On 9/17/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
> > On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook <[EMAIL PROTECTED]> wrote:
> >
> > > This patch against 2.6.23-rc6 will cause modpost to fail if any device
> > > id lists are incorrectly terminated, after reporting the offender.
> >
> > I'm getting this:
> >
> > rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
> > 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> > 0x00 0x00 0x00 0x00 0x00
> > FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not
> > terminated
> > with a NULL entry!
> >
> > ("rusb2/pvrusb2" ??)
>
> Hmm? Are you sure you didn't see any "drivers/media/video/pv" before the
> "rusb2/pvrusb2" bit?
Fairly. I looked twice.
> Looking at Kees' patch (and the existing code), I've no
> clue how/why this should happen ... will try to reproduce here ...
>
>
> > but:
> >
> > struct usb_device_id pvr2_device_table[] = {
> > [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
> > [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> > { USB_DEVICE(0, 0) },
> > };
> >
> > looks OK?
> >
> > Using plain old "{ }" shut the warning up.
>
> USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
> a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
> assigns "0x03 0x00" (in little endian) to usb_device_id.match_flags. And
> I don't think the USB code treats such an entry as an empty entry (?)
>
> Interestingly, the "USB_DEVICE(0, 0)" thing is absent from latest -git
> tree and also in my copy of 23-rc4-mm1 -- so this looks like something
> you must've merged recently.
git-dvb very carefully does
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb
+++ a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
@@ -44,7 +44,7 @@
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
- { }
+ { USB_DEVICE(0, 0) },
};
MODULE_DEVICE_TABLE(usb, pvr2_device_table);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Mon, 17 Sep 2007 05:54:45 +0530 Satyam Sharma [EMAIL PROTECTED] wrote:
On 9/17/07, Andrew Morton [EMAIL PROTECTED] wrote:
On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook [EMAIL PROTECTED] wrote:
This patch against 2.6.23-rc6 will cause modpost to fail if any device
id lists are incorrectly terminated, after reporting the offender.
I'm getting this:
rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x00 0x00 0x00 0x00 0x00
FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not
terminated
with a NULL entry!
(rusb2/pvrusb2 ??)
Hmm? Are you sure you didn't see any drivers/media/video/pv before the
rusb2/pvrusb2 bit?
Fairly. I looked twice.
Looking at Kees' patch (and the existing code), I've no
clue how/why this should happen ... will try to reproduce here ...
but:
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
{ USB_DEVICE(0, 0) },
};
looks OK?
Using plain old { } shut the warning up.
USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
assigns 0x03 0x00 (in little endian) to usb_device_id.match_flags. And
I don't think the USB code treats such an entry as an empty entry (?)
Interestingly, the USB_DEVICE(0, 0) thing is absent from latest -git
tree and also in my copy of 23-rc4-mm1 -- so this looks like something
you must've merged recently.
git-dvb very carefully does
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb
+++ a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
@@ -44,7 +44,7 @@
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
- { }
+ { USB_DEVICE(0, 0) },
};
MODULE_DEVICE_TABLE(usb, pvr2_device_table);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Sun, 16 Sep 2007 20:45:54 -0700 Kees Cook [EMAIL PROTECTED] wrote: The cleanups you suggested, who should I send those to? Or will you (or Sam?) make them directly to the kbuild.git tree? (I've never poked at this part of the kernel source before... I'm unclear on the processes surrounding it maintainership.) If they hit my inbox they won't get lost.. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Sun, 16 Sep 2007, Andrew Morton wrote:
On Mon, 17 Sep 2007 05:54:45 +0530 Satyam Sharma [EMAIL PROTECTED] wrote:
On 9/17/07, Andrew Morton [EMAIL PROTECTED] wrote:
I'm getting this:
rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x00 0x00 0x00 0x00 0x00
FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not
terminated
with a NULL entry!
(rusb2/pvrusb2 ??)
Hmm? Are you sure you didn't see any drivers/media/video/pv before the
rusb2/pvrusb2 bit?
Fairly. I looked twice.
drivers/media/video/pvrusb2/pvrusb2 comes out correctly here ...
Looking at Kees' patch (and the existing code), I've no
clue how/why this should happen ... will try to reproduce here ...
but:
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
{ USB_DEVICE(0, 0) },
};
looks OK?
Using plain old { } shut the warning up.
USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
assigns 0x03 0x00 (in little endian) to usb_device_id.match_flags. And
I don't think the USB code treats such an entry as an empty entry (?)
Interestingly, the USB_DEVICE(0, 0) thing is absent from latest -git
tree and also in my copy of 23-rc4-mm1 -- so this looks like something
you must've merged recently.
git-dvb very carefully does
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb
+++ a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
@@ -44,7 +44,7 @@
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
- { }
+ { USB_DEVICE(0, 0) },
};
MODULE_DEVICE_TABLE(usb, pvr2_device_table);
Ok, this is a false positive indeed, the core USB code does in fact
treat such an entry as an empty entry (usb_match_id() tests only the
.idVendor, .bDeviceClass, .bInterfaceClass and .driver_info members
for non-zero and not the .match_flags member).
However, a quick-grep-and-glance tells us that none of the other 2213
occurrences of USB_DEVICE() in the tree ever do this (0,0) thing,
so it does make sense to change this one to a simple { } as well --
that's clearer style anyway, and the standard way to empty-terminate
in the rest of the tree, if nothing else.
Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Tue, 18 Sep 2007 03:15:14 +0530 (IST)
Satyam Sharma [EMAIL PROTECTED] wrote:
On Sun, 16 Sep 2007, Andrew Morton wrote:
On Mon, 17 Sep 2007 05:54:45 +0530 Satyam Sharma [EMAIL PROTECTED]
wrote:
On 9/17/07, Andrew Morton [EMAIL PROTECTED] wrote:
I'm getting this:
rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x00
0x00 0x00 0x00 0x00 0x00
FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not
terminated
with a NULL entry!
(rusb2/pvrusb2 ??)
Hmm? Are you sure you didn't see any drivers/media/video/pv before the
rusb2/pvrusb2 bit?
Fairly. I looked twice.
drivers/media/video/pvrusb2/pvrusb2 comes out correctly here ...
Looking at Kees' patch (and the existing code), I've no
clue how/why this should happen ... will try to reproduce here ...
but:
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
{ USB_DEVICE(0, 0) },
};
looks OK?
Using plain old { } shut the warning up.
USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
assigns 0x03 0x00 (in little endian) to usb_device_id.match_flags. And
I don't think the USB code treats such an entry as an empty entry (?)
Interestingly, the USB_DEVICE(0, 0) thing is absent from latest -git
tree and also in my copy of 23-rc4-mm1 -- so this looks like something
you must've merged recently.
git-dvb very carefully does
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb
+++ a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
@@ -44,7 +44,7 @@
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
- { }
+ { USB_DEVICE(0, 0) },
};
MODULE_DEVICE_TABLE(usb, pvr2_device_table);
Ok, this is a false positive indeed, the core USB code does in fact
treat such an entry as an empty entry (usb_match_id() tests only the
.idVendor, .bDeviceClass, .bInterfaceClass and .driver_info members
for non-zero and not the .match_flags member).
However, a quick-grep-and-glance tells us that none of the other 2213
occurrences of USB_DEVICE() in the tree ever do this (0,0) thing,
so it does make sense to change this one to a simple { } as well --
that's clearer style anyway, and the standard way to empty-terminate
in the rest of the tree, if nothing else.
yeah, I think so. Mauro, could you please drop that change?
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
Hi Andrew,
Em Seg, 2007-09-17 às 14:50 -0700, Andrew Morton escreveu:
On Tue, 18 Sep 2007 03:15:14 +0530 (IST)
Satyam Sharma [EMAIL PROTECTED] wrote:
On Sun, 16 Sep 2007, Andrew Morton wrote:
On Mon, 17 Sep 2007 05:54:45 +0530 Satyam Sharma [EMAIL PROTECTED]
wrote:
On 9/17/07, Andrew Morton [EMAIL PROTECTED] wrote:
I'm getting this:
rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x00
0x00 0x00 0x00 0x00 0x00
FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is
not terminated
with a NULL entry!
(rusb2/pvrusb2 ??)
Hmm? Are you sure you didn't see any drivers/media/video/pv before the
rusb2/pvrusb2 bit?
Fairly. I looked twice.
drivers/media/video/pvrusb2/pvrusb2 comes out correctly here ...
Looking at Kees' patch (and the existing code), I've no
clue how/why this should happen ... will try to reproduce here ...
but:
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
{ USB_DEVICE(0, 0) },
};
looks OK?
Using plain old { } shut the warning up.
USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
assigns 0x03 0x00 (in little endian) to usb_device_id.match_flags. And
I don't think the USB code treats such an entry as an empty entry (?)
Interestingly, the USB_DEVICE(0, 0) thing is absent from latest -git
tree and also in my copy of 23-rc4-mm1 -- so this looks like something
you must've merged recently.
git-dvb very carefully does
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb
+++ a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
@@ -44,7 +44,7 @@
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
- { }
+ { USB_DEVICE(0, 0) },
};
MODULE_DEVICE_TABLE(usb, pvr2_device_table);
Ok, this is a false positive indeed, the core USB code does in fact
treat such an entry as an empty entry (usb_match_id() tests only the
.idVendor, .bDeviceClass, .bInterfaceClass and .driver_info members
for non-zero and not the .match_flags member).
However, a quick-grep-and-glance tells us that none of the other 2213
occurrences of USB_DEVICE() in the tree ever do this (0,0) thing,
so it does make sense to change this one to a simple { } as well --
that's clearer style anyway, and the standard way to empty-terminate
in the rest of the tree, if nothing else.
yeah, I think so. Mauro, could you please drop that change?
Patch dropped from my tree.
Cheers,
Mauro.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
Hi Satyam,
On Mon, Sep 17, 2007 at 06:52:52AM +0530, Satyam Sharma wrote:
> On 9/13/07, Kees Cook <[EMAIL PROTECTED]> wrote:
> >
> > This patch against 2.6.23-rc6 will cause modpost to fail if any device
> > id lists are incorrectly terminated, after reporting the offender.
> >
> > Signed-off-by: Kees Cook <[EMAIL PROTECTED]>
>
> Nice! :-)
>
> BTW a very similar idea (but for a different problem) was discussed in:
> http://lkml.org/lkml/2007/8/23/48
>
> I tried doing something about that, but gave up in between. For the
> device_id tables, a lot of infrastructure/code already exists in modpost,
> but no such luck for kobjects :-( Still, if you can do something about
> that, as he mentioned, I bet Greg would gladly accept such a patch :-)
Thanks! Hmm, perhaps I'll take that on when I learn kobjects better. :)
> > +static void device_id_check(const char *modname, const char *device_id,
> > + unsigned long size, unsigned long id_size,
> > + void *symval)
>
> If you pass the Elf_Sym *sym all the way from handle_moddevtable() (which
> means you can get rid of the sym->st_size argument in the call chain), then
> it would be possible to print out the *symbol name* too here ...
That's true. I actually threw away an earlier version of this patch
that made some extensive changes to the elf parser (due to the NOBITS
thing I explain below), but instead opted for the smaller version that
stayed out of there.
> for (p = symval+size-id_size; p < symval+size; p++) {
> if (*p) {
>
> is probably clearer ?
Ah, yeah, that's much nicer. I think I must have still have my ELF
parser hat on when I wrote that. ;)
> As I just said, printing out just the modname and device_id "type" sounds
> insufficient here. Note that they were sufficient before your patch, because
> previously, this function only checked if the device_id *type* itself was
> incorrectly defined. But here we're talking about a specific errant *symbol*.
That's true, yeah.
> > + fprintf(stderr,"\n");
> > + fatal("%s: struct %s_device_id is not terminated "
> > + "with a NULL entry!\n", modname, device_id);
>
> Subtle nit, but it's not really a "NULL" entry. It's an "empty object" entry,
> not a "NULL" pointer ... how about replacing "a NULL" with "an empty" ?
Yeah, that bugged me when I put that in too. :) Yes, "an empty" is
better.
> > @@ -527,14 +542,22 @@ void handle_moddevtable(struct module *m
> > Elf_Sym *sym, const char *symname)
> > {
> > void *symval;
> > + char *zeros = NULL;
> >
> > /* We're looking for a section relative symbol */
> > if (!sym->st_shndx || sym->st_shndx >= info->hdr->e_shnum)
> > return;
> >
> > - symval = (void *)info->hdr
> > - + info->sechdrs[sym->st_shndx].sh_offset
> > - + sym->st_value;
> > + /* Handle all-NULL symbols allocated into .bss */
> > + if (info->sechdrs[sym->st_shndx].sh_type & SHT_NOBITS) {
> > + zeros = calloc(1, sym->st_size);
> > + symval = zeros;
> > + }
>
> Hmm, I don't quite grok this case. Care to explain?
In the ELF format, the .bss segment is initialized by the loader to all
zeros, but it contains no in-file representation (since it's all zeros
and would be a waste of space). Such segments are flags as "SHT_NOBITS"
meaning that the loader must allocate cleared memory instead of loading
the segment from the file.
In the case of modules that have a totally empty *_device_id list (?!),
the compiler optimizes this into the .bss segment, since there is no need
to store an all-zero-contents object in a segment that would be loaded
from the file itself.
As a result, attempting to dereference such a symbol without noticing
the SHT_NOBITS flag lands you somewhere in uninitialized memory. So,
the above code basically side-steps the incorrect symbol location
calculation and just aims it at a cleared part of memory.
As I mentioned, there was a larger patch that attempted to sort this
out in the elf parser, but I didn't like it; it was big, not 100%
correct, and the above approach seemed like a much less invasive change.
The cleanups you suggested, who should I send those to? Or will you (or
Sam?) make them directly to the kbuild.git tree? (I've never poked at
this part of the kernel source before... I'm unclear on the processes
surrounding it maintainership.)
Thanks!
-Kees
--
Kees Cook
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
Hi Kees,
On 9/13/07, Kees Cook <[EMAIL PROTECTED]> wrote:
>
> This patch against 2.6.23-rc6 will cause modpost to fail if any device
> id lists are incorrectly terminated, after reporting the offender.
>
> Signed-off-by: Kees Cook <[EMAIL PROTECTED]>
Nice! :-)
BTW a very similar idea (but for a different problem) was discussed in:
http://lkml.org/lkml/2007/8/23/48
I tried doing something about that, but gave up in between. For the
device_id tables, a lot of infrastructure/code already exists in modpost,
but no such luck for kobjects :-( Still, if you can do something about
that, as he mentioned, I bet Greg would gladly accept such a patch :-)
> --- linux-2.6.23-rc6~/scripts/mod/file2alias.c 2007-09-11 23:17:49.0
> -0700
> +++ linux-2.6.23-rc6/scripts/mod/file2alias.c 2007-09-12 17:41:30.0
> -0700
> @@ -55,10 +55,13 @@ do {
> * Check that sizeof(device_id type) are consistent with size of section
> * in .o file. If in-consistent then userspace and kernel does not agree
> * on actual size which is a bug.
> + * Also verify that the final entry in the table is all zeros.
> **/
> -static void device_id_size_check(const char *modname, const char *device_id,
> -unsigned long size, unsigned long id_size)
> +static void device_id_check(const char *modname, const char *device_id,
> + unsigned long size, unsigned long id_size,
> + void *symval)
If you pass the Elf_Sym *sym all the way from handle_moddevtable() (which
means you can get rid of the sym->st_size argument in the call chain), then
it would be possible to print out the *symbol name* too here ...
> {
> + int i;
uint8_t *p;
> if (size % id_size || size < id_size) {
> fatal("%s: sizeof(struct %s_device_id)=%lu is not a modulo "
> "of the size of section __mod_%s_device_table=%lu.\n"
> @@ -66,6 +69,18 @@ static void device_id_size_check(const c
> "in mod_devicetable.h\n",
> modname, device_id, id_size, device_id, size,
> device_id);
> }
> + /* Verify last one is a terminator */
> + for (i = 0; i < id_size; i++ ) {
> + if ( *(uint8_t*)(symval+size-id_size+i) ) {
... and:
for (p = symval+size-id_size; p < symval+size; p++) {
if (*p) {
is probably clearer ?
> + fprintf(stderr,"%s: struct %s_device_id is %lu bytes.
> The last of
> %lu is:\n", modname, device_id, id_size, size / id_size);
As I just said, printing out just the modname and device_id "type" sounds
insufficient here. Note that they were sufficient before your patch, because
previously, this function only checked if the device_id *type* itself was
incorrectly defined. But here we're talking about a specific errant *symbol*.
> + for (i = 0; i < id_size; i++ ) {
> + fprintf(stderr,"0x%02x ",
> *(uint8_t*)(symval+size-id_size+i) );
> + }
Again, "for (p = symval+size-id_size; p < symval+size; p++) {"
and then "fprintf(..., *p);" would be cleaner.
> + fprintf(stderr,"\n");
> + fatal("%s: struct %s_device_id is not terminated "
> + "with a NULL entry!\n", modname, device_id);
Subtle nit, but it's not really a "NULL" entry. It's an "empty object" entry,
not a "NULL" pointer ... how about replacing "a NULL" with "an empty" ?
> + }
> + }
> }
>
> /* USB is special because the bcdDevice can be matched against a numeric
> range */
> @@ -168,7 +183,7 @@ static void do_usb_table(void *symval, u
> unsigned int i;
> const unsigned long id_size = sizeof(struct usb_device_id);
>
> - device_id_size_check(mod->name, "usb", size, id_size);
> + device_id_check(mod->name, "usb", size, id_size, symval);
>
> /* Leave last one: it's the terminator. */
> size -= id_size;
> @@ -505,7 +520,7 @@ static void do_table(void *symval, unsig
> char alias[500];
> int (*do_entry)(const char *, void *entry, char *alias) = function;
>
> - device_id_size_check(mod->name, device_id, size, id_size);
> + device_id_check(mod->name, device_id, size, id_size, symval);
> /* Leave last one: it's the terminator. */
> size -= id_size;
>
> @@ -527,14 +542,22 @@ void handle_moddevtable(struct module *m
> Elf_Sym *sym, const char *symname)
> {
> void *symval;
> + char *zeros = NULL;
>
> /* We're looking for a section relative symbol */
> if (!sym->st_shndx || sym->st_shndx >= info->hdr->e_shnum)
> return;
>
> - symval = (void *)info->hdr
> - + info->sechdrs[sym->st_shndx].sh_offset
> - + sym->st_value;
> + /* Handle all-NULL symbols allocated into .bss */
> + if
Re: [PATCH] modpost: detect unterminated device id lists
On 9/17/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
> On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook <[EMAIL PROTECTED]> wrote:
>
> > This patch against 2.6.23-rc6 will cause modpost to fail if any device
> > id lists are incorrectly terminated, after reporting the offender.
>
> I'm getting this:
>
> rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
> 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> 0x00 0x00 0x00 0x00 0x00
> FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not
> terminated
> with a NULL entry!
>
> ("rusb2/pvrusb2" ??)
Hmm? Are you sure you didn't see any "drivers/media/video/pv" before the
"rusb2/pvrusb2" bit? Looking at Kees' patch (and the existing code), I've no
clue how/why this should happen ... will try to reproduce here ...
> but:
>
> struct usb_device_id pvr2_device_table[] = {
> [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
> [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> { USB_DEVICE(0, 0) },
> };
>
> looks OK?
>
> Using plain old "{ }" shut the warning up.
USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
assigns "0x03 0x00" (in little endian) to usb_device_id.match_flags. And
I don't think the USB code treats such an entry as an empty entry (?)
Interestingly, the "USB_DEVICE(0, 0)" thing is absent from latest -git
tree and also in my copy of 23-rc4-mm1 -- so this looks like something
you must've merged recently.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook <[EMAIL PROTECTED]> wrote:
> This patch against 2.6.23-rc6 will cause modpost to fail if any device
> id lists are incorrectly terminated, after reporting the offender.
I'm getting this:
rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x00 0x00 0x00 0x00
FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not
terminated with a NULL entry!
("rusb2/pvrusb2" ??)
but:
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
{ USB_DEVICE(0, 0) },
};
looks OK?
Using plain old "{ }" shut the warning up.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook [EMAIL PROTECTED] wrote:
This patch against 2.6.23-rc6 will cause modpost to fail if any device
id lists are incorrectly terminated, after reporting the offender.
I'm getting this:
rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x00 0x00 0x00 0x00
FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not
terminated with a NULL entry!
(rusb2/pvrusb2 ??)
but:
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
{ USB_DEVICE(0, 0) },
};
looks OK?
Using plain old { } shut the warning up.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On 9/17/07, Andrew Morton [EMAIL PROTECTED] wrote:
On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook [EMAIL PROTECTED] wrote:
This patch against 2.6.23-rc6 will cause modpost to fail if any device
id lists are incorrectly terminated, after reporting the offender.
I'm getting this:
rusb2/pvrusb2: struct usb_device_id is 20 bytes. The last of 3 is:
0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x00 0x00 0x00 0x00 0x00
FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not
terminated
with a NULL entry!
(rusb2/pvrusb2 ??)
Hmm? Are you sure you didn't see any drivers/media/video/pv before the
rusb2/pvrusb2 bit? Looking at Kees' patch (and the existing code), I've no
clue how/why this should happen ... will try to reproduce here ...
but:
struct usb_device_id pvr2_device_table[] = {
[PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
[PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
{ USB_DEVICE(0, 0) },
};
looks OK?
Using plain old { } shut the warning up.
USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
assigns 0x03 0x00 (in little endian) to usb_device_id.match_flags. And
I don't think the USB code treats such an entry as an empty entry (?)
Interestingly, the USB_DEVICE(0, 0) thing is absent from latest -git
tree and also in my copy of 23-rc4-mm1 -- so this looks like something
you must've merged recently.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
Hi Kees,
On 9/13/07, Kees Cook [EMAIL PROTECTED] wrote:
This patch against 2.6.23-rc6 will cause modpost to fail if any device
id lists are incorrectly terminated, after reporting the offender.
Signed-off-by: Kees Cook [EMAIL PROTECTED]
Nice! :-)
BTW a very similar idea (but for a different problem) was discussed in:
http://lkml.org/lkml/2007/8/23/48
I tried doing something about that, but gave up in between. For the
device_id tables, a lot of infrastructure/code already exists in modpost,
but no such luck for kobjects :-( Still, if you can do something about
that, as he mentioned, I bet Greg would gladly accept such a patch :-)
--- linux-2.6.23-rc6~/scripts/mod/file2alias.c 2007-09-11 23:17:49.0
-0700
+++ linux-2.6.23-rc6/scripts/mod/file2alias.c 2007-09-12 17:41:30.0
-0700
@@ -55,10 +55,13 @@ do {
* Check that sizeof(device_id type) are consistent with size of section
* in .o file. If in-consistent then userspace and kernel does not agree
* on actual size which is a bug.
+ * Also verify that the final entry in the table is all zeros.
**/
-static void device_id_size_check(const char *modname, const char *device_id,
-unsigned long size, unsigned long id_size)
+static void device_id_check(const char *modname, const char *device_id,
+ unsigned long size, unsigned long id_size,
+ void *symval)
If you pass the Elf_Sym *sym all the way from handle_moddevtable() (which
means you can get rid of the sym-st_size argument in the call chain), then
it would be possible to print out the *symbol name* too here ...
{
+ int i;
uint8_t *p;
if (size % id_size || size id_size) {
fatal(%s: sizeof(struct %s_device_id)=%lu is not a modulo
of the size of section __mod_%s_device_table=%lu.\n
@@ -66,6 +69,18 @@ static void device_id_size_check(const c
in mod_devicetable.h\n,
modname, device_id, id_size, device_id, size,
device_id);
}
+ /* Verify last one is a terminator */
+ for (i = 0; i id_size; i++ ) {
+ if ( *(uint8_t*)(symval+size-id_size+i) ) {
... and:
for (p = symval+size-id_size; p symval+size; p++) {
if (*p) {
is probably clearer ?
+ fprintf(stderr,%s: struct %s_device_id is %lu bytes.
The last of
%lu is:\n, modname, device_id, id_size, size / id_size);
As I just said, printing out just the modname and device_id type sounds
insufficient here. Note that they were sufficient before your patch, because
previously, this function only checked if the device_id *type* itself was
incorrectly defined. But here we're talking about a specific errant *symbol*.
+ for (i = 0; i id_size; i++ ) {
+ fprintf(stderr,0x%02x ,
*(uint8_t*)(symval+size-id_size+i) );
+ }
Again, for (p = symval+size-id_size; p symval+size; p++) {
and then fprintf(..., *p); would be cleaner.
+ fprintf(stderr,\n);
+ fatal(%s: struct %s_device_id is not terminated
+ with a NULL entry!\n, modname, device_id);
Subtle nit, but it's not really a NULL entry. It's an empty object entry,
not a NULL pointer ... how about replacing a NULL with an empty ?
+ }
+ }
}
/* USB is special because the bcdDevice can be matched against a numeric
range */
@@ -168,7 +183,7 @@ static void do_usb_table(void *symval, u
unsigned int i;
const unsigned long id_size = sizeof(struct usb_device_id);
- device_id_size_check(mod-name, usb, size, id_size);
+ device_id_check(mod-name, usb, size, id_size, symval);
/* Leave last one: it's the terminator. */
size -= id_size;
@@ -505,7 +520,7 @@ static void do_table(void *symval, unsig
char alias[500];
int (*do_entry)(const char *, void *entry, char *alias) = function;
- device_id_size_check(mod-name, device_id, size, id_size);
+ device_id_check(mod-name, device_id, size, id_size, symval);
/* Leave last one: it's the terminator. */
size -= id_size;
@@ -527,14 +542,22 @@ void handle_moddevtable(struct module *m
Elf_Sym *sym, const char *symname)
{
void *symval;
+ char *zeros = NULL;
/* We're looking for a section relative symbol */
if (!sym-st_shndx || sym-st_shndx = info-hdr-e_shnum)
return;
- symval = (void *)info-hdr
- + info-sechdrs[sym-st_shndx].sh_offset
- + sym-st_value;
+ /* Handle all-NULL symbols allocated into .bss */
+ if (info-sechdrs[sym-st_shndx].sh_type SHT_NOBITS) {
+ zeros = calloc(1, sym-st_size);
+ symval = zeros;
+ }
Hmm,
Re: [PATCH] modpost: detect unterminated device id lists
Hi Satyam,
On Mon, Sep 17, 2007 at 06:52:52AM +0530, Satyam Sharma wrote:
On 9/13/07, Kees Cook [EMAIL PROTECTED] wrote:
This patch against 2.6.23-rc6 will cause modpost to fail if any device
id lists are incorrectly terminated, after reporting the offender.
Signed-off-by: Kees Cook [EMAIL PROTECTED]
Nice! :-)
BTW a very similar idea (but for a different problem) was discussed in:
http://lkml.org/lkml/2007/8/23/48
I tried doing something about that, but gave up in between. For the
device_id tables, a lot of infrastructure/code already exists in modpost,
but no such luck for kobjects :-( Still, if you can do something about
that, as he mentioned, I bet Greg would gladly accept such a patch :-)
Thanks! Hmm, perhaps I'll take that on when I learn kobjects better. :)
+static void device_id_check(const char *modname, const char *device_id,
+ unsigned long size, unsigned long id_size,
+ void *symval)
If you pass the Elf_Sym *sym all the way from handle_moddevtable() (which
means you can get rid of the sym-st_size argument in the call chain), then
it would be possible to print out the *symbol name* too here ...
That's true. I actually threw away an earlier version of this patch
that made some extensive changes to the elf parser (due to the NOBITS
thing I explain below), but instead opted for the smaller version that
stayed out of there.
for (p = symval+size-id_size; p symval+size; p++) {
if (*p) {
is probably clearer ?
Ah, yeah, that's much nicer. I think I must have still have my ELF
parser hat on when I wrote that. ;)
As I just said, printing out just the modname and device_id type sounds
insufficient here. Note that they were sufficient before your patch, because
previously, this function only checked if the device_id *type* itself was
incorrectly defined. But here we're talking about a specific errant *symbol*.
That's true, yeah.
+ fprintf(stderr,\n);
+ fatal(%s: struct %s_device_id is not terminated
+ with a NULL entry!\n, modname, device_id);
Subtle nit, but it's not really a NULL entry. It's an empty object entry,
not a NULL pointer ... how about replacing a NULL with an empty ?
Yeah, that bugged me when I put that in too. :) Yes, an empty is
better.
@@ -527,14 +542,22 @@ void handle_moddevtable(struct module *m
Elf_Sym *sym, const char *symname)
{
void *symval;
+ char *zeros = NULL;
/* We're looking for a section relative symbol */
if (!sym-st_shndx || sym-st_shndx = info-hdr-e_shnum)
return;
- symval = (void *)info-hdr
- + info-sechdrs[sym-st_shndx].sh_offset
- + sym-st_value;
+ /* Handle all-NULL symbols allocated into .bss */
+ if (info-sechdrs[sym-st_shndx].sh_type SHT_NOBITS) {
+ zeros = calloc(1, sym-st_size);
+ symval = zeros;
+ }
Hmm, I don't quite grok this case. Care to explain?
In the ELF format, the .bss segment is initialized by the loader to all
zeros, but it contains no in-file representation (since it's all zeros
and would be a waste of space). Such segments are flags as SHT_NOBITS
meaning that the loader must allocate cleared memory instead of loading
the segment from the file.
In the case of modules that have a totally empty *_device_id list (?!),
the compiler optimizes this into the .bss segment, since there is no need
to store an all-zero-contents object in a segment that would be loaded
from the file itself.
As a result, attempting to dereference such a symbol without noticing
the SHT_NOBITS flag lands you somewhere in uninitialized memory. So,
the above code basically side-steps the incorrect symbol location
calculation and just aims it at a cleared part of memory.
As I mentioned, there was a larger patch that attempted to sort this
out in the elf parser, but I didn't like it; it was big, not 100%
correct, and the above approach seemed like a much less invasive change.
The cleanups you suggested, who should I send those to? Or will you (or
Sam?) make them directly to the kbuild.git tree? (I've never poked at
this part of the kernel source before... I'm unclear on the processes
surrounding it maintainership.)
Thanks!
-Kees
--
Kees Cook
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] modpost: detect unterminated device id lists
On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook <[EMAIL PROTECTED]> wrote: > On Wed, Sep 12, 2007 at 02:53:56PM -0700, Greg KH wrote: > > On Wed, Sep 12, 2007 at 03:48:49PM +0400, Alexey Dobriyan wrote: > > > On 9/12/07, Jeff Garzik <[EMAIL PROTECTED]> wrote: > > > > Kees Cook wrote: > > > > > This patch against 2.6.23-rc6 fixes a couple drivers that do not > > > > > correctly terminate their pci_device_id lists. This results in > > > > > garbage > > > > > being spewed into modules.pcimap when the module happens to not have > > > > > 28 NULL bytes following the table, and/or the last PCI ID is actually > > > > > truncated from the table when calculating the modules.alias PCI > > > > > aliases, > > > > > cause those unfortunate device IDs to not auto-load. > > > > > > > > > > Signed-off-by: Kees Cook <[EMAIL PROTECTED]> > > > > > > > > ACK > > > > > > I mut say, non-terminated PCI ids lists are constant PITA. There should be > > > a way to a) put it in macro[1], so that terminator automatically added, > > > and > > > b) still allow #ifdef inside table like, e.g. 8139too does. > > > > > > [1] or not macro, because #ifdef inside macros aren't allowed. > > > > If you know of a way to do this in an easier manner, patches are always > > gladly accepted :) > > This patch against 2.6.23-rc6 will cause modpost to fail if any device > id lists are incorrectly terminated, after reporting the offender. ooh, clever chap. > + fprintf(stderr,"%s: struct %s_device_id is %lu bytes. > The last of %lu is:\n", modname, device_id, id_size, size / id_size); dude, bid on this: http://cgi.ebay.com/Wyse-WY55-General-Purpose-Serial-Terminal-No-Keyboard_W0QQitemZ230169388145QQihZ013QQcategoryZ51280QQssPageNameZWDVWQQrdZ1QQcmdZViewItem (not allowed to use 132-column mode, either) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] modpost: detect unterminated device id lists
On Wed, Sep 12, 2007 at 02:53:56PM -0700, Greg KH wrote:
> On Wed, Sep 12, 2007 at 03:48:49PM +0400, Alexey Dobriyan wrote:
> > On 9/12/07, Jeff Garzik <[EMAIL PROTECTED]> wrote:
> > > Kees Cook wrote:
> > > > This patch against 2.6.23-rc6 fixes a couple drivers that do not
> > > > correctly terminate their pci_device_id lists. This results in garbage
> > > > being spewed into modules.pcimap when the module happens to not have
> > > > 28 NULL bytes following the table, and/or the last PCI ID is actually
> > > > truncated from the table when calculating the modules.alias PCI aliases,
> > > > cause those unfortunate device IDs to not auto-load.
> > > >
> > > > Signed-off-by: Kees Cook <[EMAIL PROTECTED]>
> > >
> > > ACK
> >
> > I mut say, non-terminated PCI ids lists are constant PITA. There should be
> > a way to a) put it in macro[1], so that terminator automatically added, and
> > b) still allow #ifdef inside table like, e.g. 8139too does.
> >
> > [1] or not macro, because #ifdef inside macros aren't allowed.
>
> If you know of a way to do this in an easier manner, patches are always
> gladly accepted :)
This patch against 2.6.23-rc6 will cause modpost to fail if any device
id lists are incorrectly terminated, after reporting the offender.
Signed-off-by: Kees Cook <[EMAIL PROTECTED]>
---
linux-2.6.23-rc6/scripts/mod/file2alias.c | 39 --
1 file changed, 32 insertions(+), 7 deletions(-)
---
diff -urp -x '*.o' linux-2.6.23-rc6~/scripts/mod/file2alias.c
linux-2.6.23-rc6/scripts/mod/file2alias.c
--- linux-2.6.23-rc6~/scripts/mod/file2alias.c 2007-09-11 23:17:49.0
-0700
+++ linux-2.6.23-rc6/scripts/mod/file2alias.c 2007-09-12 17:41:30.0
-0700
@@ -55,10 +55,13 @@ do {
* Check that sizeof(device_id type) are consistent with size of section
* in .o file. If in-consistent then userspace and kernel does not agree
* on actual size which is a bug.
+ * Also verify that the final entry in the table is all zeros.
**/
-static void device_id_size_check(const char *modname, const char *device_id,
-unsigned long size, unsigned long id_size)
+static void device_id_check(const char *modname, const char *device_id,
+ unsigned long size, unsigned long id_size,
+ void *symval)
{
+ int i;
if (size % id_size || size < id_size) {
fatal("%s: sizeof(struct %s_device_id)=%lu is not a modulo "
"of the size of section __mod_%s_device_table=%lu.\n"
@@ -66,6 +69,18 @@ static void device_id_size_check(const c
"in mod_devicetable.h\n",
modname, device_id, id_size, device_id, size, device_id);
}
+ /* Verify last one is a terminator */
+ for (i = 0; i < id_size; i++ ) {
+ if ( *(uint8_t*)(symval+size-id_size+i) ) {
+ fprintf(stderr,"%s: struct %s_device_id is %lu bytes.
The last of %lu is:\n", modname, device_id, id_size, size / id_size);
+ for (i = 0; i < id_size; i++ ) {
+ fprintf(stderr,"0x%02x ",
*(uint8_t*)(symval+size-id_size+i) );
+ }
+ fprintf(stderr,"\n");
+ fatal("%s: struct %s_device_id is not terminated "
+ "with a NULL entry!\n", modname, device_id);
+ }
+ }
}
/* USB is special because the bcdDevice can be matched against a numeric range
*/
@@ -168,7 +183,7 @@ static void do_usb_table(void *symval, u
unsigned int i;
const unsigned long id_size = sizeof(struct usb_device_id);
- device_id_size_check(mod->name, "usb", size, id_size);
+ device_id_check(mod->name, "usb", size, id_size, symval);
/* Leave last one: it's the terminator. */
size -= id_size;
@@ -505,7 +520,7 @@ static void do_table(void *symval, unsig
char alias[500];
int (*do_entry)(const char *, void *entry, char *alias) = function;
- device_id_size_check(mod->name, device_id, size, id_size);
+ device_id_check(mod->name, device_id, size, id_size, symval);
/* Leave last one: it's the terminator. */
size -= id_size;
@@ -527,14 +542,22 @@ void handle_moddevtable(struct module *m
Elf_Sym *sym, const char *symname)
{
void *symval;
+ char *zeros = NULL;
/* We're looking for a section relative symbol */
if (!sym->st_shndx || sym->st_shndx >= info->hdr->e_shnum)
return;
- symval = (void *)info->hdr
- + info->sechdrs[sym->st_shndx].sh_offset
- + sym->st_value;
+ /* Handle all-NULL symbols allocated into .bss */
+ if (info->sechdrs[sym->st_shndx].sh_type & SHT_NOBITS) {
+ zeros = calloc(1, sym->st_size);
+ symval = zeros;
+ }
+
[PATCH] modpost: detect unterminated device id lists
On Wed, Sep 12, 2007 at 02:53:56PM -0700, Greg KH wrote:
On Wed, Sep 12, 2007 at 03:48:49PM +0400, Alexey Dobriyan wrote:
On 9/12/07, Jeff Garzik [EMAIL PROTECTED] wrote:
Kees Cook wrote:
This patch against 2.6.23-rc6 fixes a couple drivers that do not
correctly terminate their pci_device_id lists. This results in garbage
being spewed into modules.pcimap when the module happens to not have
28 NULL bytes following the table, and/or the last PCI ID is actually
truncated from the table when calculating the modules.alias PCI aliases,
cause those unfortunate device IDs to not auto-load.
Signed-off-by: Kees Cook [EMAIL PROTECTED]
ACK
I mut say, non-terminated PCI ids lists are constant PITA. There should be
a way to a) put it in macro[1], so that terminator automatically added, and
b) still allow #ifdef inside table like, e.g. 8139too does.
[1] or not macro, because #ifdef inside macros aren't allowed.
If you know of a way to do this in an easier manner, patches are always
gladly accepted :)
This patch against 2.6.23-rc6 will cause modpost to fail if any device
id lists are incorrectly terminated, after reporting the offender.
Signed-off-by: Kees Cook [EMAIL PROTECTED]
---
linux-2.6.23-rc6/scripts/mod/file2alias.c | 39 --
1 file changed, 32 insertions(+), 7 deletions(-)
---
diff -urp -x '*.o' linux-2.6.23-rc6~/scripts/mod/file2alias.c
linux-2.6.23-rc6/scripts/mod/file2alias.c
--- linux-2.6.23-rc6~/scripts/mod/file2alias.c 2007-09-11 23:17:49.0
-0700
+++ linux-2.6.23-rc6/scripts/mod/file2alias.c 2007-09-12 17:41:30.0
-0700
@@ -55,10 +55,13 @@ do {
* Check that sizeof(device_id type) are consistent with size of section
* in .o file. If in-consistent then userspace and kernel does not agree
* on actual size which is a bug.
+ * Also verify that the final entry in the table is all zeros.
**/
-static void device_id_size_check(const char *modname, const char *device_id,
-unsigned long size, unsigned long id_size)
+static void device_id_check(const char *modname, const char *device_id,
+ unsigned long size, unsigned long id_size,
+ void *symval)
{
+ int i;
if (size % id_size || size id_size) {
fatal(%s: sizeof(struct %s_device_id)=%lu is not a modulo
of the size of section __mod_%s_device_table=%lu.\n
@@ -66,6 +69,18 @@ static void device_id_size_check(const c
in mod_devicetable.h\n,
modname, device_id, id_size, device_id, size, device_id);
}
+ /* Verify last one is a terminator */
+ for (i = 0; i id_size; i++ ) {
+ if ( *(uint8_t*)(symval+size-id_size+i) ) {
+ fprintf(stderr,%s: struct %s_device_id is %lu bytes.
The last of %lu is:\n, modname, device_id, id_size, size / id_size);
+ for (i = 0; i id_size; i++ ) {
+ fprintf(stderr,0x%02x ,
*(uint8_t*)(symval+size-id_size+i) );
+ }
+ fprintf(stderr,\n);
+ fatal(%s: struct %s_device_id is not terminated
+ with a NULL entry!\n, modname, device_id);
+ }
+ }
}
/* USB is special because the bcdDevice can be matched against a numeric range
*/
@@ -168,7 +183,7 @@ static void do_usb_table(void *symval, u
unsigned int i;
const unsigned long id_size = sizeof(struct usb_device_id);
- device_id_size_check(mod-name, usb, size, id_size);
+ device_id_check(mod-name, usb, size, id_size, symval);
/* Leave last one: it's the terminator. */
size -= id_size;
@@ -505,7 +520,7 @@ static void do_table(void *symval, unsig
char alias[500];
int (*do_entry)(const char *, void *entry, char *alias) = function;
- device_id_size_check(mod-name, device_id, size, id_size);
+ device_id_check(mod-name, device_id, size, id_size, symval);
/* Leave last one: it's the terminator. */
size -= id_size;
@@ -527,14 +542,22 @@ void handle_moddevtable(struct module *m
Elf_Sym *sym, const char *symname)
{
void *symval;
+ char *zeros = NULL;
/* We're looking for a section relative symbol */
if (!sym-st_shndx || sym-st_shndx = info-hdr-e_shnum)
return;
- symval = (void *)info-hdr
- + info-sechdrs[sym-st_shndx].sh_offset
- + sym-st_value;
+ /* Handle all-NULL symbols allocated into .bss */
+ if (info-sechdrs[sym-st_shndx].sh_type SHT_NOBITS) {
+ zeros = calloc(1, sym-st_size);
+ symval = zeros;
+ }
+ else {
+ symval = (void *)info-hdr
+ +
Re: [PATCH] modpost: detect unterminated device id lists
On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook [EMAIL PROTECTED] wrote: On Wed, Sep 12, 2007 at 02:53:56PM -0700, Greg KH wrote: On Wed, Sep 12, 2007 at 03:48:49PM +0400, Alexey Dobriyan wrote: On 9/12/07, Jeff Garzik [EMAIL PROTECTED] wrote: Kees Cook wrote: This patch against 2.6.23-rc6 fixes a couple drivers that do not correctly terminate their pci_device_id lists. This results in garbage being spewed into modules.pcimap when the module happens to not have 28 NULL bytes following the table, and/or the last PCI ID is actually truncated from the table when calculating the modules.alias PCI aliases, cause those unfortunate device IDs to not auto-load. Signed-off-by: Kees Cook [EMAIL PROTECTED] ACK I mut say, non-terminated PCI ids lists are constant PITA. There should be a way to a) put it in macro[1], so that terminator automatically added, and b) still allow #ifdef inside table like, e.g. 8139too does. [1] or not macro, because #ifdef inside macros aren't allowed. If you know of a way to do this in an easier manner, patches are always gladly accepted :) This patch against 2.6.23-rc6 will cause modpost to fail if any device id lists are incorrectly terminated, after reporting the offender. ooh, clever chap. + fprintf(stderr,%s: struct %s_device_id is %lu bytes. The last of %lu is:\n, modname, device_id, id_size, size / id_size); dude, bid on this: http://cgi.ebay.com/Wyse-WY55-General-Purpose-Serial-Terminal-No-Keyboard_W0QQitemZ230169388145QQihZ013QQcategoryZ51280QQssPageNameZWDVWQQrdZ1QQcmdZViewItem (not allowed to use 132-column mode, either) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
