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]>
---
For example, cafe_nand...
drivers/mtd/nand/cafe_nand.c:
static struct pci_device_id cafe_nand_tbl[] = {
{ 0x11ab, 0x4100, PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_MEMORY_FLASH << 8, 0xFFFF0 }
};
# should have 1 PCI alias, but we have none
$ modinfo cafe_nand | grep alias | wc -l
0
# should have 1 PCI map, but we have lots of trash
$ grep cafe_nand /lib/modules/$(uname -r)/modules.pcimap
modules.pcimap:cafe_nand 0x000011ab 0x00004100 0xffffffff 0xffffffff 0x00050100 0x000ffff0 0x0
modules.pcimap:cafe_nand 0x696d6974 0x0000676e 0x00000000 0x00000000 0x00000000 0x00000000 0x0
modules.pcimap:cafe_nand 0x00000003 0x00000000 0x00000000 0x00000000 0x00000000 0x00000000 0x0
modules.pcimap:cafe_nand 0x00000004 0x00000000 0x00000000 0x00000000 0x63656863 0x6363656b 0x0
modules.pcimap:cafe_nand 0x65640067 0x00677562 0x70696b73 0x00746262 0x64657375 0x4200616d 0x0
modules.pcimap:cafe_nand 0x000000bc 0x00000000 0x00000000 0x00000000 0x00000000 0x00000000 0x0
It may make sense to patch module-init-tools to correctly yell about
mismatches, as it has all the details when examining the ELF.
Note also, then p54 driver (in p54/prism54pci.c, not in mainline yet) suffers
from the same problem.
linux-2.6.23-rc6/drivers/char/ipmi/ipmi_si_intf.c | 3 ++-
linux-2.6.23-rc6/drivers/mtd/nand/cafe_nand.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
---
diff -uNrp linux-2.6.23-rc6~/drivers/char/ipmi/ipmi_si_intf.c linux-2.6.23-rc6/drivers/char/ipmi/ipmi_si_intf.c
--- linux-2.6.23-rc6~/drivers/char/ipmi/ipmi_si_intf.c 2007-09-11 23:17:13.000000000 -0700
+++ linux-2.6.23-rc6/drivers/char/ipmi/ipmi_si_intf.c 2007-09-11 23:21:51.000000000 -0700
@@ -2215,7 +2215,8 @@ static int ipmi_pci_resume(struct pci_de
static struct pci_device_id ipmi_pci_devices[] = {
{ PCI_DEVICE(PCI_HP_VENDOR_ID, PCI_MMC_DEVICE_ID) },
- { PCI_DEVICE_CLASS(PCI_ERMC_CLASSCODE, PCI_ERMC_CLASSCODE_MASK) }
+ { PCI_DEVICE_CLASS(PCI_ERMC_CLASSCODE, PCI_ERMC_CLASSCODE_MASK) },
+ { 0, }
};
MODULE_DEVICE_TABLE(pci, ipmi_pci_devices);
diff -uNrp linux-2.6.23-rc6~/drivers/mtd/nand/cafe_nand.c linux-2.6.23-rc6/drivers/mtd/nand/cafe_nand.c
--- linux-2.6.23-rc6~/drivers/mtd/nand/cafe_nand.c 2007-07-08 16:32:17.000000000 -0700
+++ linux-2.6.23-rc6/drivers/mtd/nand/cafe_nand.c 2007-09-11 23:22:11.000000000 -0700
@@ -816,7 +816,8 @@ static void __devexit cafe_nand_remove(s
}
static struct pci_device_id cafe_nand_tbl[] = {
- { 0x11ab, 0x4100, PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_MEMORY_FLASH << 8, 0xFFFF0 }
+ { 0x11ab, 0x4100, PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_MEMORY_FLASH << 8, 0xFFFF0 },
+ { 0, }
};
MODULE_DEVICE_TABLE(pci, cafe_nand_tbl);
--
Kees Cook
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
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.
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 :)
thanks,
greg k-h
On Wed, 12 Sep 2007 14:53:56 -0700
Greg KH <[email protected]> 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 :)
Change (ie: fix) the APIs to take a `length' arg, then fix up 10^42 drivers.
Oh, you said "easy" ;)
Perhaps there's some clever way in which we can check that the tables are
correctly terminated. I guess some static code-checker could do it. A
weaker option would be to do some runtime hack which carefully walks the
table and checks stuff.
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.000000000 -0700
+++ linux-2.6.23-rc6/scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -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
+ + info->sechdrs[sym->st_shndx].sh_offset
+ + sym->st_value;
+ }
if (sym_is(symname, "__mod_pci_device_table"))
do_table(symval, sym->st_size,
@@ -599,6 +622,8 @@ void handle_moddevtable(struct module *m
do_table(symval, sym->st_size,
sizeof(struct parisc_device_id), "parisc",
do_parisc_entry, mod);
+
+ if (zeros) free(zeros);
}
/* Now add out buffered information to the generated C source */
--
Kees Cook
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)
[non-terminated PCI ids arrays]
Here is compile-time hack (yep, warped and whitespace damaged :))
It's better than modpost-time hack. because it triggers earlier.
It's worse than modpost-time hack, because of tree-wide changes.
diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
index f4e4298..b895b5f 100644
--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -237,7 +237,7 @@ static const struct {
};
-static struct pci_device_id rtl8139_pci_tbl[] = {
+PCI_MODULE_DEVICE_TABLE_BEGIN(rtl8139_pci_tbl)
{0x10ec, 0x8139, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
{0x10ec, 0x8138, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
{0x1113, 0x1211, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
@@ -273,10 +273,7 @@ static struct pci_device_id rtl8139_pci_tbl[] = {
{PCI_ANY_ID, 0x8139, 0x10ec, 0x8139, 0, 0, RTL8139 },
{PCI_ANY_ID, 0x8139, 0x1186, 0x1300, 0, 0, RTL8139 },
{PCI_ANY_ID, 0x8139, 0x13d1, 0xab06, 0, 0, RTL8139 },
-
- {0,}
-};
-MODULE_DEVICE_TABLE (pci, rtl8139_pci_tbl);
+PCI_MODULE_DEVICE_TABLE_END
static struct {
const char str[ETH_GSTRING_LEN];
diff --git a/include/linux/module.h b/include/linux/module.h
index b6a646c..aef3cd9 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -137,6 +137,13 @@ extern struct module __this_module;
#define MODULE_DEVICE_TABLE(type,name) \
MODULE_GENERIC_TABLE(type##_device,name)
+#define PCI_MODULE_DEVICE_TABLE_BEGIN(name) \
+ MODULE_DEVICE_TABLE(pci, name); \
+ static struct pci_device_id name[] = {
+
+#define PCI_MODULE_DEVICE_TABLE_END \
+ {}};
+
/* Version of form [<epoch>:]<version>[-<extra-version>].
Or for CVS/RCS ID version, everything but the number is stripped.
<epoch>: A (small) unsigned integer which allows you to start versions
Alexey Dobriyan wrote:
> -static struct pci_device_id rtl8139_pci_tbl[] = {
> +PCI_MODULE_DEVICE_TABLE_BEGIN(rtl8139_pci_tbl)
> {0x10ec, 0x8139, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
> {0x10ec, 0x8138, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
> {0x1113, 0x1211, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
> @@ -273,10 +273,7 @@ static struct pci_device_id rtl8139_pci_tbl[] = {
> {PCI_ANY_ID, 0x8139, 0x10ec, 0x8139, 0, 0, RTL8139 },
> {PCI_ANY_ID, 0x8139, 0x1186, 0x1300, 0, 0, RTL8139 },
> {PCI_ANY_ID, 0x8139, 0x13d1, 0xab06, 0, 0, RTL8139 },
> -
> - {0,}
> -};
> -MODULE_DEVICE_TABLE (pci, rtl8139_pci_tbl);
> +PCI_MODULE_DEVICE_TABLE_END
I think the previous version looks better.
Jeff
On Thu, Sep 13, 2007 at 10:34:02AM +0400, Alexey Dobriyan wrote:
> [non-terminated PCI ids arrays]
>
> Here is compile-time hack (yep, warped and whitespace damaged :))
> It's better than modpost-time hack. because it triggers earlier.
> It's worse than modpost-time hack, because of tree-wide changes.
>
> diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
> index f4e4298..b895b5f 100644
> --- a/drivers/net/8139too.c
> +++ b/drivers/net/8139too.c
> @@ -237,7 +237,7 @@ static const struct {
> };
>
>
> -static struct pci_device_id rtl8139_pci_tbl[] = {
> +PCI_MODULE_DEVICE_TABLE_BEGIN(rtl8139_pci_tbl)
> {0x10ec, 0x8139, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
> {0x10ec, 0x8138, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
> {0x1113, 0x1211, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
> @@ -273,10 +273,7 @@ static struct pci_device_id rtl8139_pci_tbl[] = {
> {PCI_ANY_ID, 0x8139, 0x10ec, 0x8139, 0, 0, RTL8139 },
> {PCI_ANY_ID, 0x8139, 0x1186, 0x1300, 0, 0, RTL8139 },
> {PCI_ANY_ID, 0x8139, 0x13d1, 0xab06, 0, 0, RTL8139 },
> -
> - {0,}
> -};
> -MODULE_DEVICE_TABLE (pci, rtl8139_pci_tbl);
> +PCI_MODULE_DEVICE_TABLE_END
Looks at bit too magic to my taste.
And anyway deferring the check until modpost time should not cause troubles
anyway assuming people do build their kernel after adding a device ID.
And the above requires one to use the special macro in all places
whereas the modpost check will catch it also for new device ID users.
Sam
On Sep 13 2007 02:42, Jeff Garzik wrote:
> Alexey Dobriyan wrote:
>> -static struct pci_device_id rtl8139_pci_tbl[] = {
>> +PCI_MODULE_DEVICE_TABLE_BEGIN(rtl8139_pci_tbl)
>> {0x10ec, 0x8139, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
>> {0x10ec, 0x8138, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
>> {0x1113, 0x1211, PCI_ANY_ID, PCI_ANY_ID, 0, 0, RTL8139 },
>> @@ -273,10 +273,7 @@ static struct pci_device_id rtl8139_pci_tbl[] = {
>> {PCI_ANY_ID, 0x8139, 0x10ec, 0x8139, 0, 0, RTL8139 },
>> {PCI_ANY_ID, 0x8139, 0x1186, 0x1300, 0, 0, RTL8139 },
>> {PCI_ANY_ID, 0x8139, 0x13d1, 0xab06, 0, 0, RTL8139 },
>> -
>> - {0,}
>> -};
>> -MODULE_DEVICE_TABLE (pci, rtl8139_pci_tbl);
>> +PCI_MODULE_DEVICE_TABLE_END
That looks sooo... wxwidgets-like :)
> I think the previous version looks better.
Nod.
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.
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.
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.000000000 -0700
> +++ linux-2.6.23-rc6/scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -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, I don't quite grok this case. Care to explain?
> + else {
> + symval = (void *)info->hdr
> + + info->sechdrs[sym->st_shndx].sh_offset
> + + sym->st_value;
> + }
>
> if (sym_is(symname, "__mod_pci_device_table"))
> do_table(symval, sym->st_size,
> @@ -599,6 +622,8 @@ void handle_moddevtable(struct module *m
> do_table(symval, sym->st_size,
> sizeof(struct parisc_device_id), "parisc",
> do_parisc_entry, mod);
> +
> + if (zeros) free(zeros);
> }
>
> /* Now add out buffered information to the generated C source */
Satyam
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
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);
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..
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
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?
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.