On Fri, Jul 22, 2022 at 12:25 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
>
> 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.
Sorry, I don't mean we should do it in this patchset.
>
> 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
> > > >
