On Thu, Jul 21, 2022 at 10:28 AM Hao Luo <haoluo@xxxxxxxxxx> wrote:
>
> Hi Joanne,
>
> On Wed, Jul 20, 2022 at 7:49 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
> >
> > Add an additional test, "data_slice_use_after_release2", for ensuring
> > that data slices are correctly invalidated by the verifier after the
> > dynptr whose ref obj id they track is released. In particular, this
> > tests data slice invalidation for dynptrs located at a non-zero offset
> > from the frame pointer.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> > .../testing/selftests/bpf/prog_tests/dynptr.c | 3 +-
> > .../testing/selftests/bpf/progs/dynptr_fail.c | 32 ++++++++++++++++++-
> > 2 files changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> > index 3c7aa82b98e2..bcf80b9f7c27 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> > @@ -22,7 +22,8 @@ static struct {
> > {"add_dynptr_to_map2", "invalid indirect read from stack"},
> > {"data_slice_out_of_bounds_ringbuf", "value is outside of the allowed memory range"},
> > {"data_slice_out_of_bounds_map_value", "value is outside of the allowed memory range"},
> > - {"data_slice_use_after_release", "invalid mem access 'scalar'"},
> > + {"data_slice_use_after_release1", "invalid mem access 'scalar'"},
> > + {"data_slice_use_after_release2", "invalid mem access 'scalar'"},
> > {"data_slice_missing_null_check1", "invalid mem access 'mem_or_null'"},
> > {"data_slice_missing_null_check2", "invalid mem access 'mem_or_null'"},
> > {"invalid_helper1", "invalid indirect read from stack"},
> > diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> > index d811cff73597..d8c4ed3ee146 100644
> > --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> > +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> > @@ -248,7 +248,7 @@ int data_slice_out_of_bounds_map_value(void *ctx)
> >
> > /* A data slice can't be used after it has been released */
> > SEC("?raw_tp/sys_nanosleep")
> > -int data_slice_use_after_release(void *ctx)
> > +int data_slice_use_after_release1(void *ctx)
> > {
> > struct bpf_dynptr ptr;
> > struct sample *sample;
> > @@ -272,6 +272,36 @@ int data_slice_use_after_release(void *ctx)
> > return 0;
> > }
> >
> > +SEC("?raw_tp/sys_nanosleep")
> > +int data_slice_use_after_release2(void *ctx)
>
> Could you put comments explaining the reason for failure, like other test cases?
>
Hi Hao. The explanation for the data_slice_use_after_release test
cases is above the "data_slice_use_after_release1" case, but I can
also copy/paste that comment to above "data_slice_use_after_release2"
as well to make it easier to spot. I'll do that for v2.
> > +{
> > + struct bpf_dynptr ptr1, ptr2;
> > + struct sample *sample;
> > +
> > + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr1);
> > + bpf_ringbuf_reserve_dynptr(&ringbuf, sizeof(*sample), 0, &ptr2);
> > +
> > + sample = bpf_dynptr_data(&ptr2, 0, sizeof(*sample));
> > + if (!sample)
> > + goto done;
> > +
> > + sample->pid = 23;
> > +
> > + bpf_ringbuf_submit_dynptr(&ptr2, 0);
> > +
> > + /* this should fail */
> > + sample->pid = 23;
> > +
> > + bpf_ringbuf_submit_dynptr(&ptr1, 0);
> > +
> > + return 0;
> > +
> > +done:
> > + bpf_ringbuf_discard_dynptr(&ptr2, 0);
> > + bpf_ringbuf_discard_dynptr(&ptr1, 0);
> > + return 0;
> > +}
> > +
>
> Joanne, I haven't been following the effort of dynptr, so I am still
> learning dynptr. Is there any use of `ptr1` in this test case?
The use of ptr1 is so that ptr2 will be at a non-zero offset from the
frame pointer. This bug previously was unspotted because we were only
testing invalidated data slices for ptrs that were at a zero offset.
I will include a comment about this in the test to make it more clear :)
>
> > /* A data slice must be first checked for NULL */
> > SEC("?raw_tp/sys_nanosleep")
> > int data_slice_missing_null_check1(void *ctx)
> > --
> > 2.30.2
> >
