Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1042807AbdDUWcy (ORCPT ); Fri, 21 Apr 2017 18:32:54 -0400 Received: from mail-io0-f175.google.com ([209.85.223.175]:33957 "EHLO mail-io0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1042715AbdDUWcv (ORCPT ); Fri, 21 Apr 2017 18:32:51 -0400 MIME-Version: 1.0 In-Reply-To: <20170421062744.alfsijkvkhvgg3im@jeyu> References: <1492038989-82136-1-git-send-email-keescook@chromium.org> <1492038989-82136-3-git-send-email-keescook@chromium.org> <20170421062744.alfsijkvkhvgg3im@jeyu> From: Kees Cook Date: Fri, 21 Apr 2017 15:32:50 -0700 X-Google-Sender-Auth: pyE3nmsZHMnpDhom_RtlAnCs7vY Message-ID: Subject: Re: [PATCH 2/2] module: Add module name to modinfo To: Jessica Yu Cc: Rusty Russell , Nicholas Piggin , Josh Poimboeuf , Heinrich Schuchardt , "Peter Zijlstra (Intel)" , Andrew Morton , Ard Biesheuvel , Chris Metcalf , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5802 Lines: 166 On Thu, Apr 20, 2017 at 11:27 PM, Jessica Yu wrote: > +++ 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 >> --- >> 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? Ah yes, excellent point. I'll fix this. > >> 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()). Eek! Yes, good catch. > >> @@ -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. Ah, you're right. I'll drop this. > > >> #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 \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 >> > Thanks for the review! -Kees -- Kees Cook Pixel Security