linux-modules.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] module: ELF validation enhancement and cleanups
@ 2023-03-19 21:35 Luis Chamberlain
  2023-03-19 21:35 ` [PATCH 1/5] module: add sanity check for ELF module section Luis Chamberlain
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Luis Chamberlain @ 2023-03-19 21:35 UTC (permalink / raw)
  To: linux-modules, linux-kernel, pmladek, david, petr.pavlu, prarit
  Cc: christophe.leroy, song, mcgrof

While doing a cleanup of load_module() to do less work before we allocate [0],
one of the undocumented tricks we pull off is memcpy'ing the struct module
from the module.mod.c into the kernel, with the modifications we've made
to it on load_module(). This puts a bit of love to make the clearer, and
extends our ELF validity checker to ensure we verify this before allowing
us to even process a module.

This effort has discovered a new possible build issue we have to fix:

It is in theory possible today to modify the module struct module size,
let a kernel developer lazily just build the module (say make fs/xfs/)
and then try to insert that module without ensuring the module size
expected should have grown. You can verify the size with:

nm --print-size --size-sort fs/xfs/xfs.ko | grep __this_module
0000000000000000 0000000000000500 D __this_module

The struct module size will be different per each kernel configuration,
and so this is system build dependent. The new ELF check put in place
prevents this situation and also make the use case of memcpying the
struct module very clear, along with ensuring we keep all modifications
we've made to it.

[0] https://lkml.kernel.org/r/20230311051712.4095040-1-mcgrof@kernel.org

Luis Chamberlain (5):
  module: add sanity check for ELF module section
  module: add stop-grap sanity check on module memcpy()
  module: move more elf validity checks to elf_validity_check()
  module: merge remnants of setup_load_info() to elf validation
  module: fold usermode helper kmod into modules directory

 MAINTAINERS                |  13 +--
 kernel/Makefile            |   1 -
 kernel/module/Makefile     |   4 +-
 kernel/{ => module}/kmod.c |   0
 kernel/module/main.c       | 219 ++++++++++++++++++++++++-------------
 5 files changed, 148 insertions(+), 89 deletions(-)
 rename kernel/{ => module}/kmod.c (100%)

-- 
2.39.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/5] module: add sanity check for ELF module section
  2023-03-19 21:35 [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
@ 2023-03-19 21:35 ` Luis Chamberlain
  2023-03-19 21:35 ` [PATCH 2/5] module: add stop-grap sanity check on module memcpy() Luis Chamberlain
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Luis Chamberlain @ 2023-03-19 21:35 UTC (permalink / raw)
  To: linux-modules, linux-kernel, pmladek, david, petr.pavlu, prarit
  Cc: christophe.leroy, song, mcgrof

The ELF ".gnu.linkonce.this_module" section is special, it is what we
use to construct the struct module __this_module, which THIS_MODULE
points to. When userspace loads a module we always deal first with a
copy of the userspace buffer, and twiddle with the userspace copy's
version of the struct module. Eventually we allocate memory to do a
memcpy() of that struct module, under the assumption that the module
size is right. But we have no validity checks against the size or
the requirements for the section.

Add some validity checks for the special module section early and while
at it, cache the module section index early, so we don't have to do that
later.

While at it, just move over the assigment of the info->mod to make the
code clearer. The validity checker also adds an explicit size check to
ensure the module section size matches the kernel's run time size for
sizeof(struct module). This should prevent sloppy loads of modules
which are built today *without* actually increasing the size of
the struct module. A developer today can for example expand the size
of struct module, rebuild a directoroy 'make fs/xfs/' for example and
then try to insmode the driver there. That module would in effect have
an incorrect size. This new size check would put a stop gap against such
mistakes.

This also makes the entire goal of ".gnu.linkonce.this_module" pretty
clear. Before this patch verification of the goal / intent required some
Indian Jones whips, torches and cleaning up big old spider webs.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 kernel/module/main.c | 62 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 9 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index cf097ffe6a4a..e1a9dd51c036 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (C) 2002 Richard Henderson
  * Copyright (C) 2001 Rusty Russell, 2002, 2010 Rusty Russell IBM.
+ * Copyright (C) 2023 Luis Chamberlain <mcgrof@kernel.org>
  */
 
 #define INCLUDE_VERMAGIC
@@ -1656,6 +1657,7 @@ static int elf_validity_check(struct load_info *info)
 	unsigned int i;
 	Elf_Shdr *shdr, *strhdr;
 	int err;
+	unsigned int num_mod_secs = 0, mod_idx;
 
 	if (info->len < sizeof(*(info->hdr))) {
 		pr_err("Invalid ELF header len %lu\n", info->len);
@@ -1767,6 +1769,11 @@ static int elf_validity_check(struct load_info *info)
 					i, shdr->sh_type);
 				return err;
 			}
+			if (strcmp(info->secstrings + shdr->sh_name,
+				   ".gnu.linkonce.this_module") == 0) {
+				num_mod_secs++;
+				mod_idx = i;
+			}
 
 			if (shdr->sh_flags & SHF_ALLOC) {
 				if (shdr->sh_name >= strhdr->sh_size) {
@@ -1779,6 +1786,52 @@ static int elf_validity_check(struct load_info *info)
 		}
 	}
 
+	/*
+	 * The ".gnu.linkonce.this_module" ELF section is special. It is
+	 * what modpost uses to refer to __this_module and let's use rely
+	 * on THIS_MODULE to point to &__this_module properly. The kernel's
+	 * modpost declares it on each modules's *.mod.c file. If the struct
+	 * module of the kernel changes a full kernel rebuild is required.
+	 *
+	 * We have a few expectaions for this special section, the following
+	 * code validates all this for us:
+	 *
+	 *   o Only one section must exist
+	 *   o We expect the kernel to always have to allocate it: SHF_ALLOC
+	 *   o The section size must match the kernel's run time's struct module
+	 *     size
+	 */
+	if (num_mod_secs != 1) {
+		pr_err("Only one .gnu.linkonce.this_module section must exist.\n");
+		goto no_exec;
+	}
+
+	shdr = &info->sechdrs[mod_idx];
+
+	/*
+	 * This is already implied on the switch above, however let's be
+	 * pedantic about it.
+	 */
+	if (shdr->sh_type == SHT_NOBITS) {
+		pr_err(".gnu.linkonce.this_module section must have a size set\n");
+		goto no_exec;
+	}
+
+	if (!(shdr->sh_flags & SHF_ALLOC)) {
+		pr_err(".gnu.linkonce.this_module must occupy memory during process execution\n");
+		goto no_exec;
+	}
+
+	if (shdr->sh_size != sizeof(struct module)) {
+		pr_err(".gnu.linkonce.this_module section size must match the kernel's built struct module size at run time\n");
+		goto no_exec;
+	}
+
+	info->index.mod = mod_idx;
+
+	/* This is temporary: point mod into copy of data. */
+	info->mod = (void *)info->hdr + shdr->sh_offset;
+
 	return 0;
 
 no_exec:
@@ -1925,15 +1978,6 @@ static int setup_load_info(struct load_info *info, int flags)
 		return -ENOEXEC;
 	}
 
-	info->index.mod = find_sec(info, ".gnu.linkonce.this_module");
-	if (!info->index.mod) {
-		pr_warn("%s: No module found in object\n",
-			info->name ?: "(missing .modinfo section or name field)");
-		return -ENOEXEC;
-	}
-	/* This is temporary: point mod into copy of data. */
-	info->mod = (void *)info->hdr + info->sechdrs[info->index.mod].sh_offset;
-
 	/*
 	 * If we didn't load the .modinfo 'name' field earlier, fall back to
 	 * on-disk struct mod 'name' field.
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/5] module: add stop-grap sanity check on module memcpy()
  2023-03-19 21:35 [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
  2023-03-19 21:35 ` [PATCH 1/5] module: add sanity check for ELF module section Luis Chamberlain
@ 2023-03-19 21:35 ` Luis Chamberlain
  2023-03-19 21:35 ` [PATCH 3/5] module: move more elf validity checks to elf_validity_check() Luis Chamberlain
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Luis Chamberlain @ 2023-03-19 21:35 UTC (permalink / raw)
  To: linux-modules, linux-kernel, pmladek, david, petr.pavlu, prarit
  Cc: christophe.leroy, song, mcgrof

The integrity of the struct module we load is important, and although
our ELF validator already checks that the module section must match
struct module, add a stop-gap check before we memcpy() the final minted
module. This also makes those inspecting the code what the goal is.

While at it, clarify the goal behind updating the sh_addr address.
The current comment is pretty misleading.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 kernel/module/main.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index e1a9dd51c036..fbe62d1625bc 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2213,7 +2213,8 @@ static int move_module(struct module *mod, struct load_info *info)
 {
 	int i;
 	void *ptr;
-	enum mod_mem_type t;
+	enum mod_mem_type t = 0;
+	int ret = -ENOMEM;
 
 	for_each_mod_mem_type(type) {
 		if (!mod->mem[type].size) {
@@ -2249,9 +2250,26 @@ static int move_module(struct module *mod, struct load_info *info)
 
 		dest = mod->mem[type].base + (shdr->sh_entsize & SH_ENTSIZE_OFFSET_MASK);
 
-		if (shdr->sh_type != SHT_NOBITS)
+		if (shdr->sh_type != SHT_NOBITS) {
+			/*
+			 * Our ELF checker already validated this, but let's
+			 * be pedantic and make the goal clearer. We actually
+			 * end up copying over all modifications made to the
+			 * userspace copy of the entire struct module.
+			 */
+			if (i == info->index.mod &&
+			   (WARN_ON_ONCE(shdr->sh_size != sizeof(struct module)))) {
+				ret = -ENOEXEC;
+				goto out_enomem;
+			}
 			memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
-		/* Update sh_addr to point to copy in image. */
+		}
+		/*
+		 * Update the userspace copy's ELF section address to point to
+		 * our newly allocated memory as a pure convenience so that
+		 * users of info can keep taking advantage and using the newly
+		 * minted official memory area.
+		 */
 		shdr->sh_addr = (unsigned long)dest;
 		pr_debug("\t0x%lx %s\n",
 			 (long)shdr->sh_addr, info->secstrings + shdr->sh_name);
@@ -2261,7 +2279,7 @@ static int move_module(struct module *mod, struct load_info *info)
 out_enomem:
 	for (t--; t >= 0; t--)
 		module_memory_free(mod->mem[t].base, t);
-	return -ENOMEM;
+	return ret;
 }
 
 static int check_export_symbol_versions(struct module *mod)
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/5] module: move more elf validity checks to elf_validity_check()
  2023-03-19 21:35 [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
  2023-03-19 21:35 ` [PATCH 1/5] module: add sanity check for ELF module section Luis Chamberlain
  2023-03-19 21:35 ` [PATCH 2/5] module: add stop-grap sanity check on module memcpy() Luis Chamberlain
@ 2023-03-19 21:35 ` Luis Chamberlain
  2023-03-19 21:35 ` [PATCH 4/5] module: merge remnants of setup_load_info() to elf validation Luis Chamberlain
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Luis Chamberlain @ 2023-03-19 21:35 UTC (permalink / raw)
  To: linux-modules, linux-kernel, pmladek, david, petr.pavlu, prarit
  Cc: christophe.leroy, song, mcgrof

The symbol and strings section validation currently happen in
setup_load_info() but since they are also doing validity checks
move this to elf_validity_check().

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 kernel/module/main.c | 79 ++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 35 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index fbe62d1625bc..84a7f96cf35a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1658,6 +1658,8 @@ static int elf_validity_check(struct load_info *info)
 	Elf_Shdr *shdr, *strhdr;
 	int err;
 	unsigned int num_mod_secs = 0, mod_idx;
+	unsigned int num_info_secs = 0, info_idx;
+	unsigned int num_sym_secs = 0, sym_idx;
 
 	if (info->len < sizeof(*(info->hdr))) {
 		pr_err("Invalid ELF header len %lu\n", info->len);
@@ -1761,6 +1763,8 @@ static int elf_validity_check(struct load_info *info)
 				       info->hdr->e_shnum);
 				goto no_exec;
 			}
+			num_sym_secs++;
+			sym_idx = i;
 			fallthrough;
 		default:
 			err = validate_section_offset(info, shdr);
@@ -1773,6 +1777,10 @@ static int elf_validity_check(struct load_info *info)
 				   ".gnu.linkonce.this_module") == 0) {
 				num_mod_secs++;
 				mod_idx = i;
+			} else if (strcmp(info->secstrings + shdr->sh_name,
+				   ".modinfo") == 0) {
+				num_info_secs++;
+				info_idx = i;
 			}
 
 			if (shdr->sh_flags & SHF_ALLOC) {
@@ -1786,6 +1794,27 @@ static int elf_validity_check(struct load_info *info)
 		}
 	}
 
+	if (num_info_secs > 1) {
+		pr_err("Only one .modinfo section must exist.\n");
+		goto no_exec;
+	} else if (num_info_secs == 1) {
+		/* Try to find a name early so we can log errors with a module name */
+		info->index.info = info_idx;
+		info->name = get_modinfo(info, "name");
+	}
+
+	if (num_sym_secs != 1) {
+		pr_warn("%s: module has no symbols (stripped?)\n",
+			info->name ?: "(missing .modinfo section or name field)");
+		goto no_exec;
+	}
+
+	/* Sets internal symbols and strings. */
+	info->index.sym = sym_idx;
+	shdr = &info->sechdrs[sym_idx];
+	info->index.str = shdr->sh_link;
+	info->strtab = (char *)info->hdr + info->sechdrs[info->index.str].sh_offset;
+
 	/*
 	 * The ".gnu.linkonce.this_module" ELF section is special. It is
 	 * what modpost uses to refer to __this_module and let's use rely
@@ -1802,7 +1831,8 @@ static int elf_validity_check(struct load_info *info)
 	 *     size
 	 */
 	if (num_mod_secs != 1) {
-		pr_err("Only one .gnu.linkonce.this_module section must exist.\n");
+		pr_err("module %s: Only one .gnu.linkonce.this_module section must exist.\n",
+		       info->name ?: "(missing .modinfo section or name field)");
 		goto no_exec;
 	}
 
@@ -1813,17 +1843,20 @@ static int elf_validity_check(struct load_info *info)
 	 * pedantic about it.
 	 */
 	if (shdr->sh_type == SHT_NOBITS) {
-		pr_err(".gnu.linkonce.this_module section must have a size set\n");
+		pr_err("module %s: .gnu.linkonce.this_module section must have a size set\n",
+		       info->name ?: "(missing .modinfo section or name field)");
 		goto no_exec;
 	}
 
 	if (!(shdr->sh_flags & SHF_ALLOC)) {
-		pr_err(".gnu.linkonce.this_module must occupy memory during process execution\n");
+		pr_err("module %s: .gnu.linkonce.this_module must occupy memory during process execution\n",
+		       info->name ?: "(missing .modinfo section or name field)");
 		goto no_exec;
 	}
 
 	if (shdr->sh_size != sizeof(struct module)) {
-		pr_err(".gnu.linkonce.this_module section size must match the kernel's built struct module size at run time\n");
+		pr_err("module %s: .gnu.linkonce.this_module section size must match the kernel's built struct module size at run time\n",
+		       info->name ?: "(missing .modinfo section or name field)");
 		goto no_exec;
 	}
 
@@ -1832,6 +1865,13 @@ static int elf_validity_check(struct load_info *info)
 	/* This is temporary: point mod into copy of data. */
 	info->mod = (void *)info->hdr + shdr->sh_offset;
 
+	/*
+	 * If we didn't load the .modinfo 'name' field earlier, fall back to
+	 * on-disk struct mod 'name' field.
+	 */
+	if (!info->name)
+		info->name = info->mod->name;
+
 	return 0;
 
 no_exec:
@@ -1954,37 +1994,6 @@ static int rewrite_section_headers(struct load_info *info, int flags)
  */
 static int setup_load_info(struct load_info *info, int flags)
 {
-	unsigned int i;
-
-	/* Try to find a name early so we can log errors with a module name */
-	info->index.info = find_sec(info, ".modinfo");
-	if (info->index.info)
-		info->name = get_modinfo(info, "name");
-
-	/* Find internal symbols and strings. */
-	for (i = 1; i < info->hdr->e_shnum; i++) {
-		if (info->sechdrs[i].sh_type == SHT_SYMTAB) {
-			info->index.sym = i;
-			info->index.str = info->sechdrs[i].sh_link;
-			info->strtab = (char *)info->hdr
-				+ info->sechdrs[info->index.str].sh_offset;
-			break;
-		}
-	}
-
-	if (info->index.sym == 0) {
-		pr_warn("%s: module has no symbols (stripped?)\n",
-			info->name ?: "(missing .modinfo section or name field)");
-		return -ENOEXEC;
-	}
-
-	/*
-	 * If we didn't load the .modinfo 'name' field earlier, fall back to
-	 * on-disk struct mod 'name' field.
-	 */
-	if (!info->name)
-		info->name = info->mod->name;
-
 	if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
 		info->index.vers = 0; /* Pretend no __versions section! */
 	else
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 4/5] module: merge remnants of setup_load_info() to elf validation
  2023-03-19 21:35 [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
                   ` (2 preceding siblings ...)
  2023-03-19 21:35 ` [PATCH 3/5] module: move more elf validity checks to elf_validity_check() Luis Chamberlain
@ 2023-03-19 21:35 ` Luis Chamberlain
  2023-03-19 21:35 ` [PATCH 5/5] module: fold usermode helper kmod into modules directory Luis Chamberlain
  2023-03-22 23:43 ` [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
  5 siblings, 0 replies; 7+ messages in thread
From: Luis Chamberlain @ 2023-03-19 21:35 UTC (permalink / raw)
  To: linux-modules, linux-kernel, pmladek, david, petr.pavlu, prarit
  Cc: christophe.leroy, song, mcgrof

The setup_load_info() was actually had ELF validation checks of its
own. To later cache useful variables as an secondary step just means
looping again over the ELF sections we just validated. We can simply
keep tabs of the key sections of interest as we validate the module
ELF section in one swoop, so do that and merge the two routines
together.

Expand a bit on the documentation / intent / goals.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 kernel/module/main.c | 60 ++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 84a7f96cf35a..929644d79d38 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1647,12 +1647,26 @@ static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
 }
 
 /*
- * Sanity checks against invalid binaries, wrong arch, weird elf version.
+ * Check userspace passed ELF module against our expectations, and cache
+ * useful variables for further processing as we go.
  *
- * Also do basic validity checks against section offsets and sizes, the
+ * This does basic validity checks against section offsets and sizes, the
  * section name string table, and the indices used for it (sh_name).
+ *
+ * As a last step, since we're already checking the ELF sections we cache
+ * useful variables which will be used later for our convenience:
+ *
+ * 	o pointers to section headers
+ * 	o cache the modinfo symbol section
+ * 	o cache the string symbol section
+ * 	o cache the module section
+ *
+ * As a last step we set info->mod to the temporary copy of the module in
+ * info->hdr. The final one will be allocated in move_module(). Any
+ * modifications we make to our copy of the module will be carried over
+ * to the final minted module.
  */
-static int elf_validity_check(struct load_info *info)
+static int elf_validity_cache_copy(struct load_info *info, int flags)
 {
 	unsigned int i;
 	Elf_Shdr *shdr, *strhdr;
@@ -1872,6 +1886,13 @@ static int elf_validity_check(struct load_info *info)
 	if (!info->name)
 		info->name = info->mod->name;
 
+	if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
+		info->index.vers = 0; /* Pretend no __versions section! */
+	else
+		info->index.vers = find_sec(info, "__versions");
+
+	info->index.pcpu = find_pcpusec(info);
+
 	return 0;
 
 no_exec:
@@ -1984,26 +2005,6 @@ static int rewrite_section_headers(struct load_info *info, int flags)
 	return 0;
 }
 
-/*
- * Set up our basic convenience variables (pointers to section headers,
- * search for module section index etc), and do some basic section
- * verification.
- *
- * Set info->mod to the temporary copy of the module in info->hdr. The final one
- * will be allocated in move_module().
- */
-static int setup_load_info(struct load_info *info, int flags)
-{
-	if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
-		info->index.vers = 0; /* Pretend no __versions section! */
-	else
-		info->index.vers = find_sec(info, "__versions");
-
-	info->index.pcpu = find_pcpusec(info);
-
-	return 0;
-}
-
 /*
  * These calls taint the kernel depending certain module circumstances */
 static void module_augment_kernel_taints(struct module *mod, struct load_info *info)
@@ -2809,17 +2810,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	/*
 	 * Do basic sanity checks against the ELF header and
-	 * sections.
-	 */
-	err = elf_validity_check(info);
-	if (err)
-		goto free_copy;
-
-	/*
-	 * Everything checks out, so set up the section info
-	 * in the info structure.
+	 * sections. Cache useful sections and set the
+	 * info->mod to the userspace passed struct module.
 	 */
-	err = setup_load_info(info, flags);
+	err = elf_validity_cache_copy(info, flags);
 	if (err)
 		goto free_copy;
 
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 5/5] module: fold usermode helper kmod into modules directory
  2023-03-19 21:35 [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
                   ` (3 preceding siblings ...)
  2023-03-19 21:35 ` [PATCH 4/5] module: merge remnants of setup_load_info() to elf validation Luis Chamberlain
@ 2023-03-19 21:35 ` Luis Chamberlain
  2023-03-22 23:43 ` [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
  5 siblings, 0 replies; 7+ messages in thread
From: Luis Chamberlain @ 2023-03-19 21:35 UTC (permalink / raw)
  To: linux-modules, linux-kernel, pmladek, david, petr.pavlu, prarit
  Cc: christophe.leroy, song, mcgrof

The kernel/kmod.c is already only built if we enabled modules, so
just stuff it under kernel/module/kmod.c and unify the MAINTAINERS
file for it.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 MAINTAINERS                | 13 +++----------
 kernel/Makefile            |  1 -
 kernel/module/Makefile     |  4 +++-
 kernel/{ => module}/kmod.c |  0
 4 files changed, 6 insertions(+), 12 deletions(-)
 rename kernel/{ => module}/kmod.c (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8d5bc223f305..1ca0e26aa9f8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11522,16 +11522,6 @@ F:	include/linux/kmemleak.h
 F:	mm/kmemleak.c
 F:	samples/kmemleak/kmemleak-test.c
 
-KMOD KERNEL MODULE LOADER - USERMODE HELPER
-M:	Luis Chamberlain <mcgrof@kernel.org>
-L:	linux-kernel@vger.kernel.org
-L:	linux-modules@vger.kernel.org
-S:	Maintained
-F:	include/linux/kmod.h
-F:	kernel/kmod.c
-F:	lib/test_kmod.c
-F:	tools/testing/selftests/kmod/
-
 KMSAN
 M:	Alexander Potapenko <glider@google.com>
 R:	Marco Elver <elver@google.com>
@@ -14083,8 +14073,11 @@ L:	linux-kernel@vger.kernel.org
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
 F:	include/linux/module.h
+F:	include/linux/kmod.h
 F:	kernel/module/
 F:	scripts/module*
+F:	lib/test_kmod.c
+F:	tools/testing/selftests/kmod/
 
 MONOLITHIC POWER SYSTEM PMIC DRIVER
 M:	Saravanan Sekar <sravanhome@gmail.com>
diff --git a/kernel/Makefile b/kernel/Makefile
index 10ef068f598d..3dd4ea433ee9 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -13,7 +13,6 @@ obj-y     = fork.o exec_domain.o panic.o \
 	    async.o range.o smpboot.o ucount.o regset.o
 
 obj-$(CONFIG_USERMODE_DRIVER) += usermode_driver.o
-obj-$(CONFIG_MODULES) += kmod.o
 obj-$(CONFIG_MULTIUSER) += groups.o
 
 ifdef CONFIG_FUNCTION_TRACER
diff --git a/kernel/module/Makefile b/kernel/module/Makefile
index 948efea81e85..5b1d26b53b8d 100644
--- a/kernel/module/Makefile
+++ b/kernel/module/Makefile
@@ -7,7 +7,9 @@
 # and produce insane amounts of uninteresting coverage.
 KCOV_INSTRUMENT_module.o := n
 
-obj-y += main.o strict_rwx.o
+obj-y += main.o
+obj-y += strict_rwx.o
+obj-y += kmod.o
 obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
 obj-$(CONFIG_MODULE_SIG) += signing.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
diff --git a/kernel/kmod.c b/kernel/module/kmod.c
similarity index 100%
rename from kernel/kmod.c
rename to kernel/module/kmod.c
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/5] module: ELF validation enhancement and cleanups
  2023-03-19 21:35 [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
                   ` (4 preceding siblings ...)
  2023-03-19 21:35 ` [PATCH 5/5] module: fold usermode helper kmod into modules directory Luis Chamberlain
@ 2023-03-22 23:43 ` Luis Chamberlain
  5 siblings, 0 replies; 7+ messages in thread
From: Luis Chamberlain @ 2023-03-22 23:43 UTC (permalink / raw)
  To: linux-modules, linux-kernel, pmladek, david, petr.pavlu, prarit
  Cc: christophe.leroy, song

On Sun, Mar 19, 2023 at 02:35:37PM -0700, Luis Chamberlain wrote:
> While doing a cleanup of load_module() to do less work before we allocate [0],
> one of the undocumented tricks we pull off is memcpy'ing the struct module
> from the module.mod.c into the kernel, with the modifications we've made
> to it on load_module(). This puts a bit of love to make the clearer, and
> extends our ELF validity checker to ensure we verify this before allowing
> us to even process a module.
> 
> This effort has discovered a new possible build issue we have to fix:
> 
> It is in theory possible today to modify the module struct module size,
> let a kernel developer lazily just build the module (say make fs/xfs/)
> and then try to insert that module without ensuring the module size
> expected should have grown. You can verify the size with:
> 
> nm --print-size --size-sort fs/xfs/xfs.ko | grep __this_module
> 0000000000000000 0000000000000500 D __this_module
> 
> The struct module size will be different per each kernel configuration,
> and so this is system build dependent. The new ELF check put in place
> prevents this situation and also make the use case of memcpying the
> struct module very clear, along with ensuring we keep all modifications
> we've made to it.
> 
> [0] https://lkml.kernel.org/r/20230311051712.4095040-1-mcgrof@kernel.org

I've taken these into modules-next for more testing. If folks spot                                                                                                                            
issues in them though let me know and I can yank them before the merge                                                                                                                        
window.

  Luis

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-03-22 23:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-19 21:35 [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
2023-03-19 21:35 ` [PATCH 1/5] module: add sanity check for ELF module section Luis Chamberlain
2023-03-19 21:35 ` [PATCH 2/5] module: add stop-grap sanity check on module memcpy() Luis Chamberlain
2023-03-19 21:35 ` [PATCH 3/5] module: move more elf validity checks to elf_validity_check() Luis Chamberlain
2023-03-19 21:35 ` [PATCH 4/5] module: merge remnants of setup_load_info() to elf validation Luis Chamberlain
2023-03-19 21:35 ` [PATCH 5/5] module: fold usermode helper kmod into modules directory Luis Chamberlain
2023-03-22 23:43 ` [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).