linux-kernel - Re: [PATCH v2 02/12] buildid: Add method to get running kernel's build ID

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]

Message-ID: <161661277766.3012082.5402015164122526850@swboyd.mtv.corp.google.com>
Date:   Wed, 24 Mar 2021 12:06:17 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     linux-kernel@...r.kernel.org, Jiri Olsa <jolsa@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Jessica Yu <jeyu@...nel.org>,
        Evan Green <evgreen@...omium.org>,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        Dave Young <dyoung@...hat.com>, Baoquan He <bhe@...hat.com>,
        Vivek Goyal <vgoyal@...hat.com>, kexec@...ts.infradead.org
Subject: Re: [PATCH v2 02/12] buildid: Add method to get running kernel's build ID

Quoting Rasmus Villemoes (2021-03-24 02:24:27)
> On 24/03/2021 03.04, Stephen Boyd wrote:
> > Add vmlinux_build_id() so that callers can print a hex format string
> > representation of the running kernel's build ID. This will be used in
> > the kdump and dump_stack code so that developers can easily locate the
> > vmlinux debug symbols for a crash/stacktrace.
> > 
> > Cc: Jiri Olsa <jolsa@...nel.org>
> > Cc: Alexei Starovoitov <ast@...nel.org>
> > Cc: Jessica Yu <jeyu@...nel.org>
> > Cc: Evan Green <evgreen@...omium.org>
> > Cc: Hsin-Yi Wang <hsinyi@...omium.org>
> > Cc: Dave Young <dyoung@...hat.com>
> > Cc: Baoquan He <bhe@...hat.com>
> > Cc: Vivek Goyal <vgoyal@...hat.com>
> > Cc: <kexec@...ts.infradead.org>
> > Signed-off-by: Stephen Boyd <swboyd@...omium.org>
> > ---
> >  include/linux/buildid.h |  2 ++
> >  lib/buildid.c           | 19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/include/linux/buildid.h b/include/linux/buildid.h
> > index ebce93f26d06..2ff6b1b7cc9b 100644
> > --- a/include/linux/buildid.h
> > +++ b/include/linux/buildid.h
> > @@ -10,4 +10,6 @@ int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
> >                  __u32 *size);
> >  int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
> >  
> > +const unsigned char *vmlinux_build_id(void);
> > +
> >  #endif
> > diff --git a/lib/buildid.c b/lib/buildid.c
> > index 010ab0674cb9..fa1b6466b4b8 100644
> > --- a/lib/buildid.c
> > +++ b/lib/buildid.c
> > @@ -4,6 +4,7 @@
> >  #include <linux/elf.h>
> >  #include <linux/kernel.h>
> >  #include <linux/pagemap.h>
> > +#include <linux/string.h>
> >  
> >  #define BUILD_ID 3
> >  
> > @@ -171,3 +172,21 @@ int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size)
> >  {
> >       return parse_build_id_buf(build_id, NULL, buf, buf_size);
> >  }
> > +
> > +/**
> > + * vmlinux_build_id - Get the running kernel's build ID
> > + *
> > + * Return: Running kernel's build ID
> > + */
> > +const unsigned char *vmlinux_build_id(void)
> > +{
> > +     extern const void __start_notes __weak;
> > +     extern const void __stop_notes __weak;
> > +     unsigned int size = &__stop_notes - &__start_notes;
> > +     static unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX];
> > +
> > +     if (!memchr_inv(vmlinux_build_id, 0, BUILD_ID_SIZE_MAX))
> > +             build_id_parse_buf(&__start_notes, vmlinux_build_id, size);
> > +
> > +     return vmlinux_build_id;
> > +}
> > 
> 
> Hm, is there any reason to do that initialization lazily and thus need
> an accessor? If the system is coming down hard, there's a (very very
> small) risk that one thread starts finding the build id, is in the
> middle of the memcpy, another thread also ends up wanting the vmlinux
> build id, sees some non-nul byte, and proceeds to using the partially
> written vmlinux_build_id.
> 
> Perhaps consider just exposing the vmlinux_build_id[] array itself,
> adding a init_vmlinux_build_id() call somewhere early in start_kernel().
> 
> It could then also be made __ro_after_init.
> 
> In any case, if you decide to keep the current way, please rename the
> local variable (just "build_id" is fine) so that it doesn't shadow the
> very function it resides in, that's very confusing.
> 

No particular reason to do it this way. I'll take that approach to
initialize it early in start_kernel() and then expose the array instead.
Thanks!