Re: [PATCH bpf-next v1 2/2] selftests/bpf: add extra test for using dynptr data slice after release

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jul 22, 2022 at 9:40 AM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>
> 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.

Thanks Joanne, appreciate the comment about the cause of failure! I
noticed that in other test cases like

ringbuf_missing_release1()
ringbuf_missing_release2()

There is a general description before the release1, and more details
inside each function. Maybe following that convention is better? Sorry
for having many requests from me. :)

> > > +{
> > > +       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 :)
>

Sounds good. It would be great if we can fix that bug and spare the
use of ptr1 here.

Thanks!

> >
> > >  /* 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
> > >



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux