[PATCH 0/3] lockdown/module: make module name available for module_sig_check()

[PATCH 0/3] lockdown/module: make module name available for module_sig_check()

[Jessica Yu]

Hi David,

The changes here involve cleaning up load_module() (patches 1 and 2) in
preparation for patch 3. The general idea is to do some preliminary module
section parsing and set up load info convenience variables earlier so that
we could log the module name during the module signature verification check
if it fails. Right now the module name is not logged if signature
verification fails, and it would be helpful to know which module failed
loading.

Currently, all patches are based on the lockdown tree:

    http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=lockdown

But my plan is probably to take patches 1 and 2 through the modules-next
tree as they are generic cleanups, but I wanted to give you a heads up for
patch 3, which should probably be taken through the lockdown tree.

Thanks!

Jessica

---
Jessica Yu (3):
  module: make it clear when we're handling the module copy in info->hdr
  module: setup load info before module_sig_check()
  modsign: print module name along with error message

 kernel/module.c | 105 ++++++++++++++++++++++++++++++--------------------------
 1 file changed, 57 insertions(+), 48 deletions(-)

-- 
2.16.3

[PATCH 1/3] module: make it clear when we're handling the module copy in info->hdr

[Jessica Yu]

In load_module(), it's not always clear whether we're handling the
temporary module copy in info->hdr (which is freed at the end of
load_module()) or if we're handling the module already allocated and
copied to it's final place. Adding an info->mod field and using it
whenever we're handling the temporary copy makes that explicitly clear.

Signed-off-by: Jessica Yu <jeyu@kernel.org>
---
 kernel/module.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 9c1709a05037..e8eba00bfed7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -312,6 +312,8 @@ EXPORT_SYMBOL(unregister_module_notifier);
 
 struct load_info {
 	const char *name;
+	/* pointer to module in temporary copy, freed at end of load_module() */
+	struct module *mod;
 	Elf_Ehdr *hdr;
 	unsigned long len;
 	Elf_Shdr *sechdrs;
@@ -2979,14 +2981,13 @@ static int rewrite_section_headers(struct load_info *info, int flags)
  * search for module section index etc), and do some basic section
  * verification.
  *
- * Return the temporary module pointer (we'll replace it with the final
- * one when we move the module sections around).
+ * Set info->mod to the temporary copy of the module in info->hdr. The final one
+ * will be allocated in move_module().
  */
-static struct module *setup_load_info(struct load_info *info, int flags)
+static int setup_load_info(struct load_info *info, int flags)
 {
 	unsigned int i;
 	int err;
-	struct module *mod;
 
 	/* Set up the convenience variables */
 	info->sechdrs = (void *)info->hdr + info->hdr->e_shoff;
@@ -2995,7 +2996,7 @@ static struct module *setup_load_info(struct load_info *info, int flags)
 
 	err = rewrite_section_headers(info, flags);
 	if (err)
-		return ERR_PTR(err);
+		return err;
 
 	/* Find internal symbols and strings. */
 	for (i = 1; i < info->hdr->e_shnum; i++) {
@@ -3012,30 +3013,30 @@ static struct module *setup_load_info(struct load_info *info, int flags)
 	if (!info->index.mod) {
 		pr_warn("%s: No module found in object\n",
 			info->name ?: "(missing .modinfo name field)");
-		return ERR_PTR(-ENOEXEC);
+		return -ENOEXEC;
 	}
 	/* This is temporary: point mod into copy of data. */
-	mod = (void *)info->sechdrs[info->index.mod].sh_addr;
+	info->mod = (void *)info->sechdrs[info->index.mod].sh_addr;
 
 	/*
 	 * If we didn't load the .modinfo 'name' field, fall back to
 	 * on-disk struct mod 'name' field.
 	 */
 	if (!info->name)
-		info->name = mod->name;
+		info->name = info->mod->name;
 
 	if (info->index.sym == 0) {
 		pr_warn("%s: module has no symbols (stripped?)\n", info->name);
-		return ERR_PTR(-ENOEXEC);
+		return -ENOEXEC;
 	}
 
 	info->index.pcpu = find_pcpusec(info);
 
 	/* Check module struct version now, before we try to use module. */
-	if (!check_modstruct_version(info, mod))
-		return ERR_PTR(-ENOEXEC);
+	if (!check_modstruct_version(info, info->mod))
+		return -ENOEXEC;
 
-	return mod;
+	return 0;
 }
 
 static int check_modinfo(struct module *mod, struct load_info *info, int flags)
@@ -3330,25 +3331,24 @@ core_param(module_blacklist, module_blacklist, charp, 0400);
 
 static struct module *layout_and_allocate(struct load_info *info, int flags)
 {
-	/* Module within temporary copy. */
 	struct module *mod;
 	unsigned int ndx;
 	int err;
 
-	mod = setup_load_info(info, flags);
-	if (IS_ERR(mod))
-		return mod;
+	err = setup_load_info(info, flags);
+	if (err)
+		return ERR_PTR(err);
 
 	if (blacklisted(info->name))
 		return ERR_PTR(-EPERM);
 
-	err = check_modinfo(mod, info, flags);
+	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, mod);
+					info->secstrings, info->mod);
 	if (err < 0)
 		return ERR_PTR(err);
 
@@ -3367,11 +3367,11 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	/* Determine total sizes, and put offsets in sh_entsize.  For now
 	   this is done generically; there doesn't appear to be any
 	   special cases for the architectures. */
-	layout_sections(mod, info);
-	layout_symtab(mod, info);
+	layout_sections(info->mod, info);
+	layout_symtab(info->mod, info);
 
 	/* Allocate and move to the final place */
-	err = move_module(mod, info);
+	err = move_module(info->mod, info);
 	if (err)
 		return ERR_PTR(err);
 
-- 
2.16.3

[PATCH 2/3] module: setup load info before module_sig_check()

[Jessica Yu]

We want to be able to log the module name in early error messages, such as
when module signature verification fails.  Previously, the module name is
set in layout_and_allocate(), meaning that any error messages that happen
before (such as those in module_sig_check()) won't be logged with a module
name, which isn't terribly helpful.

In order to do this, reshuffle the order in load_module() and set up
load info earlier so that we can log the module name along with these
error messages. This requires splitting rewrite_section_headers() out of
setup_load_info().

While we're at it, clean up and split up the operations done in
layout_and_allocate(), setup_load_info(), and rewrite_section_headers()
more cleanly so these functions only perform what their names suggest.

Signed-off-by: Jessica Yu <jeyu@kernel.org>
---
 kernel/module.c | 77 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index e8eba00bfed7..ae824a6f1a03 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2491,7 +2491,11 @@ static char *get_modinfo(struct load_info *info, const char *tag)
 	Elf_Shdr *infosec = &info->sechdrs[info->index.info];
 	unsigned long size = infosec->sh_size;
 
-	for (p = (char *)infosec->sh_addr; p; p = next_string(p, &size)) {
+	/*
+	 * get_modinfo() calls made before rewrite_section_headers()
+	 * must use sh_offset, as sh_addr isn't set!
+	 */
+	for (p = (char *)info->hdr + infosec->sh_offset; p; p = next_string(p, &size)) {
 		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
 			return p + taglen + 1;
 	}
@@ -2960,17 +2964,7 @@ static int rewrite_section_headers(struct load_info *info, int flags)
 	}
 
 	/* Track but don't keep modinfo and version sections. */
-	if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
-		info->index.vers = 0; /* Pretend no __versions section! */
-	else
-		info->index.vers = find_sec(info, "__versions");
 	info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC;
-
-	info->index.info = find_sec(info, ".modinfo");
-	if (!info->index.info)
-		info->name = "(missing .modinfo section)";
-	else
-		info->name = get_modinfo(info, "name");
 	info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC;
 
 	return 0;
@@ -2987,16 +2981,18 @@ 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;
-	int err;
 
 	/* Set up the convenience variables */
 	info->sechdrs = (void *)info->hdr + info->hdr->e_shoff;
 	info->secstrings = (void *)info->hdr
 		+ info->sechdrs[info->hdr->e_shstrndx].sh_offset;
 
-	err = rewrite_section_headers(info, flags);
-	if (err)
-		return err;
+	/* 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 = "(missing .modinfo section)";
+	else
+		info->name = get_modinfo(info, "name");
 
 	/* Find internal symbols and strings. */
 	for (i = 1; i < info->hdr->e_shnum; i++) {
@@ -3009,6 +3005,11 @@ static int setup_load_info(struct load_info *info, int flags)
 		}
 	}
 
+	if (info->index.sym == 0) {
+		pr_warn("%s: module has no symbols (stripped?)\n", info->name);
+		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",
@@ -3016,26 +3017,22 @@ static int setup_load_info(struct load_info *info, int flags)
 		return -ENOEXEC;
 	}
 	/* This is temporary: point mod into copy of data. */
-	info->mod = (void *)info->sechdrs[info->index.mod].sh_addr;
+	info->mod = (void *)info->hdr + info->sechdrs[info->index.mod].sh_offset;
 
 	/*
-	 * If we didn't load the .modinfo 'name' field, fall back to
+	 * 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 (info->index.sym == 0) {
-		pr_warn("%s: module has no symbols (stripped?)\n", info->name);
-		return -ENOEXEC;
-	}
+	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);
 
-	/* Check module struct version now, before we try to use module. */
-	if (!check_modstruct_version(info, info->mod))
-		return -ENOEXEC;
-
 	return 0;
 }
 
@@ -3335,13 +3332,6 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	unsigned int ndx;
 	int err;
 
-	err = setup_load_info(info, flags);
-	if (err)
-		return ERR_PTR(err);
-
-	if (blacklisted(info->name))
-		return ERR_PTR(-EPERM);
-
 	err = check_modinfo(info->mod, info, flags);
 	if (err)
 		return ERR_PTR(err);
@@ -3684,17 +3674,36 @@ static int load_module(struct load_info *info, const char __user *uargs,
 		       int flags, bool can_do_ima_check)
 {
 	struct module *mod;
-	long err;
+	long err = 0;
 	char *after_dashes;
 
+	err = elf_header_check(info);
+	if (err)
+		goto free_copy;
+
+	err = setup_load_info(info, flags);
+	if (err)
+		goto free_copy;
+
+	if (blacklisted(info->name)) {
+		err = -EPERM;
+		goto free_copy;
+	}
+
 	err = module_sig_check(info, flags, can_do_ima_check);
 	if (err)
 		goto free_copy;
 
-	err = elf_header_check(info);
+	err = rewrite_section_headers(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.16.3

[PATCH 3/3] modsign: print module name along with error message

[Jessica Yu]

It is useful to know which module failed signature verification, so
print the module name along with the error message.

Signed-off-by: Jessica Yu <jeyu@kernel.org>
---
 kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index ae824a6f1a03..8670585eb5f7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2807,7 +2807,7 @@ static int module_sig_check(struct load_info *info, int flags,
 		reason = "Loading of module with unavailable key";
 	decide:
 		if (sig_enforce) {
-			pr_notice("%s is rejected\n", reason);
+			pr_notice("%s: %s is rejected\n", info->name, reason);
 			return -EKEYREJECTED;
 		}
 
-- 
2.16.3

Re: [PATCH 3/3] modsign: print module name along with error message

[Jessica Yu]

+++ Jessica Yu [30/05/18 11:08 +0200]:
>It is useful to know which module failed signature verification, so
>print the module name along with the error message.
>
>Signed-off-by: Jessica Yu <jeyu@kernel.org>
>---
> kernel/module.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index ae824a6f1a03..8670585eb5f7 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2807,7 +2807,7 @@ static int module_sig_check(struct load_info *info, int flags,
> 		reason = "Loading of module with unavailable key";
> 	decide:
> 		if (sig_enforce) {
>-			pr_notice("%s is rejected\n", reason);
>+			pr_notice("%s: %s is rejected\n", info->name, reason);
> 			return -EKEYREJECTED;
> 		}
>

Hi David!

I've just merged patches 1 and 2 into modules-next. Would you be
interested this patch (patch 3) if/when you push lockdown upstream
again?

Thanks!

Jessica