Re: [PATCH bpf-next 1/5] bpf: Support unwinding via r3

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: missing r3 unwind check for async and exception callbacks in process_bpf_exit_full()
--

commit 2eb5a83ebef2f014ae77160f4cea6d3a54b2f738
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Support unwinding via r3

This commit adds verifier and JIT support for subprograms annotated with the
BTF decl_tag "unwind", which use r3 (BPF_REG_UNWIND) at exit to indicate
whether they return normally or unwind to a caller's landing pad.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2c812a5f1da87..7f1aefafa0b9b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -11961,6 +11983,16 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
>  					       "At callback return", "R0");
>  			return -EINVAL;
>  		}
> +		/* the helper that called it does not look at r3 */
> +		if (ret_unwind) {
> +			if (!bpf_register_is_null(&callee->regs[BPF_REG_UNWIND])) {
> +				verbose(env, "callback cannot unwind\n");
> +				return -EINVAL;
> +			}
> +			err = mark_chain_precision(env, BPF_REG_UNWIND);
> +			if (err)
> +				return err;
> +		}
>  		if (!bpf_calls_callback(env, callee->callsite)) {
>  			verifier_bug(env, "in callback at %d, callsite %d !calls_callback",
>  				     *insn_idx, callee->callsite);

[Severity: Medium]
Does this check in prepare_func_exit() miss asynchronous callbacks and
exception callbacks that are annotated with the "unwind" BTF decl_tag?

When a subprogram is verified as an asynchronous callback (via push_async_cb()
with in_async_callback_fn = true) or as an exception callback (via
do_check_common() with in_exception_callback_fn = true), it runs at
curframe == 0.

When BPF_EXIT is reached in process_bpf_exit_full():

kernel/bpf/verifier.c:process_bpf_exit_full() {
	...
	if (env->cur_state->curframe) {
		/* exit from nested function */
		err = prepare_func_exit(env, &env->insn_idx);
		...
	}
	...
	if (cur_frame->subprogno &&
	    !cur_frame->in_async_callback_fn &&
	    !cur_frame->in_exception_callback_fn)
		err = check_global_subprog_return_code(env);
	else
		err = check_return_code(env, BPF_REG_0, "R0");
}

Both prepare_func_exit() and check_global_subprog_return_code() are skipped,
and check_return_code() only validates R0:

kernel/bpf/verifier.c:check_return_code() {
	...
	if (frame->in_async_callback_fn) {
		exit_ctx = "At async callback return";
		range = frame->callback_ret_range;
		goto enforce_retval;
	}
}

Because llvm lowers unwind paths to r0 = 0; r3 = 1; exit, R0 satisfies the
callback return range while BPF_REG_UNWIND (R3) is never checked in
check_return_code(). Meanwhile, bpf_compute_live_registers() marks
BPF_REG_UNWIND as live at BPF_EXIT for all subprog > 0 with ret_unwind set.

Should check_return_code() (or process_bpf_exit_full()) also enforce that
BPF_REG_UNWIND is 0 when subprog_info(env, frame->subprogno)->ret_unwind is
set for async and exception callbacks?

[ ... ]

> @@ -19335,6 +19383,12 @@ static int check_global_subprog_return_code(struct bpf_verifier_env *env)
>  	u32 i, nregs;
>  	int err;
>  
> +	if (subprog_info(env, subprog)->ret_unwind) {
> +		err = check_global_ret_scalar_reg(env, BPF_REG_UNWIND);
> +		if (err)
> +			return err;
> +	}
> +
>  	if (subprog_returns_void(env, subprog))
>  		return 0;
>  

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018010246.1643877-1-alexei.starovoitov@gmail.com?part=1