Re: [PATCH bpf-next v7 07/22] bpf: Resume a covered call at its landing pad

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: landing pad state desync via discarded memory mutations allows OOB access
--

commit 41b3c5e14924fae7175095b7012838a14c2e5ffc
Author: Yonghong Song <yonghong.song@linux.dev>

bpf: Resume a covered call at its landing pad

This commit allows a covered call to resume at its landing pad when bpf_unwind()
is invoked by a subprogram. It pushes a cleanup pad branch with the caller's
pre-call state and handles bpf_unwind() by returning PROCESS_BPF_EXIT for
nested frames.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fc3df452de2e4..ee074d4a936b5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -19167,6 +19186,57 @@ enum {
>  	INSN_IDX_UPDATED = 2,
>  };
>  
> +static int push_cleanup_pad_branch(struct bpf_verifier_env *env, int insn_idx)
> +{
> +	struct bpf_verifier_state *branch;
> +	struct bpf_func_state *frame;
> +	int pad = bpf_exc_pad_of_call(env, insn_idx);
> +
> +	if (pad < 0)
> +		return 0;
> +	branch = push_stack(env, pad, insn_idx, false);
> +	if (IS_ERR(branch))
> +		return PTR_ERR(branch);
> +	frame = branch->frame[branch->curframe];
> +	/*
> +	 * The state at that call with the caller-saved registers gone: the
> +	 * callee's epilogue put r6-r9 and the stack back on the way out.
> +	 */
> +	clear_caller_saved_regs(env, frame->regs);
> +	mark_reg_unknown(env, frame->regs, BPF_REG_0);
> +	return 0;
> +}
> +
> +static int process_bpf_unwind(struct bpf_verifier_env *env, int *insn_idx,
> +			      bool *do_print_state)
> +{
> +	struct bpf_func_state *frame = cur_func(env);
> +	int pad = bpf_exc_pad_of_call(env, *insn_idx);
> +	int err;
> +
> +	if (pad < 0) {
> +		err = check_resource_leak(env, false, !env->cur_state->curframe,
> +					  "an unwind with no landing pad");
> +		if (err)
> +			return err;
> +		if (env->cur_state->curframe)
> +			return PROCESS_BPF_EXIT;

[Severity: Critical]
When process_bpf_unwind() returns PROCESS_BPF_EXIT here, the verifier stops
exploring the callee and discards its state. Does this discard any memory
mutations made by the callee to pointer arguments before calling bpf_unwind()?

Since the CPU unwinds the stack without undoing memory writes at runtime,
could this desynchronize the verifier state from runtime state and allow
out-of-bounds memory accesses?

> +		/*
> +		 * The main program's frame returns at once, which is the
> +		 * program returning. Mark r0 the zero the fixups leave after
> +		 * the call, and leave through the exit, which is what holds
> +		 * that zero to the program type.
> +		 */
> +		mark_reg_unknown(env, cur_regs(env), BPF_REG_0);
> +		mark_reg_known_zero(env, cur_regs(env), BPF_REG_0);
> +		return process_bpf_exit_full(env, do_print_state, false);
> +	}
> +	clear_caller_saved_regs(env, frame->regs);
> +	mark_reg_unknown(env, frame->regs, BPF_REG_0);
> +	*insn_idx = pad;
> +	return INSN_IDX_UPDATED;
> +}
> +

[ ... ]

> @@ -19421,7 +19491,29 @@ static int do_check_insn(struct bpf_verifier_env *env, bool *do_print_state)
>  					return -EINVAL;
>  				}
>  			}
> +			if (bpf_is_unwind_kfunc(insn) || bpf_is_unwind_resume_kfunc(insn)) {
> +				err = check_kfunc_allowed_only(env, insn, env->insn_idx);
> +				if (err)
> +					return err;
> +				if (bpf_is_unwind_kfunc(insn))
> +					return process_bpf_unwind(env, &env->insn_idx,
> +								  do_print_state);
> +				/*
> +				 * Mark r0 a known zero -- unknown first, as
> +				 * the known-zero helper keeps the type it
> +				 * finds, which here is NOT_INIT. The fixups
> +				 * lower this to 'r0 = 0; exit', so the frame
> +				 * returns a real zero.
> +				 */
> +				mark_reg_unknown(env, cur_regs(env), BPF_REG_0);
> +				mark_reg_known_zero(env, cur_regs(env), BPF_REG_0);
> +				return process_bpf_exit_full(env, do_print_state, false);
> +			}
>  			mark_reg_scratched(env, BPF_REG_0);
> +			/* An unwind out of this call resumes at the pad. */
> +			err = push_cleanup_pad_branch(env, env->insn_idx);
> +			if (err)
> +				return err;

[Severity: Critical]
push_cleanup_pad_branch() pushes a branch using a snapshot of the pre-call
state. If the callee modified a stack variable (e.g., an array index) and then
unwound the stack, does the verifier evaluate the landing pad with the original
unmodified value?

Could a BPF program use this to bypass bounds checks by mutating an index in
the callee before unwinding, causing the verifier to optimize out the checks
in the landing pad while using the out-of-bounds value at runtime?

>  			if (bpf_in_stack_arg_cnt(&env->subprog_info[cur_func(env)->subprogno]))
>  				cur_func(env)->no_stack_arg_load = true;
>  			if (bpf_is_callx(insn))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929001601.3242665-1-yonghong.song@linux.dev?part=7