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