| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 21 Apr 2017 15:32:50 -0700
From: Kees Cook <keescook@...omium.org>
To: Jessica Yu <jeyu@...hat.com>
Cc: Rusty Russell <rusty@...tcorp.com.au>,
Nicholas Piggin <npiggin@...il.com>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Heinrich Schuchardt <xypron.glpk@....de>,
"Peter Zijlstra (Intel)" <peterz@...radead.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Chris Metcalf <cmetcalf@...lanox.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] module: Add module name to modinfo
On Thu, Apr 20, 2017 at 11:27 PM, Jessica Yu <jeyu@...hat.com> 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 <keescook@...omium.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?
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 <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
>>
>
Thanks for the review!
-Kees
--
Kees Cook
Pixel Security
Powered by blists - more mailing lists