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
> <memxor@xxxxxxxxx> 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 <void@xxxxxxxxxxxxx>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
>
> Acked-by: Joanne Koong <joannelkoong@xxxxxxxxx>
>
> 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.
