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@xxxxxxxxx> 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@xxxxxxxxx>
>
> Acked-by: Joanne Koong <joannelkoong@xxxxxxxxx>
>
> > ---
> > .../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.
