lockdown/module: make module name available for module_sig_check()

PATCH In review Jessica Yu 1 participant 5 messages May 30, 2018 – Jun 22, 2018

lore

Share & tools

lore

Jessica Yu

author

2018-05-30 09:09 UTC #0 lore raw

cover lockdown/module: make module name available for module_sig_check()

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

Signature · 1 line

2.16.3

Jessica Yu

author

2018-05-30 09:09 UTC #1 lore raw

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

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 <[email protected]>

---
 kernel/module.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

kernel/module.c +21 ~~-21~~

index 9c1709a05037..e8eba00bfed7 100644
@@ -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);

Signature · 1 line

2.16.3

Jessica Yu

author

2018-05-30 09:09 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/module.c | 77 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 34 deletions(-)

kernel/module.c +43 ~~-34~~

index e8eba00bfed7..ae824a6f1a03 100644
@@ -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)) {

Signature · 1 line

2.16.3

Jessica Yu

author

2018-05-30 09:09 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/module.c +1 -1

index ae824a6f1a03..8670585eb5f7 100644
@@ -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;
 		}

Signature · 1 line

2.16.3

Jessica Yu

author

2018-06-22 15:28 UTC #4 lore raw

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

+++ Jessica Yu [30/05/18 11:08 +0200]:

Quoted 13 lines

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 <[email protected]>
---
kernel/module.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
kernel/module.c +0 -0
index ae824a6f1a03..8670585eb5f7 100644

kernel/module.c static int module_sig_check(struct load_info *info, int flags, · line 2813

Quoted 8 lines

kernel/module.c +1 -1

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