[bpf-next,v1,7/7] selftests/bpf: Add test for dynptr reinit in user_ringbuf callback

Commit Message

Kumar Kartikeya Dwivedi Nov. 15, 2022, 12:01 a.m. UTC

The original support for bpf_user_ringbuf_drain callbacks simply
short-circuited checks for the dynptr state, allowing users to pass
PTR_TO_DYNPTR (now CONST_PTR_TO_DYNPTR) to helpers that initialize a
dynptr. This bug would have also surfaced with other dynptr helpers in
the future that changed dynptr view or modified it in some way.

Include test cases for all cases, i.e. both bpf_dynptr_from_mem and
bpf_ringbuf_reserve_dynptr, and ensure verifier rejects both of them.
Without the fix, both of these programs load and pass verification.

Acked-by: David Vernet <[e-mail]>
Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
---
 .../selftests/bpf/prog_tests/user_ringbuf.c   |  2 ++
 .../selftests/bpf/progs/user_ringbuf_fail.c   | 35 +++++++++++++++++++
 2 files changed, 37 insertions(+)

Comments

Joanne Koong Nov. 15, 2022, 6:36 p.m. UTC | #1

On Mon, Nov 14, 2022 at 4:01 PM Kumar Kartikeya Dwivedi
<[e-mail]> wrote:
>
> The original support for bpf_user_ringbuf_drain callbacks simply
> short-circuited checks for the dynptr state, allowing users to pass
> PTR_TO_DYNPTR (now CONST_PTR_TO_DYNPTR) to helpers that initialize a
> dynptr. This bug would have also surfaced with other dynptr helpers in
> the future that changed dynptr view or modified it in some way.
>
> Include test cases for all cases, i.e. both bpf_dynptr_from_mem and
> bpf_ringbuf_reserve_dynptr, and ensure verifier rejects both of them.
> Without the fix, both of these programs load and pass verification.
>
> Acked-by: David Vernet <[e-mail]>
> Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>

Acked-by: Joanne Koong <[e-mail]>

Left a small comment below.

> ---
>  .../selftests/bpf/prog_tests/user_ringbuf.c   |  2 ++
>  .../selftests/bpf/progs/user_ringbuf_fail.c   | 35 +++++++++++++++++++
>  2 files changed, 37 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> index 39882580cb90..500a63bb70a8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> @@ -676,6 +676,8 @@ static struct {
>         {"user_ringbuf_callback_discard_dynptr", "cannot release unowned const bpf_dynptr"},
>         {"user_ringbuf_callback_submit_dynptr", "cannot release unowned const bpf_dynptr"},
>         {"user_ringbuf_callback_invalid_return", "At callback return the register R0 has value"},
> +       {"user_ringbuf_callback_reinit_dynptr_mem", "Dynptr has to be an uninitialized dynptr"},
> +       {"user_ringbuf_callback_reinit_dynptr_ringbuf", "Dynptr has to be an uninitialized dynptr"},
>  };
>
>  #define SUCCESS_TEST(_func) { _func, #_func }
> diff --git a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> index 82aba4529aa9..7730d13c0cea 100644
> --- a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> +++ b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> @@ -18,6 +18,13 @@ struct {
>         __uint(type, BPF_MAP_TYPE_USER_RINGBUF);
>  } user_ringbuf SEC(".maps");
>
> +struct {
> +       __uint(type, BPF_MAP_TYPE_RINGBUF);
> +       __uint(max_entries, 2);
> +} ringbuf SEC(".maps");
> +
> +static int map_value;
> +
>  static long
>  bad_access1(struct bpf_dynptr *dynptr, void *context)
>  {
> @@ -175,3 +182,31 @@ int user_ringbuf_callback_invalid_return(void *ctx)
>
>         return 0;
>  }
> +
> +static long
> +try_reinit_dynptr_mem(struct bpf_dynptr *dynptr, void *context)
> +{
> +       bpf_dynptr_from_mem(&map_value, 4, 0, dynptr);
> +       return 0;
> +}
> +
> +static long
> +try_reinit_dynptr_ringbuf(struct bpf_dynptr *dynptr, void *context)
> +{
> +       bpf_ringbuf_reserve_dynptr(&ringbuf, 8, 0, dynptr);
> +       return 0;
> +}
> +
> +SEC("?raw_tp/sys_nanosleep")
> +int user_ringbuf_callback_reinit_dynptr_mem(void *ctx)
> +{
> +       bpf_user_ringbuf_drain(&user_ringbuf, try_reinit_dynptr_mem, NULL, 0);
> +       return 0;
> +}
> +
> +SEC("?raw_tp/sys_nanosleep")

nit: here and above, I think this should just be "?raw_tp/" without
the nanosleep, since there is no nanosleep tracepoint.

> +int user_ringbuf_callback_reinit_dynptr_ringbuf(void *ctx)
> +{
> +       bpf_user_ringbuf_drain(&user_ringbuf, try_reinit_dynptr_ringbuf, NULL, 0);
> +       return 0;
> +}
> --
> 2.38.1
>

Kumar Kartikeya Dwivedi Nov. 15, 2022, 7:41 p.m. UTC | #2

On Wed, Nov 16, 2022 at 12:06:36AM IST, Joanne Koong wrote:
> On Mon, Nov 14, 2022 at 4:01 PM Kumar Kartikeya Dwivedi
> <[e-mail]> wrote:
> >
> > The original support for bpf_user_ringbuf_drain callbacks simply
> > short-circuited checks for the dynptr state, allowing users to pass
> > PTR_TO_DYNPTR (now CONST_PTR_TO_DYNPTR) to helpers that initialize a
> > dynptr. This bug would have also surfaced with other dynptr helpers in
> > the future that changed dynptr view or modified it in some way.
> >
> > Include test cases for all cases, i.e. both bpf_dynptr_from_mem and
> > bpf_ringbuf_reserve_dynptr, and ensure verifier rejects both of them.
> > Without the fix, both of these programs load and pass verification.
> >
> > Acked-by: David Vernet <[e-mail]>
> > Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
>
> Acked-by: Joanne Koong <[e-mail]>
>
> Left a small comment below.
>
> > ---
> >  .../selftests/bpf/prog_tests/user_ringbuf.c   |  2 ++
> >  .../selftests/bpf/progs/user_ringbuf_fail.c   | 35 +++++++++++++++++++
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> > index 39882580cb90..500a63bb70a8 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> > @@ -676,6 +676,8 @@ static struct {
> >         {"user_ringbuf_callback_discard_dynptr", "cannot release unowned const bpf_dynptr"},
> >         {"user_ringbuf_callback_submit_dynptr", "cannot release unowned const bpf_dynptr"},
> >         {"user_ringbuf_callback_invalid_return", "At callback return the register R0 has value"},
> > +       {"user_ringbuf_callback_reinit_dynptr_mem", "Dynptr has to be an uninitialized dynptr"},
> > +       {"user_ringbuf_callback_reinit_dynptr_ringbuf", "Dynptr has to be an uninitialized dynptr"},
> >  };
> >
> >  #define SUCCESS_TEST(_func) { _func, #_func }
> > diff --git a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> > index 82aba4529aa9..7730d13c0cea 100644
> > --- a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> > +++ b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> > @@ -18,6 +18,13 @@ struct {
> >         __uint(type, BPF_MAP_TYPE_USER_RINGBUF);
> >  } user_ringbuf SEC(".maps");
> >
> > +struct {
> > +       __uint(type, BPF_MAP_TYPE_RINGBUF);
> > +       __uint(max_entries, 2);
> > +} ringbuf SEC(".maps");
> > +
> > +static int map_value;
> > +
> >  static long
> >  bad_access1(struct bpf_dynptr *dynptr, void *context)
> >  {
> > @@ -175,3 +182,31 @@ int user_ringbuf_callback_invalid_return(void *ctx)
> >
> >         return 0;
> >  }
> > +
> > +static long
> > +try_reinit_dynptr_mem(struct bpf_dynptr *dynptr, void *context)
> > +{
> > +       bpf_dynptr_from_mem(&map_value, 4, 0, dynptr);
> > +       return 0;
> > +}
> > +
> > +static long
> > +try_reinit_dynptr_ringbuf(struct bpf_dynptr *dynptr, void *context)
> > +{
> > +       bpf_ringbuf_reserve_dynptr(&ringbuf, 8, 0, dynptr);
> > +       return 0;
> > +}
> > +
> > +SEC("?raw_tp/sys_nanosleep")
> > +int user_ringbuf_callback_reinit_dynptr_mem(void *ctx)
> > +{
> > +       bpf_user_ringbuf_drain(&user_ringbuf, try_reinit_dynptr_mem, NULL, 0);
> > +       return 0;
> > +}
> > +
> > +SEC("?raw_tp/sys_nanosleep")
>
> nit: here and above, I think this should just be "?raw_tp/" without
> the nanosleep, since there is no nanosleep tracepoint.
>

True, looks like it's the same for all prior cases in this file as well. I will
drop sys_nanosleep for all of them.

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
index 39882580cb90..500a63bb70a8 100644
--- a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
+++ b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
@@ -676,6 +676,8 @@  static struct {
 	{"user_ringbuf_callback_discard_dynptr", "cannot release unowned const bpf_dynptr"},
 	{"user_ringbuf_callback_submit_dynptr", "cannot release unowned const bpf_dynptr"},
 	{"user_ringbuf_callback_invalid_return", "At callback return the register R0 has value"},
+	{"user_ringbuf_callback_reinit_dynptr_mem", "Dynptr has to be an uninitialized dynptr"},
+	{"user_ringbuf_callback_reinit_dynptr_ringbuf", "Dynptr has to be an uninitialized dynptr"},
 };
 
 #define SUCCESS_TEST(_func) { _func, #_func }
diff --git a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
index 82aba4529aa9..7730d13c0cea 100644
--- a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
+++ b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
@@ -18,6 +18,13 @@  struct {
 	__uint(type, BPF_MAP_TYPE_USER_RINGBUF);
 } user_ringbuf SEC(".maps");
 
+struct {
+	__uint(type, BPF_MAP_TYPE_RINGBUF);
+	__uint(max_entries, 2);
+} ringbuf SEC(".maps");
+
+static int map_value;
+
 static long
 bad_access1(struct bpf_dynptr *dynptr, void *context)
 {
@@ -175,3 +182,31 @@  int user_ringbuf_callback_invalid_return(void *ctx)
 
 	return 0;
 }
+
+static long
+try_reinit_dynptr_mem(struct bpf_dynptr *dynptr, void *context)
+{
+	bpf_dynptr_from_mem(&map_value, 4, 0, dynptr);
+	return 0;
+}
+
+static long
+try_reinit_dynptr_ringbuf(struct bpf_dynptr *dynptr, void *context)
+{
+	bpf_ringbuf_reserve_dynptr(&ringbuf, 8, 0, dynptr);
+	return 0;
+}
+
+SEC("?raw_tp/sys_nanosleep")
+int user_ringbuf_callback_reinit_dynptr_mem(void *ctx)
+{
+	bpf_user_ringbuf_drain(&user_ringbuf, try_reinit_dynptr_mem, NULL, 0);
+	return 0;
+}
+
+SEC("?raw_tp/sys_nanosleep")
+int user_ringbuf_callback_reinit_dynptr_ringbuf(void *ctx)
+{
+	bpf_user_ringbuf_drain(&user_ringbuf, try_reinit_dynptr_ringbuf, NULL, 0);
+	return 0;
+}