linux-kernel - Re: [PATCH RFC 1/5] mm: Store build id in file object

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: <Y+T9iwfvj23gtDMA@krava>
Date:   Thu, 9 Feb 2023 15:04:59 +0100
From:   Jiri Olsa <olsajiri@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Hao Luo <haoluo@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        bpf@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-perf-users@...r.kernel.org, Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH RFC 1/5] mm: Store build id in file object

On Wed, Feb 08, 2023 at 03:52:40PM -0800, Andrii Nakryiko wrote:

SNIP

> > diff --git a/include/linux/buildid.h b/include/linux/buildid.h
> > index 3b7a0ff4642f..7c818085ad2c 100644
> > --- a/include/linux/buildid.h
> > +++ b/include/linux/buildid.h
> > @@ -3,9 +3,15 @@
> >  #define _LINUX_BUILDID_H
> >
> >  #include <linux/mm_types.h>
> > +#include <linux/slab.h>
> >
> >  #define BUILD_ID_SIZE_MAX 20
> >
> > +struct build_id {
> > +       u32 sz;
> > +       char data[BUILD_ID_SIZE_MAX];
> 
> don't know if 21 vs 24 matters for kmem_cache_create(), but we don't
> need 4 bytes to store build_id size, given max size is 20, so maybe
> use u8 for sz?

ok

> 
> > +};
> > +
> >  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);
> > @@ -17,4 +23,15 @@ void init_vmlinux_build_id(void);
> >  static inline void init_vmlinux_build_id(void) { }
> >  #endif
> >
> > +#ifdef CONFIG_FILE_BUILD_ID
> > +void __init build_id_init(void);
> > +void build_id_free(struct build_id *bid);
> > +int vma_get_build_id(struct vm_area_struct *vma, struct build_id **bidp);
> > +void file_build_id_free(struct file *f);
> > +#else
> > +static inline void __init build_id_init(void) { }
> > +static inline void build_id_free(struct build_id *bid) { }
> > +static inline void file_build_id_free(struct file *f) { }
> > +#endif /* CONFIG_FILE_BUILD_ID */
> > +
> >  #endif
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index c1769a2c5d70..9ad5e5fbf680 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -975,6 +975,9 @@ struct file {
> >         struct address_space    *f_mapping;
> >         errseq_t                f_wb_err;
> >         errseq_t                f_sb_err; /* for syncfs */
> > +#ifdef CONFIG_FILE_BUILD_ID
> > +       struct build_id         *f_bid;
> 
> naming nit: anything wrong with f_buildid or f_build_id? all the
> related APIs use fully spelled out "build_id"

ok

SNIP

> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 425a9349e610..a06f744206e3 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2530,6 +2530,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >         pgoff_t vm_pgoff;
> >         int error;
> >         MA_STATE(mas, &mm->mm_mt, addr, end - 1);
> > +       struct build_id *bid = NULL;
> >
> >         /* Check against address space limit. */
> >         if (!may_expand_vm(mm, vm_flags, len >> PAGE_SHIFT)) {
> > @@ -2626,6 +2627,13 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >                 if (error)
> >                         goto unmap_and_free_vma;
> >
> > +#ifdef CONFIG_FILE_BUILD_ID
> > +               if (vma->vm_flags & VM_EXEC && !file->f_bid) {
> > +                       error = vma_get_build_id(vma, &bid);
> > +                       if (error)
> > +                               goto close_and_free_vma;
> 
> do we want to fail mmap_region() if we get -ENOMEM from
> vma_get_build_id()? can't we just store ERR_PTR(error) in f_bid field?
> So we'll have f_bid == NULL for non-exec files, ERR_PTR() for when we
> tried and failed to get build ID, and a valid pointer if we succeeded?

I guess we can do that.. might be handy for debugging

also build_id_parse might fail on missing build id, so you're right,
we should not fail mmap_region in here

thanks,
jirka