Re: [PATCHv2 bpf-next 1/2] bpf: Add bpf_vma_build_id_parse function and kfunc

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Nov 14, 2022 at 10:42:15AM -0800, Song Liu wrote:
> On Mon, Nov 14, 2022 at 9:41 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Nov 11, 2022 at 6:33 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding bpf_vma_build_id_parse function to retrieve build id from
> > > passed vma object and making it available as bpf kfunc.
> > >
> > > We can't use build_id_parse directly as kfunc, because we would
> > > not have control over the build id buffer size provided by user.
> > >
> > > Instead we are adding new bpf_vma_build_id_parse function with
> > > 'build_id__sz' argument that instructs verifier to check for the
> > > available space in build_id buffer.
> > >
> > > This way  we check that there's  always available memory space
> > > behind build_id pointer. We also check that the build_id__sz is
> > > at least BUILD_ID_SIZE_MAX so we can place any buildid in.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  include/linux/bpf.h  |  5 +++++
> > >  kernel/bpf/helpers.c | 16 ++++++++++++++++
> > >  2 files changed, 21 insertions(+)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 798aec816970..5e7c4c50da8e 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -2779,4 +2779,9 @@ struct bpf_key {
> > >         bool has_ref;
> > >  };
> > >  #endif /* CONFIG_KEYS */
> > > +
> > > +extern int bpf_vma_build_id_parse(struct vm_area_struct *vma,
> > > +                                 unsigned char *build_id,
> > > +                                 size_t build_id__sz);
> > > +
> > >  #endif /* _LINUX_BPF_H */
> > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > index 283f55bbeb70..af7a30dafff3 100644
> > > --- a/kernel/bpf/helpers.c
> > > +++ b/kernel/bpf/helpers.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/proc_ns.h>
> > >  #include <linux/security.h>
> > >  #include <linux/btf_ids.h>
> > > +#include <linux/buildid.h>
> > >
> > >  #include "../../lib/kstrtox.h"
> > >
> > > @@ -1706,10 +1707,25 @@ bpf_base_func_proto(enum bpf_func_id func_id)
> > >         }
> > >  }
> > >
> > > +int bpf_vma_build_id_parse(struct vm_area_struct *vma,
> > > +                          unsigned char *build_id,
> > > +                          size_t build_id__sz)
> > > +{
> > > +       __u32 size;
> > > +       int err;
> > > +
> > > +       if (build_id__sz < BUILD_ID_SIZE_MAX)
> > > +               return -EINVAL;
> > > +
> > > +       err = build_id_parse(vma, build_id, &size);
> > > +       return err ?: (int) size;
> > > +}
> >
> > And you'll allow any tracing prog to call it like this?
> > Feels obviously unsafe unless I'm missing something big here.
> > See the amount of safety checks that
> > stack_map_get_build_id_offset() does.
> > Why can we get away without them here?
> >

ugh right.. I use it always from bpf_find_vma callback, that's probably
why I did not realize that, because it's already locked there

> > The use case is not clear to me as well.
> > Do you alwasy expect to call this kfunc from bpf_find_vma callback ?
> 
> It is also safe to call it from iter/task_vma programs. Some allow list
> seems necessary here.

I have 2 places I call it from: sched_process_exec tp_btf and process iterator
both through bpf_find_vma callback.. so I think we can allow it only from
bpf_find_vma callback.. that should keep it simple for start

thanks,
jirka