Skip to contentSkip to conversation

2.5.48: BUG() at kernel/module.c:1000

DISCUSSION [email protected] 3 participants 4 messages Nov 18, 2002 – Nov 20, 2002 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

[email protected]

author
#1 lore raw
Quoted 11 lines

Date: Mon, 18 Nov 2002 19:06:56 +0100
From: Petr Vandrovec <[email protected]>
To: [email protected]
Cc: [email protected]
Subject: 2.5.48: BUG() at kernel/module.c:1000

Hi Rusty,
I'm trying to get VMware working, and unfortunately new insmod
is not able to load generated module. It died at line 1000 of kernel/module.c, because of used.core_size > mod->core_size:

     INIT=0/0  CORE=34252/34228

Hi,

This appears to be due to the COMMON symbol "errno".

The code (get_sizes) that calculates the amount of space required by the sections assumes that the first one is loaded at address zero (or large alignment) when performing subsequent alignments.

Unfortunately, this is not the case when the actual load takes place because the common area (length common_length) is allocated first. This needs to be rounded up to the strictest alignment of any of the ALLOC sections before the copies start. (Hence the difference of (2**5 - 8) which is apparent in the CORE values above.)

cheers,

-Malcolm

#2 lore raw

In message <[email protected]> you write:

Quoted 3 lines

The code (get_sizes) that calculates the amount of space required by the sections assumes that the first one is loaded at address zero (or large alignment) when performing subsequent alignments.

Please test this patch (Petr, contains fix for you too).

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

kernel/module.c +7 -7
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.48/kernel/module.c working-2.5.48-fixes/kernel/module.c
@@ -788,9 +788,10 @@ static void simplify_symbols(Elf_Shdr *s
 /* Get the total allocation size of the init and non-init sections */
 static struct sizes get_sizes(const Elf_Ehdr *hdr,
 			      const Elf_Shdr *sechdrs,
-			      const char *secstrings)
+			      const char *secstrings,
+			      unsigned long common_length)
 {
-	struct sizes ret = { 0, 0 };
+	struct sizes ret = { 0, common_length };
 	unsigned i;
 
 	/* Everything marked ALLOC (this includes the exported

@@ -943,10 +944,9 @@ static struct module *load_module(void *
 	mod->live = 0;
 	module_unload_init(mod);
 
-	/* How much space will we need?  (Common area in core) */
-	sizes = get_sizes(hdr, sechdrs, secstrings);
-	common_length = read_commons(hdr, &sechdrs[symindex]);
-	sizes.core_size += common_length;
+	/* How much space will we need?  (Common area in first) */
+	sizes = get_sizes(hdr, sechdrs, secstrings,
+			  read_commons(hdr, &sechdrs[symindex]));
 
 	/* Set these up, and allow archs to manipulate them. */
 	mod->core_size = sizes.core_size;

@@ -973,7 +973,7 @@ static struct module *load_module(void *
 	mod->module_core = ptr;
 
 	ptr = module_alloc(mod->init_size);
-	if (!ptr) {
+	if (!ptr && mod->init_size) {
 		err = -ENOMEM;
 		goto free_core;
 	}
#3 lore raw
Quoted 6 lines · Rusty Russell

In message <[email protected]> you write:

Quoted 3 lines

The code (get_sizes) that calculates the amount of space required by the sections assumes that the first one is loaded at address zero (or large alignment) when performing subsequent alignments.

Please test this patch (Petr, contains fix for you too).

Hi Rusty,
I was getting oopses with your patch (due to uninitialized common_length).
With this one (against clean 2.5.48) it works fine (both parport and vmmon can be insmodded/rmmoded (parport only until it is used by lp, but that's another story)).

I also modified copy_sections code to verify that we are not
overrunning allocated regions. So now you should get -ENOEXEC instead of BUG() + corrupted kernel.
Best regards,
Petr Vandrovec [email protected]

kernel/module.c +13 -7
@@ -607,14 +607,17 @@
 {
 	void *dest;
 	unsigned long *use;
+	unsigned long max;
 
 	/* Only copy to init section if there is one */
 	if (strstr(name, ".init") && mod->module_init) {
 		dest = mod->module_init;
 		use = &used->init_size;
+		max = mod->init_size;
 	} else {
 		dest = mod->module_core;
 		use = &used->core_size;
+		max = mod->core_size;
 	}
 
 	/* Align up */

@@ -622,6 +625,9 @@
 	dest += *use;
 	*use += sechdr->sh_size;
 
+	if (*use > max)
+		return ERR_PTR(-ENOEXEC);
+
 	/* May not actually be in the file (eg. bss). */
 	if (sechdr->sh_type != SHT_NOBITS)
 		memcpy(dest, base + sechdr->sh_offset, sechdr->sh_size);

@@ -788,9 +794,10 @@
 /* Get the total allocation size of the init and non-init sections */
 static struct sizes get_sizes(const Elf_Ehdr *hdr,
 			      const Elf_Shdr *sechdrs,
-			      const char *secstrings)
+			      const char *secstrings,
+			      unsigned long common_length)
 {
-	struct sizes ret = { 0, 0 };
+	struct sizes ret = { 0, common_length };
 	unsigned i;
 
 	/* Everything marked ALLOC (this includes the exported

@@ -943,10 +950,9 @@
 	mod->live = 0;
 	module_unload_init(mod);
 
-	/* How much space will we need?  (Common area in core) */
-	sizes = get_sizes(hdr, sechdrs, secstrings);
-	common_length = read_commons(hdr, &sechdrs[symindex]);
-	sizes.core_size += common_length;
+	/* How much space will we need?  (Common area in first) */
+	sizes = get_sizes(hdr, sechdrs, secstrings,
+			  common_length = read_commons(hdr, &sechdrs[symindex]));
 
 	/* Set these up, and allow archs to manipulate them. */
 	mod->core_size = sizes.core_size;

@@ -973,7 +979,7 @@
 	mod->module_core = ptr;
 
 	ptr = module_alloc(mod->init_size);
-	if (!ptr) {
+	if (!ptr && mod->init_size) {
 		err = -ENOMEM;
 		goto free_core;
 	}
#4 lore raw

In message <20021119115444.GA2011@vana> you write:

Quoted 14 lines · Rusty Russell
Quoted 6 lines

In message <[email protected]> you write:

Quoted 3 lines

The code (get_sizes) that calculates the amount of space required by the sections assumes that the first one is loaded at address zero (or large alignment) when performing subsequent alignments.

Please test this patch (Petr, contains fix for you too).

Hi Rusty,
I was getting oopses with your patch (due to uninitialized common_length).
With this one (against clean 2.5.48) it works fine (both parport and vmmon can be insmodded/rmmoded (parport only until it is used by lp, but that's another story)).

Quoted 3 lines

I also modified copy_sections code to verify that we are not
overrunning allocated regions. So now you should get -ENOEXEC instead of BUG() + corrupted kernel.

Hmm, OK. It *really* shouldn't happen unless there's a bug though.

Anyway, I made a tiny cleanup (I prefer not to do assignments inside function parameters, it's icky).

Linus, please apply: fixes miscalculation of required module size due to alignment issues, and also doesn't think that no init section is an allocation failure.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Module length calculation fix and module with no init fix
Author: Rusty Russell and Petr Vandrovec
Status: Tested on 2.5.48
Depends: Module/module.patch.gz

D: Fixes miscalculation of required module size due to alignment
D: issues of first section after common, and also doesn't think that
D: no init section is an allocation failure.

kernel/module.c +12 -6
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/kernel/module.c working-2.5-bk-spacefix/kernel/module.c
@@ -592,14 +592,17 @@ static void *copy_section(const char *na
 {
 	void *dest;
 	unsigned long *use;
+	unsigned long max;
 
 	/* Only copy to init section if there is one */
 	if (strstr(name, ".init") && mod->module_init) {
 		dest = mod->module_init;
 		use = &used->init_size;
+		max = mod->init_size;
 	} else {
 		dest = mod->module_core;
 		use = &used->core_size;
+		max = mod->core_size;
 	}
 
 	/* Align up */

@@ -607,6 +610,9 @@ static void *copy_section(const char *na
 	dest += *use;
 	*use += sechdr->sh_size;
 
+	if (*use > max)
+		return ERR_PTR(-ENOEXEC);
+
 	/* May not actually be in the file (eg. bss). */
 	if (sechdr->sh_type != SHT_NOBITS)
 		memcpy(dest, base + sechdr->sh_offset, sechdr->sh_size);

@@ -773,9 +779,10 @@ static void simplify_symbols(Elf_Shdr *s
 /* Get the total allocation size of the init and non-init sections */
 static struct sizes get_sizes(const Elf_Ehdr *hdr,
 			      const Elf_Shdr *sechdrs,
-			      const char *secstrings)
+			      const char *secstrings,
+			      unsigned long common_length)
 {
-	struct sizes ret = { 0, 0 };
+	struct sizes ret = { 0, common_length };
 	unsigned i;
 
 	/* Everything marked ALLOC (this includes the exported

@@ -933,10 +940,9 @@ static struct module *load_module(void *
 	mod->live = 0;
 	module_unload_init(mod);
 
-	/* How much space will we need?  (Common area in core) */
-	sizes = get_sizes(hdr, sechdrs, secstrings);
+	/* How much space will we need?  (Common area in first) */
 	common_length = read_commons(hdr, &sechdrs[symindex]);
-	sizes.core_size += common_length;
+	sizes = get_sizes(hdr, sechdrs, secstrings, common_length);
 
 	/* Set these up, and allow archs to manipulate them. */
 	mod->core_size = sizes.core_size;

@@ -963,7 +969,7 @@ static struct module *load_module(void *
 	mod->module_core = ptr;
 
 	ptr = module_alloc(mod->init_size);
-	if (!ptr) {
+	if (!ptr && mod->init_size) {
 		err = -ENOMEM;
 		goto free_core;
 	}