Messages in this thread Patch in this message | | | Date | Thu, 20 Apr 2017 23:27:46 -0700 | From | Jessica Yu <> | Subject | Re: [PATCH 2/2] module: Add module name to modinfo |
| |
+++ Kees Cook [12/04/17 16:16 -0700]: >Accessing the mod structure (e.g. for mod->name) prior to having completed >check_modstruct_version() can result in writing garbage to the error logs >if the layout of the mod structure loaded from disk doesn't match the >running kernel's mod structure layout. This kind of mismatch will become >much more likely if a kernel is built with different randomization seed >for the struct layout randomization plugin. > >Instead, add and use a new modinfo string for logging the module name. > >Signed-off-by: Kees Cook <keescook@chromium.org> >--- > kernel/module.c | 21 ++++++++++++++------- > scripts/mod/modpost.c | 1 + > 2 files changed, 15 insertions(+), 7 deletions(-) > >diff --git a/kernel/module.c b/kernel/module.c >index ed6cf2367217..ed4a399174ba 100644 >--- a/kernel/module.c >+++ b/kernel/module.c >@@ -299,6 +299,7 @@ int unregister_module_notifier(struct notifier_block *nb) > EXPORT_SYMBOL(unregister_module_notifier); > > struct load_info { >+ char *name; > Elf_Ehdr *hdr; > unsigned long len; > Elf_Shdr *sechdrs; >@@ -1297,12 +1298,12 @@ static int check_version(const struct load_info *info, > } > > /* Broken toolchain. Warn once, then let it go.. */ >- pr_warn_once("%s: no symbol version for %s\n", mod->name, symname); >+ pr_warn_once("%s: no symbol version for %s\n", info->name, symname); > return 1; > > bad_version: > pr_warn("%s: disagrees about version of symbol %s\n", >- mod->name, symname); >+ info->name, symname); > return 0; > } > >@@ -2892,9 +2893,15 @@ static int rewrite_section_headers(struct load_info *info, int flags) > 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 = "unknown"; >+ else >+ info->name = get_modinfo(info, "name");
Hm, if we attempt to load an old module without the new name field (i.e. all modules currently), we'll end up printing "(null)" for all the prints that use info->name. Maybe we can fall back to mod->name if the name field isn't there and the kernel wasn't compiled with the randstruct plugin?
> info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC; >- info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC; >+ > return 0; > } > >@@ -2934,14 +2941,14 @@ static struct module *setup_load_info(struct load_info *info, int flags) > > info->index.mod = find_sec(info, ".gnu.linkonce.this_module"); > if (!info->index.mod) { >- pr_warn("No module found in object\n"); >+ pr_warn("%s: No module found in object\n", info->name); > return ERR_PTR(-ENOEXEC); > } > /* This is temporary: point mod into copy of data. */ > mod = (void *)info->sechdrs[info->index.mod].sh_addr; > > if (info->index.sym == 0) { >- pr_warn("%s: module has no symbols (stripped?)\n", mod->name); >+ pr_warn("%s: module has no symbols (stripped?)\n", info->name); > return ERR_PTR(-ENOEXEC); > } > >@@ -2969,7 +2976,7 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags) > return err; > } else if (!same_magic(modmagic, vermagic, info->index.vers)) { > pr_err("%s: version magic '%s' should be '%s'\n", >- mod->name, modmagic, vermagic); >+ info->name, modmagic, vermagic); > return -ENOEXEC; > } >
Maybe we want to use info->name for the blacklisted() check as well (see layout_and_allocate()).
>@@ -3622,7 +3629,7 @@ static int load_module(struct load_info *info, const char __user *uargs, > if (!mod->sig_ok) { > pr_notice_once("%s: module verification failed: signature " > "and/or required key missing - tainting " >- "kernel\n", mod->name); >+ "kernel\n", info->name); > add_taint_module(mod, TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK); > }
Do we still need to use info->name here? At this point we're done with layout_and_allocate(), plus the modstruct check and vermagic check, and mod should be pointing to its new place in memory.
> #endif >diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c >index 30d752a4a6a6..48397feb08fb 100644 >--- a/scripts/mod/modpost.c >+++ b/scripts/mod/modpost.c >@@ -2126,6 +2126,7 @@ static void add_header(struct buffer *b, struct module *mod) > buf_printf(b, "#include <linux/compiler.h>\n"); > buf_printf(b, "\n"); > buf_printf(b, "MODULE_INFO(vermagic, VERMAGIC_STRING);\n"); >+ buf_printf(b, "MODULE_INFO(name, KBUILD_MODNAME);\n"); > buf_printf(b, "\n"); > buf_printf(b, "__visible struct module __this_module\n"); > buf_printf(b, "__attribute__((section(\".gnu.linkonce.this_module\"))) = {\n"); >-- >2.7.4 >
| |