| Message ID | 20230120070355.1983560-13-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynptr fixes | expand |
On Thu, Jan 19, 2023 at 11:04 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > First test that we allow overwriting dynptr slots and reinitializing > them in unreferenced case, and disallow overwriting for referenced case. > Include tests to ensure slices obtained from destroyed dynptrs are being > invalidated on their destruction. The destruction needs to be scoped, as > in slices of dynptr A should not be invalidated when dynptr B is > destroyed. Next, test that MEM_UNINIT doesn't allow writing dynptr stack > slots. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> Acked-by: Joanne Koong <joannelkoong@gmail.com> > --- > .../testing/selftests/bpf/progs/dynptr_fail.c | 129 ++++++++++++++++++ > 1 file changed, 129 insertions(+) > > diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c > index 1cbec5468879..c10abb98e47d 100644 > --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c > +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c > @@ -900,3 +900,132 @@ int dynptr_partial_slot_invalidate(struct __sk_buff *ctx) > ); > return 0; > } > + > +SEC("?raw_tp") > +__success > +int dynptr_overwrite_unref(void *ctx) > +{ > + struct bpf_dynptr ptr; > + > + if (get_map_val_dynptr(&ptr)) > + return 0; > + if (get_map_val_dynptr(&ptr)) > + return 0; > + if (get_map_val_dynptr(&ptr)) > + return 0; > + > + return 0; > +} > + > +SEC("?raw_tp") > +__failure __msg("R1 type=scalar expected=percpu_ptr_") > +int dynptr_invalidate_slice_or_null(void *ctx) > +{ > + struct bpf_dynptr ptr; > + __u8 *p; > + > + if (get_map_val_dynptr(&ptr)) > + return 0; > + > + p = bpf_dynptr_data(&ptr, 0, 1); > + *(__u8 *)&ptr = 0; > + bpf_this_cpu_ptr(p); > + return 0; > +} > + nit: do you mind adding in a comment ("/* this should fail */") above the line that triggers the verifier error to the new tests? > +SEC("?raw_tp") > +__failure __msg("R7 invalid mem access 'scalar'") > +int dynptr_invalidate_slice_failure(void *ctx) > +{ > + struct bpf_dynptr ptr1; > + struct bpf_dynptr ptr2; > + __u8 *p1, *p2; > + > + if (get_map_val_dynptr(&ptr1)) > + return 0; > + if (get_map_val_dynptr(&ptr2)) > + return 0; > + > + p1 = bpf_dynptr_data(&ptr1, 0, 1); > + if (!p1) > + return 0; > + p2 = bpf_dynptr_data(&ptr2, 0, 1); > + if (!p2) > + return 0; > + > + *(__u8 *)&ptr1 = 0; > + return *p1; > +} > + > +SEC("?raw_tp") > +__success > +int dynptr_invalidate_slice_success(void *ctx) > +{ > + struct bpf_dynptr ptr1; > + struct bpf_dynptr ptr2; > + __u8 *p1, *p2; > + > + if (get_map_val_dynptr(&ptr1)) > + return 1; > + if (get_map_val_dynptr(&ptr2)) > + return 1; > + > + p1 = bpf_dynptr_data(&ptr1, 0, 1); > + if (!p1) > + return 1; > + p2 = bpf_dynptr_data(&ptr2, 0, 1); > + if (!p2) > + return 1; > + > + *(__u8 *)&ptr1 = 0; > + return *p2; > +} > + > +SEC("?raw_tp") > +__failure __msg("cannot overwrite referenced dynptr") > +int dynptr_overwrite_ref(void *ctx) > +{ > + struct bpf_dynptr ptr; > + > + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr); > + if (get_map_val_dynptr(&ptr)) > + bpf_ringbuf_discard_dynptr(&ptr, 0); > + return 0; > +} > + > +/* Reject writes to dynptr slot from bpf_dynptr_read */ > +SEC("?raw_tp") > +__failure __msg("potential write to dynptr at off=-16") > +int dynptr_read_into_slot(void *ctx) > +{ > + union { > + struct { > + char _pad[48]; > + struct bpf_dynptr ptr; > + }; > + char buf[64]; > + } data; > + > + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &data.ptr); > + /* this should fail */ > + bpf_dynptr_read(data.buf, sizeof(data.buf), &data.ptr, 0, 0); > + > + return 0; > +} > + > +/* Reject writes to dynptr slot for uninit arg */ > +SEC("?raw_tp") > +__failure __msg("potential write to dynptr at off=-16") > +int uninit_write_into_slot(void *ctx) > +{ > + struct { > + char buf[64]; > + struct bpf_dynptr ptr; > + } data; > + > + bpf_ringbuf_reserve_dynptr(&ringbuf, 80, 0, &data.ptr); > + /* this should fail */ > + bpf_get_current_comm(data.buf, 80); > + > + return 0; > +} Another test I think would be helpful is verifying that data slices are invalidated if the dynptr is invalidated within a callback. Something like: static int callback(__u32 index, void *data) { *(__u32 *)data = 123; return 0; } /* If the dynptr is written into in a callback function, its data slices should be invalidated as well */ SEC("?raw_tp") __failure __msg("invalid mem access 'scalar'") int invalid_data_slices(void *ctx) { struct bpf_dynptr ptr; __u32 *slice; get_map_val_dynptr(&ptr); slice = bpf_dynptr_data(&ptr, 0, sizeof(_u32)); if (!slice) return 0; bpf_loop(10, callback, &ptr, 0); /* this should fail */ *slice = 1; return 0; } > -- > 2.39.1 >
On Sat, Jan 21, 2023 at 04:44:42AM IST, Joanne Koong wrote: > On Thu, Jan 19, 2023 at 11:04 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > First test that we allow overwriting dynptr slots and reinitializing > > them in unreferenced case, and disallow overwriting for referenced case. > > Include tests to ensure slices obtained from destroyed dynptrs are being > > invalidated on their destruction. The destruction needs to be scoped, as > > in slices of dynptr A should not be invalidated when dynptr B is > > destroyed. Next, test that MEM_UNINIT doesn't allow writing dynptr stack > > slots. > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > Acked-by: Joanne Koong <joannelkoong@gmail.com> > > > --- > > .../testing/selftests/bpf/progs/dynptr_fail.c | 129 ++++++++++++++++++ > > 1 file changed, 129 insertions(+) > > > > diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c > > index 1cbec5468879..c10abb98e47d 100644 > > --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c > > +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c > > @@ -900,3 +900,132 @@ int dynptr_partial_slot_invalidate(struct __sk_buff *ctx) > > ); > > return 0; > > } > > + > > +SEC("?raw_tp") > > +__success > > +int dynptr_overwrite_unref(void *ctx) > > +{ > > + struct bpf_dynptr ptr; > > + > > + if (get_map_val_dynptr(&ptr)) > > + return 0; > > + if (get_map_val_dynptr(&ptr)) > > + return 0; > > + if (get_map_val_dynptr(&ptr)) > > + return 0; > > + > > + return 0; > > +} > > + > > +SEC("?raw_tp") > > +__failure __msg("R1 type=scalar expected=percpu_ptr_") > > +int dynptr_invalidate_slice_or_null(void *ctx) > > +{ > > + struct bpf_dynptr ptr; > > + __u8 *p; > > + > > + if (get_map_val_dynptr(&ptr)) > > + return 0; > > + > > + p = bpf_dynptr_data(&ptr, 0, 1); > > + *(__u8 *)&ptr = 0; > > + bpf_this_cpu_ptr(p); > > + return 0; > > +} > > + > > nit: do you mind adding in a comment ("/* this should fail */") above > the line that triggers the verifier error to the new tests? > I've added this comment to whichever statement triggers the error, and a short comment over the tests. > > > +SEC("?raw_tp") > > +__failure __msg("R7 invalid mem access 'scalar'") > > +int dynptr_invalidate_slice_failure(void *ctx) > > +{ > > + struct bpf_dynptr ptr1; > > + struct bpf_dynptr ptr2; > > + __u8 *p1, *p2; > > + > > + if (get_map_val_dynptr(&ptr1)) > > + return 0; > > + if (get_map_val_dynptr(&ptr2)) > > + return 0; > > + > > + p1 = bpf_dynptr_data(&ptr1, 0, 1); > > + if (!p1) > > + return 0; > > + p2 = bpf_dynptr_data(&ptr2, 0, 1); > > + if (!p2) > > + return 0; > > + > > + *(__u8 *)&ptr1 = 0; > > + return *p1; > > +} > > + > > +SEC("?raw_tp") > > +__success > > +int dynptr_invalidate_slice_success(void *ctx) > > +{ > > + struct bpf_dynptr ptr1; > > + struct bpf_dynptr ptr2; > > + __u8 *p1, *p2; > > + > > + if (get_map_val_dynptr(&ptr1)) > > + return 1; > > + if (get_map_val_dynptr(&ptr2)) > > + return 1; > > + > > + p1 = bpf_dynptr_data(&ptr1, 0, 1); > > + if (!p1) > > + return 1; > > + p2 = bpf_dynptr_data(&ptr2, 0, 1); > > + if (!p2) > > + return 1; > > + > > + *(__u8 *)&ptr1 = 0; > > + return *p2; > > +} > > + > > +SEC("?raw_tp") > > +__failure __msg("cannot overwrite referenced dynptr") > > +int dynptr_overwrite_ref(void *ctx) > > +{ > > + struct bpf_dynptr ptr; > > + > > + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr); > > + if (get_map_val_dynptr(&ptr)) > > + bpf_ringbuf_discard_dynptr(&ptr, 0); > > + return 0; > > +} > > + > > +/* Reject writes to dynptr slot from bpf_dynptr_read */ > > +SEC("?raw_tp") > > +__failure __msg("potential write to dynptr at off=-16") > > +int dynptr_read_into_slot(void *ctx) > > +{ > > + union { > > + struct { > > + char _pad[48]; > > + struct bpf_dynptr ptr; > > + }; > > + char buf[64]; > > + } data; > > + > > + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &data.ptr); > > + /* this should fail */ > > + bpf_dynptr_read(data.buf, sizeof(data.buf), &data.ptr, 0, 0); > > + > > + return 0; > > +} > > + > > +/* Reject writes to dynptr slot for uninit arg */ > > +SEC("?raw_tp") > > +__failure __msg("potential write to dynptr at off=-16") > > +int uninit_write_into_slot(void *ctx) > > +{ > > + struct { > > + char buf[64]; > > + struct bpf_dynptr ptr; > > + } data; > > + > > + bpf_ringbuf_reserve_dynptr(&ringbuf, 80, 0, &data.ptr); > > + /* this should fail */ > > + bpf_get_current_comm(data.buf, 80); > > + > > + return 0; > > +} > > Another test I think would be helpful is verifying that data slices > are invalidated if the dynptr is invalidated within a callback. > Something like: > > static int callback(__u32 index, void *data) > { > *(__u32 *)data = 123; > > return 0; > } > > /* If the dynptr is written into in a callback function, its data > slices should be invalidated as well */ > SEC("?raw_tp") > __failure __msg("invalid mem access 'scalar'") > int invalid_data_slices(void *ctx) > { > struct bpf_dynptr ptr; > __u32 *slice; > > get_map_val_dynptr(&ptr); > > slice = bpf_dynptr_data(&ptr, 0, sizeof(_u32)); > if (!slice) > return 0; > > bpf_loop(10, callback, &ptr, 0); > > /* this should fail */ > *slice = 1; > > return 0; > } Yes, looks good. I added this and one more test which tests the unint -> check_mem_access -> destroy path as well, and will respin.
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c index 1cbec5468879..c10abb98e47d 100644 --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c @@ -900,3 +900,132 @@ int dynptr_partial_slot_invalidate(struct __sk_buff *ctx) ); return 0; } + +SEC("?raw_tp") +__success +int dynptr_overwrite_unref(void *ctx) +{ + struct bpf_dynptr ptr; + + if (get_map_val_dynptr(&ptr)) + return 0; + if (get_map_val_dynptr(&ptr)) + return 0; + if (get_map_val_dynptr(&ptr)) + return 0; + + return 0; +} + +SEC("?raw_tp") +__failure __msg("R1 type=scalar expected=percpu_ptr_") +int dynptr_invalidate_slice_or_null(void *ctx) +{ + struct bpf_dynptr ptr; + __u8 *p; + + if (get_map_val_dynptr(&ptr)) + return 0; + + p = bpf_dynptr_data(&ptr, 0, 1); + *(__u8 *)&ptr = 0; + bpf_this_cpu_ptr(p); + return 0; +} + +SEC("?raw_tp") +__failure __msg("R7 invalid mem access 'scalar'") +int dynptr_invalidate_slice_failure(void *ctx) +{ + struct bpf_dynptr ptr1; + struct bpf_dynptr ptr2; + __u8 *p1, *p2; + + if (get_map_val_dynptr(&ptr1)) + return 0; + if (get_map_val_dynptr(&ptr2)) + return 0; + + p1 = bpf_dynptr_data(&ptr1, 0, 1); + if (!p1) + return 0; + p2 = bpf_dynptr_data(&ptr2, 0, 1); + if (!p2) + return 0; + + *(__u8 *)&ptr1 = 0; + return *p1; +} + +SEC("?raw_tp") +__success +int dynptr_invalidate_slice_success(void *ctx) +{ + struct bpf_dynptr ptr1; + struct bpf_dynptr ptr2; + __u8 *p1, *p2; + + if (get_map_val_dynptr(&ptr1)) + return 1; + if (get_map_val_dynptr(&ptr2)) + return 1; + + p1 = bpf_dynptr_data(&ptr1, 0, 1); + if (!p1) + return 1; + p2 = bpf_dynptr_data(&ptr2, 0, 1); + if (!p2) + return 1; + + *(__u8 *)&ptr1 = 0; + return *p2; +} + +SEC("?raw_tp") +__failure __msg("cannot overwrite referenced dynptr") +int dynptr_overwrite_ref(void *ctx) +{ + struct bpf_dynptr ptr; + + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr); + if (get_map_val_dynptr(&ptr)) + bpf_ringbuf_discard_dynptr(&ptr, 0); + return 0; +} + +/* Reject writes to dynptr slot from bpf_dynptr_read */ +SEC("?raw_tp") +__failure __msg("potential write to dynptr at off=-16") +int dynptr_read_into_slot(void *ctx) +{ + union { + struct { + char _pad[48]; + struct bpf_dynptr ptr; + }; + char buf[64]; + } data; + + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &data.ptr); + /* this should fail */ + bpf_dynptr_read(data.buf, sizeof(data.buf), &data.ptr, 0, 0); + + return 0; +} + +/* Reject writes to dynptr slot for uninit arg */ +SEC("?raw_tp") +__failure __msg("potential write to dynptr at off=-16") +int uninit_write_into_slot(void *ctx) +{ + struct { + char buf[64]; + struct bpf_dynptr ptr; + } data; + + bpf_ringbuf_reserve_dynptr(&ringbuf, 80, 0, &data.ptr); + /* this should fail */ + bpf_get_current_comm(data.buf, 80); + + return 0; +}
First test that we allow overwriting dynptr slots and reinitializing them in unreferenced case, and disallow overwriting for referenced case. Include tests to ensure slices obtained from destroyed dynptrs are being invalidated on their destruction. The destruction needs to be scoped, as in slices of dynptr A should not be invalidated when dynptr B is destroyed. Next, test that MEM_UNINIT doesn't allow writing dynptr stack slots. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- .../testing/selftests/bpf/progs/dynptr_fail.c | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+)