DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com EA6D778810
Date: Thu, 20 Apr 2017 23:27:46 -0700
From: Jessica Yu <jeyu@redhat.com>
To: Kees Cook <keescook@chromium.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>,
        Nicholas Piggin <npiggin@gmail.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Heinrich Schuchardt <xypron.glpk@gmx.de>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        Chris Metcalf <cmetcalf@mellanox.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] module: Add module name to modinfo
Message-ID: <20170421062744.alfsijkvkhvgg3im@jeyu>
References: <1492038989-82136-1-git-send-email-keescook@chromium.org>
 <1492038989-82136-3-git-send-email-keescook@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <1492038989-82136-3-git-send-email-keescook@chromium.org>
User-Agent: NeoMutt/20161126 (1.7.1)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4763
Lines: 126

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