Skip to contentSkip to conversation

pci: fix unterminated pci_device_id lists

PATCH In review Kees Cook 10 participants 20 messages Sep 12, 2007 – Sep 17, 2007

Subsystems (7)
Network drivers Media (V4L/DVB) Kbuild Memory technology devices (MTD) +3 more subsystems
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 pci: fix unterminated pci_device_id lists

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(-)

---

drivers/char/ipmi/ipmi_si_intf.c +2 -1
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
@@ -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);
drivers/mtd/nand/cafe_nand.c +2 -1
diff -uNrp linux-2.6.23-rc6~/drivers/mtd/nand/cafe_nand.c linux-2.6.23-rc6/drivers/mtd/nand/cafe_nand.c
@@ -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);
Signature · 1 line
Kees Cook
#1 lore raw
Quoted 8 lines · Kees Cook

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

#2 lore raw
Quoted 11 lines · Jeff Garzik
Quoted 8 lines · Kees Cook

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.

#3 lore raw
Quoted 11 lines · Alexey Dobriyan
Quoted 10 lines · Jeff Garzik
Quoted 8 lines · Kees Cook

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]>
Quoted 7 lines
Quoted 1 line

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

#4 lore raw
Quoted 14 lines · On Wed, 12 Sep 2007 14:53:56 -0700 Greg KH <[email protected]> wrote:
Quoted 13 lines · Alexey Dobriyan
Quoted 11 lines · Jeff Garzik
Quoted 8 lines · Kees Cook

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

Quoted 8 lines
Quoted 5 lines

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.

#5 lore raw

[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.

drivers/net/8139too.c +2 -5
index f4e4298..b895b5f 100644
@@ -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];
include/linux/module.h +7 -0
index b6a646c..aef3cd9 100644
@@ -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
#6 lore raw
Quoted 6 lines · Alexey Dobriyan
drivers/net/8139too.c +1 -1
-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 },

drivers/net/8139too.c static struct pci_device_id rtl8139_pci_tbl[] = { · line 276

Quoted 8 lines
drivers/net/8139too.c +1 -4
@@ -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

#7 lore raw
Quoted 15 lines · Jeff Garzik
Quoted 6 lines · Alexey Dobriyan
drivers/net/8139too.c +1 -1
-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 },

drivers/net/8139too.c static struct pci_device_id rtl8139_pci_tbl[] = { · line 276

Quoted 8 lines
drivers/net/8139too.c +1 -4
@@ -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 :)

Quoted 1 line

I think the previous version looks better.

Nod.

#8 lore raw
In reply to Alexey Dobriyan
Quoted 20 lines · Alexey Dobriyan

[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.

drivers/net/8139too.c +1 -1
index f4e4298..b895b5f 100644
@@ -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 },

drivers/net/8139too.c static struct pci_device_id rtl8139_pci_tbl[] = { · line 276

Quoted 8 lines
drivers/net/8139too.c +1 -4
@@ -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

author
#9 lore raw

PATCH 1/1 modpost: detect unterminated device id lists

In reply to Greg KH
Quoted 14 lines · Greg KH
Quoted 13 lines · Alexey Dobriyan
Quoted 11 lines · Jeff Garzik
Quoted 8 lines · Kees Cook

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

Quoted 8 lines
Quoted 5 lines

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(-)

---

scripts/mod/file2alias.c +32 -7
diff -urp -x '*.o' linux-2.6.23-rc6~/scripts/mod/file2alias.c linux-2.6.23-rc6/scripts/mod/file2alias.c
@@ -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 */
Signature · 1 line
Kees Cook
#10 lore raw

Re: [PATCH] modpost: detect unterminated device id lists

Quoted 19 lines · On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook <[email protected]> wrote:
Quoted 18 lines · Greg KH
Quoted 17 lines · Alexey Dobriyan
Quoted 11 lines · Jeff Garzik
Quoted 8 lines · Kees Cook

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.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

[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.

scripts/mod/file2alias.c static void device_id_size_check(const c · line 75

Quoted 1 line
scripts/mod/file2alias.c +1 -0
+			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)

#11 lore raw

Re: [PATCH] modpost: detect unterminated device id lists

In reply to Kees Cook
Quoted 2 lines · 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.

#12 lore raw

Re: [PATCH] modpost: detect unterminated device id lists

Quoted 7 lines · Andrew Morton
Quoted 2 lines · 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:

Quoted 7 lines

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 ...

Quoted 11 lines

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.

#13 lore raw

Re: [PATCH] modpost: detect unterminated device id lists

Quoted 14 lines · On Mon, 17 Sep 2007 05:54:45 +0530 "Satyam Sharma" <[email protected]>…
Quoted 13 lines · Andrew Morton
Quoted 2 lines · 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!

Quoted 4 lines
Quoted 1 line

("rusb2/pvrusb2" ??)

Hmm? Are you sure you didn't see any "drivers/media/video/pv" before the "rusb2/pvrusb2" bit?

Fairly. I looked twice.

Quoted 16 lines

Looking at Kees' patch (and the existing code), I've no clue how/why this should happen ... will try to reproduce here ...

Quoted 11 lines

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.

Quoted 8 lines

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

drivers/media/video/pvrusb2/pvrusb2-hdw.c renamed from drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb +1 -1
@@ -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);

#14 lore raw

Re: [PATCH] modpost: detect unterminated device id lists

Quoted 12 lines · Andrew Morton
Quoted 10 lines · On Mon, 17 Sep 2007 05:54:45 +0530 "Satyam Sharma" <[email protected]>…
Quoted 9 lines · Andrew Morton

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!

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

("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 ...

Quoted 30 lines
Quoted 16 lines

Looking at Kees' patch (and the existing code), I've no clue how/why this should happen ... will try to reproduce here ...

Quoted 11 lines

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.

Quoted 8 lines

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

drivers/media/video/pvrusb2/pvrusb2-hdw.c line 50

Quoted 8 lines
drivers/media/video/pvrusb2/pvrusb2-hdw.c renamed from drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb +1 -1
@@ -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

#15 lore raw

Re: [PATCH] modpost: detect unterminated device id lists

Quoted 70 lines · On Tue, 18 Sep 2007 03:15:14 +0530 (IST) Satyam Sharma <[email protected]>…
Quoted 12 lines · Andrew Morton
Quoted 10 lines · On Mon, 17 Sep 2007 05:54:45 +0530 "Satyam Sharma" <[email protected]>…
Quoted 9 lines · Andrew Morton

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!

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

("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 ...

Quoted 30 lines
Quoted 16 lines

Looking at Kees' patch (and the existing code), I've no clue how/why this should happen ... will try to reproduce here ...

Quoted 11 lines

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.

Quoted 8 lines

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

drivers/media/video/pvrusb2/pvrusb2-hdw.c line 50

Quoted 8 lines
drivers/media/video/pvrusb2/pvrusb2-hdw.c renamed from drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb +1 -1
@@ -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).

Quoted 6 lines

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?

#16 lore raw

Re: [PATCH] modpost: detect unterminated device id lists

Hi Andrew,

Em Seg, 2007-09-17 às 14:50 -0700, Andrew Morton escreveu:

Quoted 73 lines
Quoted 70 lines · On Tue, 18 Sep 2007 03:15:14 +0530 (IST) Satyam Sharma <[email protected]>…
Quoted 12 lines · Andrew Morton
Quoted 10 lines · On Mon, 17 Sep 2007 05:54:45 +0530 "Satyam Sharma" <[email protected]>…
Quoted 9 lines · Andrew Morton

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!

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

("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 ...

Quoted 30 lines
Quoted 16 lines

Looking at Kees' patch (and the existing code), I've no clue how/why this should happen ... will try to reproduce here ...

Quoted 11 lines

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.

Quoted 8 lines

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

drivers/media/video/pvrusb2/pvrusb2-hdw.c line 50

Quoted 8 lines
drivers/media/video/pvrusb2/pvrusb2-hdw.c renamed from drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb +1 -1
@@ -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).

Quoted 8 lines
Quoted 6 lines

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.

#17 lore raw

Re: [PATCH] modpost: detect unterminated device id lists

In reply to Kees Cook

Hi Kees,

Quoted 5 lines · Kees Cook

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 :-)

scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 do { · line 62

Quoted 13 lines
scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +4 -2
@@ -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 ...

scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 do { · line 64

Quoted 2 lines
scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +1 -0
 {
+       int i;

uint8_t *p;

scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 static void device_id_size_check(const c · line 75

Quoted 11 lines
scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +3 -0
        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 ?

scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 static void device_id_size_check(const c · line 77

Quoted 2 lines
scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +1 -0
+                       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*.

scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 static void device_id_size_check(const c · line 80

Quoted 3 lines
scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +3 -0
+                       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.

scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 static void device_id_size_check(const c · line 83

Quoted 3 lines
scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +3 -0
+                       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" ?

Quoted 31 lines
scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +2 -0
+               }
+       }
 }

/* USB is special because the bcdDevice can be matched against a numeric range */

scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +3 -2
@@ -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)

scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 void handle_moddevtable(struct module *m · line 555

Quoted 10 lines
scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +5 -3
                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?

Quoted 9 lines
scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +5 -0
+       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,

scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 void handle_moddevtable(struct module *m · line 629

Quoted 8 lines
scripts/mod/file2alias.c 2007-09-12 17:41:30.000000000 -0700 renamed from scripts/mod/file2alias.c 2007-09-11 23:17:49.000000000 -0700 +2 -0
@@ -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

author
#18 lore raw

Re: [PATCH] modpost: detect unterminated device id lists

Hi Satyam,

Quoted 9 lines · Satyam Sharma
Quoted 5 lines · Kees Cook

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! :-)

Quoted 7 lines

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. :)

Quoted 7 lines

scripts/mod/file2alias.c do { · line 62

Quoted 3 lines
scripts/mod/file2alias.c +3 -0
+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.

Quoted 4 lines
        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. ;)

Quoted 4 lines

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.

Quoted 6 lines

scripts/mod/file2alias.c static void device_id_size_check(const c · line 81

Quoted 3 lines
scripts/mod/file2alias.c +3 -0
+                       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.

Quoted 10 lines
Quoted 10 lines
scripts/mod/file2alias.c +1 -0
@@ -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;
Quoted 10 lines

scripts/mod/file2alias.c void handle_moddevtable(struct module *m · line 555

Quoted 8 lines
scripts/mod/file2alias.c +5 -3
-       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

Signature · 1 line
Kees Cook
#19 lore raw

Re: [PATCH] modpost: detect unterminated device id lists

Quoted 4 lines · 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..