* [PATCH 0/2] module: Add module name to modinfo
@ 2017-04-12 23:16 Kees Cook
2017-04-12 23:16 ` [PATCH 1/2] module: Pass struct load_info into symbol checks Kees Cook
2017-04-12 23:16 ` [PATCH 2/2] module: Add module name to modinfo Kees Cook
0 siblings, 2 replies; 5+ messages in thread
From: Kees Cook @ 2017-04-12 23:16 UTC (permalink / raw)
To: Jessica Yu
Cc: Kees Cook, Rusty Russell, Nicholas Piggin, Josh Poimboeuf,
Heinrich Schuchardt, Peter Zijlstra (Intel),
Andrew Morton, Ard Biesheuvel, Chris Metcalf, linux-kernel
The mod structure is accessed for the "name" field prior to validating
sanity in check_modstruct_version(). This becomes very obvious once
struct layout randomization is happening, so instead add the module
name to modinfo and use that until the mod struct has been sanity-checked.
-Kees
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] module: Pass struct load_info into symbol checks
2017-04-12 23:16 [PATCH 0/2] module: Add module name to modinfo Kees Cook
@ 2017-04-12 23:16 ` Kees Cook
2017-04-12 23:16 ` [PATCH 2/2] module: Add module name to modinfo Kees Cook
1 sibling, 0 replies; 5+ messages in thread
From: Kees Cook @ 2017-04-12 23:16 UTC (permalink / raw)
To: Jessica Yu
Cc: Kees Cook, Rusty Russell, Nicholas Piggin, Josh Poimboeuf,
Heinrich Schuchardt, Peter Zijlstra (Intel),
Andrew Morton, Ard Biesheuvel, Chris Metcalf, linux-kernel
Since we're already using values from struct load_info, just pass this
pointer in directly and use what's needed as we need it. This allows us
to access future fields in struct load_info too.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
kernel/module.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index 7eba6dea4f41..ed6cf2367217 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1257,12 +1257,13 @@ static u32 resolve_rel_crc(const s32 *crc)
return *(u32 *)((void *)crc + *crc);
}
-static int check_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static int check_version(const struct load_info *info,
const char *symname,
struct module *mod,
const s32 *crc)
{
+ Elf_Shdr *sechdrs = info->sechdrs;
+ unsigned int versindex = info->index.vers;
unsigned int i, num_versions;
struct modversion_info *versions;
@@ -1305,8 +1306,7 @@ static int check_version(Elf_Shdr *sechdrs,
return 0;
}
-static inline int check_modstruct_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static inline int check_modstruct_version(const struct load_info *info,
struct module *mod)
{
const s32 *crc;
@@ -1322,8 +1322,8 @@ static inline int check_modstruct_version(Elf_Shdr *sechdrs,
BUG();
}
preempt_enable();
- return check_version(sechdrs, versindex,
- VMLINUX_SYMBOL_STR(module_layout), mod, crc);
+ return check_version(info, VMLINUX_SYMBOL_STR(module_layout),
+ mod, crc);
}
/* First part is kernel version, which we ignore if module has crcs. */
@@ -1337,8 +1337,7 @@ static inline int same_magic(const char *amagic, const char *bmagic,
return strcmp(amagic, bmagic) == 0;
}
#else
-static inline int check_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static inline int check_version(const struct load_info *info,
const char *symname,
struct module *mod,
const s32 *crc)
@@ -1346,8 +1345,7 @@ static inline int check_version(Elf_Shdr *sechdrs,
return 1;
}
-static inline int check_modstruct_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static inline int check_modstruct_version(const struct load_info *info,
struct module *mod)
{
return 1;
@@ -1383,7 +1381,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
if (!sym)
goto unlock;
- if (!check_version(info->sechdrs, info->index.vers, name, mod, crc)) {
+ if (!check_version(info, name, mod, crc)) {
sym = ERR_PTR(-EINVAL);
goto getname;
}
@@ -2950,7 +2948,7 @@ static struct module *setup_load_info(struct load_info *info, int flags)
info->index.pcpu = find_pcpusec(info);
/* Check module struct version now, before we try to use module. */
- if (!check_modstruct_version(info->sechdrs, info->index.vers, mod))
+ if (!check_modstruct_version(info, mod))
return ERR_PTR(-ENOEXEC);
return mod;
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] module: Add module name to modinfo
2017-04-12 23:16 [PATCH 0/2] module: Add module name to modinfo Kees Cook
2017-04-12 23:16 ` [PATCH 1/2] module: Pass struct load_info into symbol checks Kees Cook
@ 2017-04-12 23:16 ` Kees Cook
2017-04-21 6:27 ` Jessica Yu
1 sibling, 1 reply; 5+ messages in thread
From: Kees Cook @ 2017-04-12 23:16 UTC (permalink / raw)
To: Jessica Yu
Cc: Kees Cook, Rusty Russell, Nicholas Piggin, Josh Poimboeuf,
Heinrich Schuchardt, Peter Zijlstra (Intel),
Andrew Morton, Ard Biesheuvel, Chris Metcalf, linux-kernel
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");
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;
}
@@ -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);
}
#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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] module: Add module name to modinfo
2017-04-12 23:16 ` [PATCH 2/2] module: Add module name to modinfo Kees Cook
@ 2017-04-21 6:27 ` Jessica Yu
2017-04-21 22:32 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Jessica Yu @ 2017-04-21 6:27 UTC (permalink / raw)
To: Kees Cook
Cc: Rusty Russell, Nicholas Piggin, Josh Poimboeuf,
Heinrich Schuchardt, Peter Zijlstra (Intel),
Andrew Morton, Ard Biesheuvel, Chris Metcalf, linux-kernel
+++ 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] module: Add module name to modinfo
2017-04-21 6:27 ` Jessica Yu
@ 2017-04-21 22:32 ` Kees Cook
0 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2017-04-21 22:32 UTC (permalink / raw)
To: Jessica Yu
Cc: Rusty Russell, Nicholas Piggin, Josh Poimboeuf,
Heinrich Schuchardt, Peter Zijlstra (Intel),
Andrew Morton, Ard Biesheuvel, Chris Metcalf, LKML
On Thu, Apr 20, 2017 at 11:27 PM, Jessica Yu <jeyu@redhat.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@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?
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-21 22:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-12 23:16 [PATCH 0/2] module: Add module name to modinfo Kees Cook
2017-04-12 23:16 ` [PATCH 1/2] module: Pass struct load_info into symbol checks Kees Cook
2017-04-12 23:16 ` [PATCH 2/2] module: Add module name to modinfo Kees Cook
2017-04-21 6:27 ` Jessica Yu
2017-04-21 22:32 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).