| 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: Mon, 24 Oct 2016 15:16:54 +0100
From: Ard Biesheuvel <ard.biesheuvel@...aro.org>
To: Kevin Cernekee <cernekee@...omium.org>
Cc: Rusty Russell <rusty@...tcorp.com.au>,
Andrew Morton <akpm@...ux-foundation.org>,
Kees Cook <keescook@...omium.org>,
Brian Norris <computersforpeace@...il.com>,
Doug Anderson <dianders@...omium.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] printk: Make %pS print offsets into modules
Hi Kevin,
On 23 October 2016 at 22:21, Kevin Cernekee <cernekee@...omium.org> wrote:
> If kallsyms cannot find a symbol for an address, entries like this will
> appear in backtraces:
>
> Call trace:
> [<ffffffbffc1ecd7c>] 0xffffffbffc1ecd7c
> [<ffffffbffc1ef7f0>] 0xffffffbffc1ef7f0
> [<ffffffbffc1f0094>] 0xffffffbffc1f0094
>
> This isn't particularly useful for debugging because modules are not
> loaded at fixed addresses. Instead, print the offset from the module's
> base, so that the offending location can be easily located in a
> disassembly of the .ko file:
>
> Call trace:
> [<ffffffbffc1d57f4>] [mwifiex_pcie+0x37f4/0x9000]
> [<ffffffbffc1d60ac>] [mwifiex_pcie+0x40ac/0x9000]
> [<ffffffbffc188fe0>] mwifiex_main_process+0xdc/0x6fc [mwifiex]
>
This looks like a useful feature, although this example is a bit
misleading, since it suggests that only some symbols are unresolveable
in some cases, whereas the primary use case for this facility imo is a
kernel with no kallsyms to begin with.
> Signed-off-by: Kevin Cernekee <cernekee@...omium.org>
> ---
> include/linux/module.h | 16 ++++++++++++++++
> kernel/kallsyms.c | 19 ++++++++++++++++++-
> kernel/module.c | 23 +++++++++++++++++++++++
> 3 files changed, 57 insertions(+), 1 deletion(-)
>
>
> Tested on 4.4 only (so the core_layout / init_layout stuff is untested).
>
Please don't send untested stuff. v4.4 is ancient history for many of
us, so this needs to be tested and working against mainline or -next
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 0c3207d26ac0..611d1e71b7c8 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -616,6 +616,14 @@ const char *module_address_lookup(unsigned long addr,
> unsigned long *offset,
> char **modname,
> char *namebuf);
> +
> +/* For kallsyms to ask which module, if any, contains addr. On success,
> + * returns true and populates module_size, module_offset, and modname. */
> +bool module_base_lookup(unsigned long addr,
> + unsigned long *module_size,
> + unsigned long *module_offset,
> + char **modname);
> +
> int lookup_module_symbol_name(unsigned long addr, char *symname);
> int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
>
> @@ -698,6 +706,14 @@ static inline const char *module_address_lookup(unsigned long addr,
> return NULL;
> }
>
> +static inline bool module_base_lookup(unsigned long addr,
> + unsigned long *module_size,
> + unsigned long *module_offset,
> + char **modname)
> +{
> + return false;
> +}
> +
> static inline int lookup_module_symbol_name(unsigned long addr, char *symname)
> {
> return -ERANGE;
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index fafd1a3ef0da..43b114e6861f 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -387,8 +387,25 @@ static int __sprint_symbol(char *buffer, unsigned long address,
>
> address += symbol_offset;
> name = kallsyms_lookup(address, &size, &offset, &modname, buffer);
> - if (!name)
> + if (!name) {
> + /*
> + * Fall back to [module_base+offset/size] if the actual
> + * symbol name is unavailable.
> + */
> + if (module_base_lookup(address, &size, &offset, &modname)) {
> + if (add_offset) {
> + return snprintf(buffer, KSYM_SYMBOL_LEN,
> + "[%s+%#lx/%#lx]", modname,
> + address - offset -
> + symbol_offset,
> + size);
> + } else {
> + return snprintf(buffer, KSYM_SYMBOL_LEN,
> + "[%s]", modname);
> + }
> + }
> return sprintf(buffer, "0x%lx", address - symbol_offset);
> + }
>
> if (name != buffer)
> strcpy(buffer, name);
> diff --git a/kernel/module.c b/kernel/module.c
> index f57dd63186e6..5e09f568c601 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3899,6 +3899,29 @@ const char *module_address_lookup(unsigned long addr,
> return ret;
> }
>
> +bool module_base_lookup(unsigned long addr,
> + unsigned long *module_size,
> + unsigned long *module_offset,
> + char **modname)
> +{
> + struct module *mod;
> +
> + preempt_disable();
> +
> + mod = __module_address(addr);
> + if (!mod) {
> + preempt_enable();
> + return false;
> + }
> +
> + *modname = mod->name;
I'm aware that this pattern may exist elsewhere, but I don't see how
it is safe to dereference modname after reenabling preemption. If mod
gets freed inadvertently, so will mod->name.
> + *module_offset = (unsigned long)mod->core_layout.base;
> + *module_size = mod->init_layout.size + mod->core_layout.size;
> +
> + preempt_enable();
> + return true;
> +}
> +
> int lookup_module_symbol_name(unsigned long addr, char *symname)
> {
> struct module *mod;
> --
> 2.8.0.rc3.226.g39d4020
>
Powered by blists - more mailing lists