linux-kernel - Re: [PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 17 Apr 2021 18:52:34 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Jessica Yu <jeyu@...nel.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Jiri Olsa <jolsa@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Evan Green <evgreen@...omium.org>,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        linux-doc@...r.kernel.org, Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces

Quoting Jessica Yu (2021-04-15 06:04:35)
> +++ Stephen Boyd [09/04/21 18:52 -0700]:
> >diff --git a/include/linux/module.h b/include/linux/module.h
> >index 59f094fa6f74..4bf869f6c944 100644
> >--- a/include/linux/module.h
> >+++ b/include/linux/module.h
> >@@ -11,6 +11,7 @@
> >
> > #include <linux/list.h>
> > #include <linux/stat.h>
> >+#include <linux/buildid.h>
> > #include <linux/compiler.h>
> > #include <linux/cache.h>
> > #include <linux/kmod.h>
> >@@ -367,6 +368,9 @@ struct module {
> >       /* Unique handle for this module */
> >       char name[MODULE_NAME_LEN];
> >
> >+      /* Module build ID */
> >+      unsigned char build_id[BUILD_ID_SIZE_MAX];
> 
> Hi Stephen,
> 
> Since this field is not used when !CONFIG_STACKTRACE_BUILD_ID, I
> would prefer to wrap this in an ifdef, similar to the other
> CONFIG-dependent fields in struct module. This makes it explicit under
> what conditions (i.e. config) the field is meant to be used.

Ok will do.

> >diff --git a/kernel/module.c b/kernel/module.c
> >index 30479355ab85..6f5bc1b046a5 100644
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -2770,6 +2771,20 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
> >       }
> >       mod->core_kallsyms.num_symtab = ndst;
> > }
> >+
> >+static void init_build_id(struct module *mod, const struct load_info *info)
> >+{
> >+      const Elf_Shdr *sechdr;
> >+      unsigned int i;
> >+
> >+      for (i = 0; i < info->hdr->e_shnum; i++) {
> >+              sechdr = &info->sechdrs[i];
> >+              if (!sect_empty(sechdr) && sechdr->sh_type == SHT_NOTE &&
> >+                  !build_id_parse_buf((void *)sechdr->sh_addr, mod->build_id,
> >+                                      sechdr->sh_size))
> >+                      break;
> >+      }
> 
> If mod->build_id is not used when !CONFIG_STACKTRACE_BUILD_ID, then we
> don't need to look for it. I would be fine with wrapping the function
> body in an ifdef (similar to what we currently do in
> del_usage_links() and do_mod_ctors()).

Ok, done.

> 
> >+}
> > #else
> > static inline void layout_symtab(struct module *mod, struct load_info *info)
> > {
> >@@ -2778,6 +2793,10 @@ static inline void layout_symtab(struct module *mod, struct load_info *info)
> > static void add_kallsyms(struct module *mod, const struct load_info *info)
> > {
> > }
> >+
> >+static void init_build_id(struct module *mod, const struct load_info *info)
> >+{
> >+}
> > #endif /* CONFIG_KALLSYMS */
> >
> > static void dynamic_debug_setup(struct module *mod, struct _ddebug *debug, unsigned int num)
> >@@ -4004,6 +4023,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >               goto free_arch_cleanup;
> >       }
> >
> >+      init_build_id(mod, info);
> >       dynamic_debug_setup(mod, info->debug, info->num_debug);
> >
> >       /* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
> >@@ -4235,7 +4255,7 @@ void * __weak dereference_module_function_descriptor(struct module *mod,
> > const char *module_address_lookup(unsigned long addr,
> >                           unsigned long *size,
> >                           unsigned long *offset,
> >-                          char **modname,
> >+                          char **modname, const unsigned char **modbuildid,
> >                           char *namebuf)
> > {
> >       const char *ret = NULL;
> >@@ -4246,6 +4266,8 @@ const char *module_address_lookup(unsigned long addr,
> >       if (mod) {
> >               if (modname)
> >                       *modname = mod->name;
> >+              if (modbuildid)
> >+                      *modbuildid = mod->build_id;
> 
> Then maybe we can set *modbuildid = NULL in the case of
> !CONFIG_STACKTRACE_BUILD_ID, similar to the kernel case in
> kallsyms_lookup_buildid().
> 

Sounds good. It means that some more ifdefs are probably required vs.
making the array size be 0 when the config is disabled but that isn't a
big problem for me. I'm reworking the code now and will test and then
send v5 shortly. Thanks!