Re: [RFC PATCH bpf-next v1 08/11] bpf: Let the callback of bpf_user_ringbuf_drain unwind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: missing callback dynptr invalidation on bpf_user_ringbuf_drain() unwind
--

commit 5e41f31a219e475ec387e971ce587d69bc948017
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Let the callback of bpf_user_ringbuf_drain unwind

This commit adds an unwind pad to bpf_user_ringbuf_drain() so that its
callback can exit via unwind_exit while releasing the sample, clearing the
busy bit, and queuing any pending wakeup. It also adds
BPF_FUNC_user_ringbuf_drain to helper_has_cb_pad() in the verifier.

> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 447863ece26a3..fdd997f36f73d 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
[ ... ]
> @@ -852,8 +854,23 @@ BPF_CALL_4(bpf_user_ringbuf_drain, struct bpf_map *, map,
>  		}
>  
>  		bpf_dynptr_init(&dynptr, sample, BPF_DYNPTR_TYPE_LOCAL, 0, size);
> -		ret = callback((uintptr_t)&dynptr, (uintptr_t)callback_ctx, 0, 0, 0);
> +		bpf_call_cb(callback, &dynptr, callback_ctx, 0, 0, 0, ret, unwound_at);
> +		if (0) {
> +unwound_at:
> +			/*
> +			 * The callback left with unwind_exit. The sample is
> +			 * consumed and counted like after a return, so that
> +			 * the wakeup below is sent for it, and the helper
> +			 * leaves through the door below, as the error paths
> +			 * do, with the r0 of the callback.
> +			 */
> +			unwound = true;
> +		}
>  		__bpf_user_ringbuf_sample_release(rb, size, flags);
> +		if (unwound) {
> +			samples++;
> +			goto schedule_work_return;
> +		}
>  	}
>  	ret = samples - discarded_samples;
>  
> @@ -867,6 +884,8 @@ BPF_CALL_4(bpf_user_ringbuf_drain, struct bpf_map *, map,
>  		irq_work_queue(&rb->work);
>  	else if (!(flags & BPF_RB_NO_WAKEUP) && samples > 0)
>  		irq_work_queue(&rb->work);
> +	if (unwound)
> +		bpf_unwind_leave();
>  	return ret;
>  }
>  
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 3c39cc0f358a1..fd63ff36eb909 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11409,6 +11409,7 @@ static bool helper_has_cb_pad(enum bpf_func_id func_id)
>  	case BPF_FUNC_loop:
>  	case BPF_FUNC_for_each_map_elem:
>  	case BPF_FUNC_find_vma:
> +	case BPF_FUNC_user_ringbuf_drain:
>  		return true;

[Severity: Critical]
Can a callback for bpf_user_ringbuf_drain() smuggle its R1 dynptr (or a
slice derived from it via bpf_dynptr_data() or bpf_dynptr_slice()) out to
the caller's unwind pad in R0 when exiting via unwind_exit?

In bpf_user_ringbuf_drain(), struct bpf_dynptr_kern dynptr lives on the
C stack frame of bpf_user_ringbuf_drain(). When the callback exits via
unwind_exit and jumps to unwound_at, bpf_user_ringbuf_drain() releases the
sample with __bpf_user_ringbuf_sample_release(), clears rb->busy, and
returns ret (the callback's R0) to the caller's unwind pad after its stack
frame is popped.

In the verifier, set_user_ringbuf_callback_state() initializes the
callback's R1 using mark_dynptr_cb_reg(), which sets R1 to
CONST_PTR_TO_DYNPTR with a fresh id that is not backed by a STACK_DYNPTR
slot in the callee's BPF stack:

kernel/bpf/verifier.c:set_user_ringbuf_callback_state() {
	...
	bpf_mark_reg_not_init(env, &callee->regs[BPF_REG_0]);
	mark_dynptr_cb_reg(env, &callee->regs[BPF_REG_1], BPF_DYNPTR_TYPE_LOCAL);
	callee->regs[BPF_REG_2] = caller->regs[BPF_REG_3];
	...
}

When the callback executes unwind_exit, prepare_func_exit() only rejects
PTR_TO_STACK in R0, copies callee->regs[BPF_REG_0] into
caller->regs[BPF_REG_0], and calls destroy_dynptrs_in_stack_slots(), which
only invalidates STACK_DYNPTR slots in the callee's stack frame:

kernel/bpf/verifier.c:prepare_func_exit() {
	...
			caller->regs[regno] = callee->regs[regno];
			bpf_diag_mod_end(env);
		}
	}

	/* Invalidate callee-local dynptrs and their slices before the frame goes away. */
	err = destroy_dynptrs_in_stack_slots(env, callee, 0,
					     callee->allocated_stack / BPF_REG_SIZE - 1);
	...
}

On the revisit of BPF_FUNC_user_ringbuf_drain in check_helper_call(),
unwound_r0 is preserved across caller-saved register scrubbing without
invalidating the callback's dynptr id:

kernel/bpf/verifier.c:check_helper_call() {
	...
	if (env->cur_state->unwound) {
		regs[BPF_REG_0] = unwound_r0;
		goto unwound;
	}
	...
}

Wouldn't this leave the caller's unwind pad with a live CONST_PTR_TO_DYNPTR
pointing to the popped C stack frame of bpf_user_ringbuf_drain(), or a live
PTR_TO_MEM slice pointing to the already-released user ringbuf sample?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=8