| Message ID | 20220721024821.251231-2-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v1,1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
Hi Joanne, On Wed, Jul 20, 2022 at 7:49 PM Joanne Koong <joannelkoong@gmail.com> 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@gmail.com> > --- > .../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? > +{ > + 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? > /* 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 >
On Thu, Jul 21, 2022 at 10:28 AM Hao Luo <haoluo@google.com> wrote: > > Hi Joanne, > > On Wed, Jul 20, 2022 at 7:49 PM Joanne Koong <joannelkoong@gmail.com> 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@gmail.com> > > --- > > .../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 > >
On Fri, Jul 22, 2022 at 9:40 AM Joanne Koong <joannelkoong@gmail.com> wrote: > > On Thu, Jul 21, 2022 at 10:28 AM Hao Luo <haoluo@google.com> wrote: > > > > Hi Joanne, > > > > On Wed, Jul 20, 2022 at 7:49 PM Joanne Koong <joannelkoong@gmail.com> 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@gmail.com> > > > --- > > > .../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 > > >
On Fri, Jul 22, 2022 at 12:25 PM Hao Luo <haoluo@google.com> wrote: > > On Fri, Jul 22, 2022 at 9:40 AM Joanne Koong <joannelkoong@gmail.com> wrote: > > > > On Thu, Jul 21, 2022 at 10:28 AM Hao Luo <haoluo@google.com> wrote: > > > > > > Hi Joanne, > > > > > > On Wed, Jul 20, 2022 at 7:49 PM Joanne Koong <joannelkoong@gmail.com> 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@gmail.com> > > > > --- > > > > .../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 > > > >
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) +{ + 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; +} + /* A data slice must be first checked for NULL */ SEC("?raw_tp/sys_nanosleep") int data_slice_missing_null_check1(void *ctx)
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@gmail.com> --- .../testing/selftests/bpf/prog_tests/dynptr.c | 3 +- .../testing/selftests/bpf/progs/dynptr_fail.c | 32 ++++++++++++++++++- 2 files changed, 33 insertions(+), 2 deletions(-)