[PATCH 00/12] module: cleanup and call taints after is inserted

[PATCH 00/12] module: cleanup and call taints after is inserted

[Luis Chamberlain]

After posting my first RFC for "module: avoid userspace pressure on unwanted
allocations" [0] I ended up doing much more cleanup on the module loading path.
One of the things that became evident while ensuring we do *less* work before
kmalloc all the things we need for the final module is we are doing a lot of
work before we even add a module onto our linked list, once its accepted for
loading and running init. We even *taint* the kernel even before we accept
a module. We also do some tainting after kernel loading.

This converges both to one point -- right as soon as we accept module
into our linked list. That is, the module is valid as per our kernel
config and we're ready to go. Most of this is just tidying code up. The
biggest functional changes is under the patch "converge taint work together".

I'll post the other functional changes in two other patch sets. This is
mostly cleanup, the next one is the new ELF checks / sanity / cleanup,
and I'm waiting to hear back from David Hildenbrand on the worthiness of
some clutches for allocation. That last part would go in the last patch
series.

In this series I've dropped completely the idea of using aliasing since
different modules can share the same alias, so using that to check if
a module is already loaded turns out not to be useful in any way.

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

Luis Chamberlain (12):
  module: move get_modinfo() helpers all above
  module: rename next_string() to module_next_tag_pair()
  module: add a for_each_modinfo_entry()
  module: move early sanity checks into a helper
  module: move check_modinfo() early to early_mod_check()
  module: rename set_license() to module_license_taint_check()
  module: split taint work out of check_modinfo_livepatch()
  module: split taint adding with info checking
  module: move tainting until after a module hits our linked list
  module: move signature taint to module_augment_kernel_taints()
  module: converge taint work together
  module: rename check_module_license_and_versions() to
    check_export_symbol_versions()

 kernel/module/internal.h |   5 +
 kernel/module/main.c     | 292 ++++++++++++++++++++-------------------
 2 files changed, 158 insertions(+), 139 deletions(-)

-- 
2.39.1

[PATCH 01/12] module: move get_modinfo() helpers all above

[Luis Chamberlain]

Instead of forward declaring routines for get_modinfo() just move
everything up. This makes no functional changes.

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

diff --git a/kernel/module/main.c b/kernel/module/main.c
index b4759f1695b7..1e739f534100 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1016,9 +1016,55 @@ int try_to_force_load(struct module *mod, const char *reason)
 #endif
 }
 
-static char *get_modinfo(const struct load_info *info, const char *tag);
+/* Parse tag=value strings from .modinfo section */
+static char *next_string(char *string, unsigned long *secsize)
+{
+	/* Skip non-zero chars */
+	while (string[0]) {
+		string++;
+		if ((*secsize)-- <= 1)
+			return NULL;
+	}
+
+	/* Skip any zero padding. */
+	while (!string[0]) {
+		string++;
+		if ((*secsize)-- <= 1)
+			return NULL;
+	}
+	return string;
+}
+
 static char *get_next_modinfo(const struct load_info *info, const char *tag,
-			      char *prev);
+			      char *prev)
+{
+	char *p;
+	unsigned int taglen = strlen(tag);
+	Elf_Shdr *infosec = &info->sechdrs[info->index.info];
+	unsigned long size = infosec->sh_size;
+
+	/*
+	 * get_modinfo() calls made before rewrite_section_headers()
+	 * must use sh_offset, as sh_addr isn't set!
+	 */
+	char *modinfo = (char *)info->hdr + infosec->sh_offset;
+
+	if (prev) {
+		size -= prev - modinfo;
+		modinfo = next_string(prev, &size);
+	}
+
+	for (p = modinfo; p; p = next_string(p, &size)) {
+		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
+			return p + taglen + 1;
+	}
+	return NULL;
+}
+
+static char *get_modinfo(const struct load_info *info, const char *tag)
+{
+	return get_next_modinfo(info, tag, NULL);
+}
 
 static int verify_namespace_is_imported(const struct load_info *info,
 					const struct kernel_symbol *sym,
@@ -1544,56 +1590,6 @@ static void set_license(struct module *mod, const char *license)
 	}
 }
 
-/* Parse tag=value strings from .modinfo section */
-static char *next_string(char *string, unsigned long *secsize)
-{
-	/* Skip non-zero chars */
-	while (string[0]) {
-		string++;
-		if ((*secsize)-- <= 1)
-			return NULL;
-	}
-
-	/* Skip any zero padding. */
-	while (!string[0]) {
-		string++;
-		if ((*secsize)-- <= 1)
-			return NULL;
-	}
-	return string;
-}
-
-static char *get_next_modinfo(const struct load_info *info, const char *tag,
-			      char *prev)
-{
-	char *p;
-	unsigned int taglen = strlen(tag);
-	Elf_Shdr *infosec = &info->sechdrs[info->index.info];
-	unsigned long size = infosec->sh_size;
-
-	/*
-	 * get_modinfo() calls made before rewrite_section_headers()
-	 * must use sh_offset, as sh_addr isn't set!
-	 */
-	char *modinfo = (char *)info->hdr + infosec->sh_offset;
-
-	if (prev) {
-		size -= prev - modinfo;
-		modinfo = next_string(prev, &size);
-	}
-
-	for (p = modinfo; p; p = next_string(p, &size)) {
-		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
-			return p + taglen + 1;
-	}
-	return NULL;
-}
-
-static char *get_modinfo(const struct load_info *info, const char *tag)
-{
-	return get_next_modinfo(info, tag, NULL);
-}
-
 static void setup_modinfo(struct module *mod, struct load_info *info)
 {
 	struct module_attribute *attr;
-- 
2.39.1

[PATCH 02/12] module: rename next_string() to module_next_tag_pair()

[Luis Chamberlain]

This makes it clearer what it is doing. While at it,
make it available to other code other than main.c.
This will be used in the subsequent patch and make
the changes easier to read.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 kernel/module/internal.h | 2 ++
 kernel/module/main.c     | 6 +++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index e3883b7d4840..1fa2328636ec 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -96,6 +96,8 @@ long module_get_offset_and_type(struct module *mod, enum mod_mem_type type,
 char *module_flags(struct module *mod, char *buf, bool show_state);
 size_t module_flags_taint(unsigned long taints, char *buf);
 
+char *module_next_tag_pair(char *string, unsigned long *secsize);
+
 static inline void module_assert_mutex_or_preempt(void)
 {
 #ifdef CONFIG_LOCKDEP
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 1e739f534100..ebb5e6b92a48 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1017,7 +1017,7 @@ int try_to_force_load(struct module *mod, const char *reason)
 }
 
 /* Parse tag=value strings from .modinfo section */
-static char *next_string(char *string, unsigned long *secsize)
+char *module_next_tag_pair(char *string, unsigned long *secsize)
 {
 	/* Skip non-zero chars */
 	while (string[0]) {
@@ -1051,10 +1051,10 @@ static char *get_next_modinfo(const struct load_info *info, const char *tag,
 
 	if (prev) {
 		size -= prev - modinfo;
-		modinfo = next_string(prev, &size);
+		modinfo = module_next_tag_pair(prev, &size);
 	}
 
-	for (p = modinfo; p; p = next_string(p, &size)) {
+	for (p = modinfo; p; p = module_next_tag_pair(p, &size)) {
 		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
 			return p + taglen + 1;
 	}
-- 
2.39.1

[PATCH 03/12] module: add a for_each_modinfo_entry()

[Luis Chamberlain]

Add a for_each_modinfo_entry() to make it easier to read and use.
This produces no functional changes but makes this code easiert
to read as we are used to with loops in the kernel and trims more
lines of code.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 kernel/module/internal.h | 3 +++
 kernel/module/main.c     | 5 +----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 1fa2328636ec..6ae29bb8836f 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -98,6 +98,9 @@ size_t module_flags_taint(unsigned long taints, char *buf);
 
 char *module_next_tag_pair(char *string, unsigned long *secsize);
 
+#define for_each_modinfo_entry(entry, info, name) \
+	for (entry = get_modinfo(info, name); entry; entry = get_next_modinfo(info, name, entry))
+
 static inline void module_assert_mutex_or_preempt(void)
 {
 #ifdef CONFIG_LOCKDEP
diff --git a/kernel/module/main.c b/kernel/module/main.c
index ebb5e6b92a48..427284ab31f1 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1075,12 +1075,9 @@ static int verify_namespace_is_imported(const struct load_info *info,
 
 	namespace = kernel_symbol_namespace(sym);
 	if (namespace && namespace[0]) {
-		imported_namespace = get_modinfo(info, "import_ns");
-		while (imported_namespace) {
+		for_each_modinfo_entry(imported_namespace, info, "import_ns") {
 			if (strcmp(namespace, imported_namespace) == 0)
 				return 0;
-			imported_namespace = get_next_modinfo(
-				info, "import_ns", imported_namespace);
 		}
 #ifdef CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS
 		pr_warn(
-- 
2.39.1

[PATCH 04/12] module: move early sanity checks into a helper

[Luis Chamberlain]

Move early sanity checkers for the module into a helper.
This let's us make it clear when we are working with the
local copy of the module prior to allocation.

This produces no functional changes, it just makes subsequent
changes easier to read.

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

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 427284ab31f1..933cef72ae13 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2668,6 +2668,31 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname,
 	return 0;
 }
 
+/* Module within temporary copy, this doesn't do any allocation  */
+static int early_mod_check(struct load_info *info, int flags)
+{
+	int err;
+
+	/*
+	 * Now that we know we have the correct module name, check
+	 * if it's blacklisted.
+	 */
+	if (blacklisted(info->name)) {
+		pr_err("Module %s is blacklisted\n", info->name);
+		return -EPERM;
+	}
+
+	err = rewrite_section_headers(info, flags);
+	if (err)
+		return err;
+
+	/* Check module struct version now, before we try to use module. */
+	if (!check_modstruct_version(info, info->mod))
+		return ENOEXEC;
+
+	return 0;
+}
+
 /*
  * Allocate and load the module: note that size of section 0 is always
  * zero, and we rely on this for optional sections.
@@ -2711,26 +2736,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err)
 		goto free_copy;
 
-	/*
-	 * Now that we know we have the correct module name, check
-	 * if it's blacklisted.
-	 */
-	if (blacklisted(info->name)) {
-		err = -EPERM;
-		pr_err("Module %s is blacklisted\n", info->name);
-		goto free_copy;
-	}
-
-	err = rewrite_section_headers(info, flags);
+	err = early_mod_check(info, flags);
 	if (err)
 		goto free_copy;
 
-	/* Check module struct version now, before we try to use module. */
-	if (!check_modstruct_version(info, info->mod)) {
-		err = -ENOEXEC;
-		goto free_copy;
-	}
-
 	/* Figure out module layout, and allocate all the memory. */
 	mod = layout_and_allocate(info, flags);
 	if (IS_ERR(mod)) {
-- 
2.39.1

[PATCH 05/12] module: move check_modinfo() early to early_mod_check()

[Luis Chamberlain]

This moves check_modinfo() to early_mod_check(). This
doesn't make any functional changes either, as check_modinfo()
was the first call on layout_and_allocate(), so we're just
moving it back one routine and at the end.

This let's us keep separate the checkers from the allocator.

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

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 933cef72ae13..95fd705328ac 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2273,10 +2273,6 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	unsigned int ndx;
 	int err;
 
-	err = check_modinfo(info->mod, info, flags);
-	if (err)
-		return ERR_PTR(err);
-
 	/* Allow arches to frob section contents and sizes.  */
 	err = module_frob_arch_sections(info->hdr, info->sechdrs,
 					info->secstrings, info->mod);
@@ -2688,7 +2684,11 @@ static int early_mod_check(struct load_info *info, int flags)
 
 	/* Check module struct version now, before we try to use module. */
 	if (!check_modstruct_version(info, info->mod))
-		return ENOEXEC;
+		return -ENOEXEC;
+
+	err = check_modinfo(info->mod, info, flags);
+	if (err)
+		return err;
 
 	return 0;
 }
-- 
2.39.1

[PATCH 06/12] module: rename set_license() to module_license_taint_check()

[Luis Chamberlain]

The set_license() routine would seem to a reader to do some sort of
setting, but it does not. It just adds a taint if the license is
not set or proprietary.

This makes what the code is doing clearer, so much we can remove
the comment about it.

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

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 95fd705328ac..5e64485ac05a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1573,7 +1573,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
 	__layout_sections(mod, info, true);
 }
 
-static void set_license(struct module *mod, const char *license)
+static void module_license_taint_check(struct module *mod, const char *license)
 {
 	if (!license)
 		license = "unspecified";
@@ -1993,8 +1993,7 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 	if (err)
 		return err;
 
-	/* Set up license info based on the info section */
-	set_license(mod, get_modinfo(info, "license"));
+	module_license_taint_check(mod, get_modinfo(info, "license"));
 
 	if (get_modinfo(info, "test")) {
 		if (!test_taint(TAINT_TEST))
-- 
2.39.1

[PATCH 07/12] module: split taint work out of check_modinfo_livepatch()

[Luis Chamberlain]

The work to taint the kernel due to a module should be split
up eventually. To aid with this, split up the tainting on
check_modinfo_livepatch().

This let's us bring more early checks together which do return
a value, and makes changes easier to read later where we stuff
all the work to do the taints in one single routine.

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

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 5e64485ac05a..cfb2ff5185fe 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1808,12 +1808,8 @@ static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
 		/* Nothing more to do */
 		return 0;
 
-	if (set_livepatch_module(mod)) {
-		add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
-		pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
-				mod->name);
+	if (set_livepatch_module(mod))
 		return 0;
-	}
 
 	pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
 	       mod->name);
@@ -1993,6 +1989,11 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 	if (err)
 		return err;
 
+	if (is_livepatch_module(mod)) {
+		add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
+		pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
+				mod->name);
+	}
 	module_license_taint_check(mod, get_modinfo(info, "license"));
 
 	if (get_modinfo(info, "test")) {
-- 
2.39.1

[PATCH 08/12] module: split taint adding with info checking

[Luis Chamberlain]

check_modinfo() actually does two things:

 a) sanity checks, some of which are fatal, and so we
    prevent the user from completing trying to load a module
 b) taints the kernel

The taints are pretty heavy handed because we're tainting the kernel
*before* we ever even get to load the module into the modules linked
list. That is, it it can fail for other reasons later as we review the
module's structure.

But this commit makes no functional changes, it just makes the intent
clearer and splits the code up where needed to make that happen.

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

diff --git a/kernel/module/main.c b/kernel/module/main.c
index cfb2ff5185fe..a3953ca18090 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1951,25 +1951,10 @@ static int setup_load_info(struct load_info *info, int flags)
 	return 0;
 }
 
-static int check_modinfo(struct module *mod, struct load_info *info, int flags)
+/*
+ * These calls taint the kernel depending certain module circumstances */
+static void module_augment_kernel_taints(struct module *mod, struct load_info *info)
 {
-	const char *modmagic = get_modinfo(info, "vermagic");
-	int err;
-
-	if (flags & MODULE_INIT_IGNORE_VERMAGIC)
-		modmagic = NULL;
-
-	/* This is allowed: modprobe --force will invalidate it. */
-	if (!modmagic) {
-		err = try_to_force_load(mod, "bad vermagic");
-		if (err)
-			return err;
-	} else if (!same_magic(modmagic, vermagic, info->index.vers)) {
-		pr_err("%s: version magic '%s' should be '%s'\n",
-		       info->name, modmagic, vermagic);
-		return -ENOEXEC;
-	}
-
 	if (!get_modinfo(info, "intree")) {
 		if (!test_taint(TAINT_OOT_MODULE))
 			pr_warn("%s: loading out-of-tree module taints kernel.\n",
@@ -1985,15 +1970,12 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 			"is unknown, you have been warned.\n", mod->name);
 	}
 
-	err = check_modinfo_livepatch(mod, info);
-	if (err)
-		return err;
-
 	if (is_livepatch_module(mod)) {
 		add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
 		pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
 				mod->name);
 	}
+
 	module_license_taint_check(mod, get_modinfo(info, "license"));
 
 	if (get_modinfo(info, "test")) {
@@ -2002,6 +1984,42 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 				mod->name);
 		add_taint_module(mod, TAINT_TEST, LOCKDEP_STILL_OK);
 	}
+}
+
+static int check_modinfo(struct module *mod, struct load_info *info, int flags)
+{
+	const char *modmagic = get_modinfo(info, "vermagic");
+	int err;
+
+	if (flags & MODULE_INIT_IGNORE_VERMAGIC)
+		modmagic = NULL;
+
+	/* This is allowed: modprobe --force will invalidate it. */
+	if (!modmagic) {
+		err = try_to_force_load(mod, "bad vermagic");
+		if (err)
+			return err;
+	} else if (!same_magic(modmagic, vermagic, info->index.vers)) {
+		pr_err("%s: version magic '%s' should be '%s'\n",
+		       info->name, modmagic, vermagic);
+		return -ENOEXEC;
+	}
+
+	err = check_modinfo_livepatch(mod, info);
+	if (err)
+		return err;
+
+	/*
+	 * We are tainting your kernel *even* if you try to load
+	 * modules with possible taints and we fail to load these
+	 * modules for other reasons.
+	 *
+	 * We have a descrepancy though, see the other taints for
+	 * signature and those in check_module_license_and_versions().
+	 *
+	 * We should compromise and converge.
+	 */
+	module_augment_kernel_taints(mod, info);
 
 	return 0;
 }
-- 
2.39.1

[PATCH 09/12] module: move tainting until after a module hits our linked list

[Luis Chamberlain]

It is silly to have taints spread out all over, we can just compromise
and add them if the module ever hit our linked list. Our sanity checkers
should just prevent crappy drivers / bogus ELF modules / etc and kconfig
options should be enough to let you *not* load things you don't want.

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

diff --git a/kernel/module/main.c b/kernel/module/main.c
index a3953ca18090..1aa71f82aca2 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2009,18 +2009,6 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 	if (err)
 		return err;
 
-	/*
-	 * We are tainting your kernel *even* if you try to load
-	 * modules with possible taints and we fail to load these
-	 * modules for other reasons.
-	 *
-	 * We have a descrepancy though, see the other taints for
-	 * signature and those in check_module_license_and_versions().
-	 *
-	 * We should compromise and converge.
-	 */
-	module_augment_kernel_taints(mod, info);
-
 	return 0;
 }
 
@@ -2772,6 +2760,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err)
 		goto free_module;
 
+	/*
+	 * We are tainting your kernel if your module gets into
+	 * the modules linked list somehow.
+	 *
+	 * We have a descrepancy though, see the other taints for
+	 * signature and those in check_module_license_and_versions().
+	 *
+	 * We should compromise and converge.
+	 */
+	module_augment_kernel_taints(mod, info);
 #ifdef CONFIG_MODULE_SIG
 	mod->sig_ok = info->sig_ok;
 	if (!mod->sig_ok) {
-- 
2.39.1

[PATCH 10/12] module: move signature taint to module_augment_kernel_taints()

[Luis Chamberlain]

Just move the signature taint into the helper:

  module_augment_kernel_taints()

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

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 1aa71f82aca2..2f1988137965 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1984,6 +1984,15 @@ static void module_augment_kernel_taints(struct module *mod, struct load_info *i
 				mod->name);
 		add_taint_module(mod, TAINT_TEST, LOCKDEP_STILL_OK);
 	}
+#ifdef CONFIG_MODULE_SIG
+	mod->sig_ok = info->sig_ok;
+	if (!mod->sig_ok) {
+		pr_notice_once("%s: module verification failed: signature "
+			       "and/or required key missing - tainting "
+			       "kernel\n", mod->name);
+		add_taint_module(mod, TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK);
+	}
+#endif
 }
 
 static int check_modinfo(struct module *mod, struct load_info *info, int flags)
@@ -2770,15 +2779,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	 * We should compromise and converge.
 	 */
 	module_augment_kernel_taints(mod, info);
-#ifdef CONFIG_MODULE_SIG
-	mod->sig_ok = info->sig_ok;
-	if (!mod->sig_ok) {
-		pr_notice_once("%s: module verification failed: signature "
-			       "and/or required key missing - tainting "
-			       "kernel\n", mod->name);
-		add_taint_module(mod, TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK);
-	}
-#endif
 
 	/* To avoid stressing percpu allocator, do this once we're unique. */
 	err = percpu_modalloc(mod, info);
-- 
2.39.1

[PATCH 11/12] module: converge taint work together

[Luis Chamberlain]

Converge on a compromise: so long as we have a module hit our linked
list of modules we taint. That is, the module was about to become live.

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

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 2f1988137965..f165d93a4ef9 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1955,6 +1955,8 @@ static int setup_load_info(struct load_info *info, int flags)
  * These calls taint the kernel depending certain module circumstances */
 static void module_augment_kernel_taints(struct module *mod, struct load_info *info)
 {
+	int prev_taint = test_taint(TAINT_PROPRIETARY_MODULE);
+
 	if (!get_modinfo(info, "intree")) {
 		if (!test_taint(TAINT_OOT_MODULE))
 			pr_warn("%s: loading out-of-tree module taints kernel.\n",
@@ -1993,6 +1995,28 @@ static void module_augment_kernel_taints(struct module *mod, struct load_info *i
 		add_taint_module(mod, TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK);
 	}
 #endif
+
+	/*
+	 * ndiswrapper is under GPL by itself, but loads proprietary modules.
+	 * Don't use add_taint_module(), as it would prevent ndiswrapper from
+	 * using GPL-only symbols it needs.
+	 */
+	if (strcmp(mod->name, "ndiswrapper") == 0)
+		add_taint(TAINT_PROPRIETARY_MODULE, LOCKDEP_NOW_UNRELIABLE);
+
+	/* driverloader was caught wrongly pretending to be under GPL */
+	if (strcmp(mod->name, "driverloader") == 0)
+		add_taint_module(mod, TAINT_PROPRIETARY_MODULE,
+				 LOCKDEP_NOW_UNRELIABLE);
+
+	/* lve claims to be GPL but upstream won't provide source */
+	if (strcmp(mod->name, "lve") == 0)
+		add_taint_module(mod, TAINT_PROPRIETARY_MODULE,
+				 LOCKDEP_NOW_UNRELIABLE);
+
+	if (!prev_taint && test_taint(TAINT_PROPRIETARY_MODULE))
+		pr_warn("%s: module license taints kernel.\n", mod->name);
+
 }
 
 static int check_modinfo(struct module *mod, struct load_info *info, int flags)
@@ -2198,29 +2222,6 @@ static int move_module(struct module *mod, struct load_info *info)
 
 static int check_module_license_and_versions(struct module *mod)
 {
-	int prev_taint = test_taint(TAINT_PROPRIETARY_MODULE);
-
-	/*
-	 * ndiswrapper is under GPL by itself, but loads proprietary modules.
-	 * Don't use add_taint_module(), as it would prevent ndiswrapper from
-	 * using GPL-only symbols it needs.
-	 */
-	if (strcmp(mod->name, "ndiswrapper") == 0)
-		add_taint(TAINT_PROPRIETARY_MODULE, LOCKDEP_NOW_UNRELIABLE);
-
-	/* driverloader was caught wrongly pretending to be under GPL */
-	if (strcmp(mod->name, "driverloader") == 0)
-		add_taint_module(mod, TAINT_PROPRIETARY_MODULE,
-				 LOCKDEP_NOW_UNRELIABLE);
-
-	/* lve claims to be GPL but upstream won't provide source */
-	if (strcmp(mod->name, "lve") == 0)
-		add_taint_module(mod, TAINT_PROPRIETARY_MODULE,
-				 LOCKDEP_NOW_UNRELIABLE);
-
-	if (!prev_taint && test_taint(TAINT_PROPRIETARY_MODULE))
-		pr_warn("%s: module license taints kernel.\n", mod->name);
-
 #ifdef CONFIG_MODVERSIONS
 	if ((mod->num_syms && !mod->crcs) ||
 	    (mod->num_gpl_syms && !mod->gpl_crcs)) {
@@ -2772,11 +2773,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	/*
 	 * We are tainting your kernel if your module gets into
 	 * the modules linked list somehow.
-	 *
-	 * We have a descrepancy though, see the other taints for
-	 * signature and those in check_module_license_and_versions().
-	 *
-	 * We should compromise and converge.
 	 */
 	module_augment_kernel_taints(mod, info);
 
-- 
2.39.1

[PATCH 12/12] module: rename check_module_license_and_versions() to check_export_symbol_versions()

[Luis Chamberlain]

This makes the routine easier to understand what the check its checking for.

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

diff --git a/kernel/module/main.c b/kernel/module/main.c
index f165d93a4ef9..cf097ffe6a4a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2220,7 +2220,7 @@ static int move_module(struct module *mod, struct load_info *info)
 	return -ENOMEM;
 }
 
-static int check_module_license_and_versions(struct module *mod)
+static int check_export_symbol_versions(struct module *mod)
 {
 #ifdef CONFIG_MODVERSIONS
 	if ((mod->num_syms && !mod->crcs) ||
@@ -2796,7 +2796,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err)
 		goto free_unload;
 
-	err = check_module_license_and_versions(mod);
+	err = check_export_symbol_versions(mod);
 	if (err)
 		goto free_unload;
 
-- 
2.39.1

Re: [PATCH 00/12] module: cleanup and call taints after is inserted

[Luis Chamberlain]

On Sun, Mar 19, 2023 at 02:27:34PM -0700, Luis Chamberlain wrote:
> After posting my first RFC for "module: avoid userspace pressure on unwanted
> allocations" [0] I ended up doing much more cleanup on the module loading path.
> One of the things that became evident while ensuring we do *less* work before
> kmalloc all the things we need for the final module is we are doing a lot of
> work before we even add a module onto our linked list, once its accepted for
> loading and running init. We even *taint* the kernel even before we accept
> a module. We also do some tainting after kernel loading.
> 
> This converges both to one point -- right as soon as we accept module
> into our linked list. That is, the module is valid as per our kernel
> config and we're ready to go. Most of this is just tidying code up. The
> biggest functional changes is under the patch "converge taint work together".
> 
> I'll post the other functional changes in two other patch sets. This is
> mostly cleanup, the next one is the new ELF checks / sanity / cleanup,
> and I'm waiting to hear back from David Hildenbrand on the worthiness of
> some clutches for allocation. That last part would go in the last patch
> series.
> 
> In this series I've dropped completely the idea of using aliasing since
> different modules can share the same alias, so using that to check if
> a module is already loaded turns out not to be useful in any way.
> 
> [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

Re: [PATCH 04/12] module: move early sanity checks into a helper

[Petr Pavlu]

On 3/19/23 22:27, Luis Chamberlain wrote:
> Move early sanity checkers for the module into a helper.
> This let's us make it clear when we are working with the
> local copy of the module prior to allocation.
> 
> This produces no functional changes, it just makes subsequent
> changes easier to read.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  kernel/module/main.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 427284ab31f1..933cef72ae13 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2668,6 +2668,31 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname,
>  	return 0;
>  }
>  
> +/* Module within temporary copy, this doesn't do any allocation  */
> +static int early_mod_check(struct load_info *info, int flags)
> +{
> +	int err;
> +
> +	/*
> +	 * Now that we know we have the correct module name, check
> +	 * if it's blacklisted.
> +	 */
> +	if (blacklisted(info->name)) {
> +		pr_err("Module %s is blacklisted\n", info->name);
> +		return -EPERM;
> +	}
> +
> +	err = rewrite_section_headers(info, flags);
> +	if (err)
> +		return err;
> +
> +	/* Check module struct version now, before we try to use module. */
> +	if (!check_modstruct_version(info, info->mod))
> +		return ENOEXEC;

The error value when check_modstruct_version() fails is changed in this patch
from -ENOEXEC to ENOEXEC and updated back again in the next patch. It would be
good to avoid introducing this temporary problem and keep the value throughout
as -ENOEXEC.

> +
> +	return 0;
> +}
> +
>  /*
>   * Allocate and load the module: note that size of section 0 is always
>   * zero, and we rely on this for optional sections.
> @@ -2711,26 +2736,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	if (err)
>  		goto free_copy;
>  
> -	/*
> -	 * Now that we know we have the correct module name, check
> -	 * if it's blacklisted.
> -	 */
> -	if (blacklisted(info->name)) {
> -		err = -EPERM;
> -		pr_err("Module %s is blacklisted\n", info->name);
> -		goto free_copy;
> -	}
> -
> -	err = rewrite_section_headers(info, flags);
> +	err = early_mod_check(info, flags);
>  	if (err)
>  		goto free_copy;
>  
> -	/* Check module struct version now, before we try to use module. */
> -	if (!check_modstruct_version(info, info->mod)) {
> -		err = -ENOEXEC;

Original value here.

> -		goto free_copy;
> -	}
> -
>  	/* Figure out module layout, and allocate all the memory. */
>  	mod = layout_and_allocate(info, flags);
>  	if (IS_ERR(mod)) {

Thanks,
Petr

Re: [PATCH 04/12] module: move early sanity checks into a helper

[Luis Chamberlain]

On Fri, Mar 24, 2023 at 02:02:06PM +0100, Petr Pavlu wrote:
> On 3/19/23 22:27, Luis Chamberlain wrote:
> > Move early sanity checkers for the module into a helper.
> > This let's us make it clear when we are working with the
> > local copy of the module prior to allocation.
> > 
> > This produces no functional changes, it just makes subsequent
> > changes easier to read.
> > 
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >  kernel/module/main.c | 43 ++++++++++++++++++++++++++-----------------
> >  1 file changed, 26 insertions(+), 17 deletions(-)
> > 
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index 427284ab31f1..933cef72ae13 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -2668,6 +2668,31 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname,
> >  	return 0;
> >  }
> >  
> > +/* Module within temporary copy, this doesn't do any allocation  */
> > +static int early_mod_check(struct load_info *info, int flags)
> > +{
> > +	int err;
> > +
> > +	/*
> > +	 * Now that we know we have the correct module name, check
> > +	 * if it's blacklisted.
> > +	 */
> > +	if (blacklisted(info->name)) {
> > +		pr_err("Module %s is blacklisted\n", info->name);
> > +		return -EPERM;
> > +	}
> > +
> > +	err = rewrite_section_headers(info, flags);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Check module struct version now, before we try to use module. */
> > +	if (!check_modstruct_version(info, info->mod))
> > +		return ENOEXEC;
> 
> The error value when check_modstruct_version() fails is changed in this patch
> from -ENOEXEC to ENOEXEC and updated back again in the next patch. It would be
> good to avoid introducing this temporary problem and keep the value throughout
> as -ENOEXEC.

Fixed, thanks.

  Luis