On Mon, Nov 14, 2022 at 4:01 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> The original support for bpf_user_ringbuf_drain callbacks simply
> short-circuited checks for the dynptr state, allowing users to pass
> PTR_TO_DYNPTR (now CONST_PTR_TO_DYNPTR) to helpers that initialize a
> dynptr. This bug would have also surfaced with other dynptr helpers in
> the future that changed dynptr view or modified it in some way.
>
> Include test cases for all cases, i.e. both bpf_dynptr_from_mem and
> bpf_ringbuf_reserve_dynptr, and ensure verifier rejects both of them.
> Without the fix, both of these programs load and pass verification.
>
> Acked-by: David Vernet <void@xxxxxxxxxxxxx>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
Acked-by: Joanne Koong <joannelkoong@xxxxxxxxx>
Left a small comment below.
> ---
> .../selftests/bpf/prog_tests/user_ringbuf.c | 2 ++
> .../selftests/bpf/progs/user_ringbuf_fail.c | 35 +++++++++++++++++++
> 2 files changed, 37 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> index 39882580cb90..500a63bb70a8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> @@ -676,6 +676,8 @@ static struct {
> {"user_ringbuf_callback_discard_dynptr", "cannot release unowned const bpf_dynptr"},
> {"user_ringbuf_callback_submit_dynptr", "cannot release unowned const bpf_dynptr"},
> {"user_ringbuf_callback_invalid_return", "At callback return the register R0 has value"},
> + {"user_ringbuf_callback_reinit_dynptr_mem", "Dynptr has to be an uninitialized dynptr"},
> + {"user_ringbuf_callback_reinit_dynptr_ringbuf", "Dynptr has to be an uninitialized dynptr"},
> };
>
> #define SUCCESS_TEST(_func) { _func, #_func }
> diff --git a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> index 82aba4529aa9..7730d13c0cea 100644
> --- a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> +++ b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> @@ -18,6 +18,13 @@ struct {
> __uint(type, BPF_MAP_TYPE_USER_RINGBUF);
> } user_ringbuf SEC(".maps");
>
> +struct {
> + __uint(type, BPF_MAP_TYPE_RINGBUF);
> + __uint(max_entries, 2);
> +} ringbuf SEC(".maps");
> +
> +static int map_value;
> +
> static long
> bad_access1(struct bpf_dynptr *dynptr, void *context)
> {
> @@ -175,3 +182,31 @@ int user_ringbuf_callback_invalid_return(void *ctx)
>
> return 0;
> }
> +
> +static long
> +try_reinit_dynptr_mem(struct bpf_dynptr *dynptr, void *context)
> +{
> + bpf_dynptr_from_mem(&map_value, 4, 0, dynptr);
> + return 0;
> +}
> +
> +static long
> +try_reinit_dynptr_ringbuf(struct bpf_dynptr *dynptr, void *context)
> +{
> + bpf_ringbuf_reserve_dynptr(&ringbuf, 8, 0, dynptr);
> + return 0;
> +}
> +
> +SEC("?raw_tp/sys_nanosleep")
> +int user_ringbuf_callback_reinit_dynptr_mem(void *ctx)
> +{
> + bpf_user_ringbuf_drain(&user_ringbuf, try_reinit_dynptr_mem, NULL, 0);
> + return 0;
> +}
> +
> +SEC("?raw_tp/sys_nanosleep")
nit: here and above, I think this should just be "?raw_tp/" without
the nanosleep, since there is no nanosleep tracepoint.
> +int user_ringbuf_callback_reinit_dynptr_ringbuf(void *ctx)
> +{
> + bpf_user_ringbuf_drain(&user_ringbuf, try_reinit_dynptr_ringbuf, NULL, 0);
> + return 0;
> +}
> --
> 2.38.1
>
