[PATCHv2 bpf-next 0/2] bpf: Add bpf_vma_build_id_parse kfunc

[PATCHv2 bpf-next 0/2] bpf: Add bpf_vma_build_id_parse kfunc

[Jiri Olsa]

hi,
first version of this patchset added helper for this functionality,
but based Alexei's feedback [1], changing it to kfunc.

With the current build_id_parse function as kfunc we can't effectively
check buffer size provided by user. Therefore adding new function as
bpf kfunc:

  int bpf_vma_build_id_parse(struct vm_area_struct *vma,
                             unsigned char *build_id,
                             size_t build_id__sz);

that triggers kfunc's verifier check for build_id/build_id__sz buffer
size and calls build_id_parse.

thanks,
jirka


[1] https://lore.kernel.org/bpf/CAADnVQKyT4Mm4EdTCYK8c070E-BwPZS_FOkWKLJC80riSGmLTg@mail.gmail.com/
---
Jiri Olsa (2):
      bpf: Add bpf_vma_build_id_parse function and kfunc
      selftests/bpf: Add bpf_vma_build_id_parse kfunc test

 include/linux/bpf.h                                             |  5 +++++
 kernel/bpf/helpers.c                                            | 16 ++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 149 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c

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

[Jiri Olsa]

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;
+}
+
 BTF_SET8_START(tracing_btf_ids)
 #ifdef CONFIG_KEXEC_CORE
 BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
 #endif
+BTF_ID_FLAGS(func, bpf_vma_build_id_parse)
 BTF_SET8_END(tracing_btf_ids)
 
 static const struct btf_kfunc_id_set tracing_kfunc_set = {
-- 
2.38.1

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

[Alexei Starovoitov]

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?

The use case is not clear to me as well.
Do you alwasy expect to call this kfunc from bpf_find_vma callback ?


> +
>  BTF_SET8_START(tracing_btf_ids)
>  #ifdef CONFIG_KEXEC_CORE
>  BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
>  #endif
> +BTF_ID_FLAGS(func, bpf_vma_build_id_parse)
>  BTF_SET8_END(tracing_btf_ids)
>
>  static const struct btf_kfunc_id_set tracing_kfunc_set = {
> --
> 2.38.1
>

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

[Song Liu]

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?
>
> 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

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

[Jiri Olsa]

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

[PATCHv2 bpf-next 2/2] selftests/bpf: Add bpf_vma_build_id_parse kfunc test

[Jiri Olsa]

Adding test for bpf_vma_build_id_parse kfunc.

On bpf side the test finds the vma of the test_progs text through the
test function pointer and reads its build id with the new kfunc.

On user side the test uses readelf to get test_progs build id and
compares it with the one from bpf side.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/bpf_vma_build_id_parse.c   | 88 +++++++++++++++++++
 .../bpf/progs/bpf_vma_build_id_parse.c        | 40 +++++++++
 2 files changed, 128 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c b/tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c
new file mode 100644
index 000000000000..83030a3b2c42
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include "bpf_vma_build_id_parse.skel.h"
+
+#define BUILDID_STR_SIZE (BPF_BUILD_ID_SIZE*2 + 1)
+
+static int read_buildid(char **build_id)
+{
+	char tmp[] = "/tmp/dataXXXXXX";
+	char buf[200];
+	int err, fd;
+	FILE *f;
+
+	fd = mkstemp(tmp);
+	if (fd == -1)
+		return -1;
+	close(fd);
+
+	snprintf(buf, sizeof(buf),
+		"readelf -n ./test_progs 2>/dev/null | grep 'Build ID' | awk '{print $3}' > %s",
+		tmp);
+
+	err = system(buf);
+	if (err)
+		goto out;
+
+	f = fopen(tmp, "r");
+	if (f) {
+		if (fscanf(f, "%ms$*\n", build_id) != 1) {
+			*build_id = NULL;
+			err = -1;
+		}
+		fclose(f);
+	}
+
+out:
+	unlink(tmp);
+	return err;
+}
+
+void test_bpf_vma_build_id_parse(void)
+{
+	char bpf_build_id[BUILDID_STR_SIZE] = {}, *build_id;
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	struct bpf_vma_build_id_parse *skel;
+	int i, err, prog_fd;
+
+	skel = bpf_vma_build_id_parse__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "bpf_vma_build_id_parse__open_and_load"))
+		return;
+
+	skel->bss->target_pid = getpid();
+	skel->bss->addr = (__u64)(uintptr_t)test_bpf_vma_build_id_parse;
+
+	err = bpf_vma_build_id_parse__attach(skel);
+	if (!ASSERT_OK(err, "bpf_vma_build_id_parse__attach"))
+		goto out;
+
+	prog_fd = bpf_program__fd(skel->progs.test1);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "test_run_err");
+	ASSERT_EQ(topts.retval, 0, "test_run_retval");
+
+	ASSERT_EQ(skel->data->ret, 0, "ret");
+
+	ASSERT_GT(skel->data->size_pass, 0, "size_pass");
+	ASSERT_EQ(skel->data->size_fail, -EINVAL, "size_fail");
+
+	/* Read build id via readelf to compare with build_id. */
+	if (!ASSERT_OK(read_buildid(&build_id), "read_buildid"))
+		goto out;
+
+	ASSERT_EQ(skel->data->size_pass, strlen(build_id)/2, "build_id_size");
+
+	/* Convert bpf build id to string, so we can compare it later. */
+	for (i = 0; i < skel->data->size_pass; i++) {
+		sprintf(bpf_build_id + i*2, "%02x",
+			(unsigned char) skel->bss->build_id[i]);
+	}
+	ASSERT_STREQ(bpf_build_id, build_id, "build_id_match");
+
+	free(build_id);
+out:
+	bpf_vma_build_id_parse__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c b/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c
new file mode 100644
index 000000000000..8937212207db
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+#define BPF_BUILD_ID_SIZE 20
+
+extern int bpf_vma_build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
+				  size_t build_id__sz) __ksym;
+
+pid_t target_pid = 0;
+__u64 addr = 0;
+
+int ret = -1;
+int size_pass = -1;
+int size_fail = -1;
+
+unsigned char build_id[BPF_BUILD_ID_SIZE];
+
+static long check_vma(struct task_struct *task, struct vm_area_struct *vma,
+		      void *data)
+{
+	size_fail = bpf_vma_build_id_parse(vma, build_id, sizeof(build_id)/2);
+	size_pass = bpf_vma_build_id_parse(vma, build_id, sizeof(build_id));
+	return 0;
+}
+
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(test1, int a)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+
+	if (task->pid != target_pid)
+		return 0;
+
+	ret = bpf_find_vma(task, addr, check_vma, NULL, 0);
+	return 0;
+}
-- 
2.38.1