On Mon, Nov 14, 2022 at 9:41 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Fri, Nov 11, 2022 at 6:33 AM Jiri Olsa <jolsa@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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?
>
> 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.
Thanks,
Song
