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