Re: [PATCH bpf-next v7 08/22] bpf: Require an unwind to leave a frame holding what it entered with

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: type confusion and memory corruption due to stale cleanup pad state
- [High] bpf: lock corruption and double-unlock via missing identity check on unwind
--

commit 585aa50d9e8811792b9a816a4d1ad6cd51d63ea7
Author: Yonghong Song <yonghong.song@linux.dev>
Subject: bpf: Require an unwind to leave a frame holding what it entered with

This commit updates the BPF verifier to record what locks and references
a program holds when a frame is entered. It enforces that an unwind must
leave the frame holding what it entered with, ensuring that landing pads
verified against pre-call snapshots remain valid.

> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 0143688896b0e..75e572a8a1bab 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -339,6 +339,18 @@ struct bpf_func_state {
>  	bool in_async_callback_fn;
>  	bool in_exception_callback_fn;
>  	bool no_stack_arg_load;
> +	/*
> +	 * What the program held when this frame was entered. An unwind leaves
> +	 * the frame without running anything below it, so the frame has to put
> +	 * these back to what it found before it goes -- otherwise a caller's
> +	 * landing pad, whose state was taken at the call, is wrong about them.
> +	 */
> +	u32 entry_active_locks;
> +	u32 entry_preempt_locks;
> +	u32 entry_rcu_locks;
> +	u32 entry_irq_id;
> +	u32 entry_id_gen;
> +	u32 entry_acquired_refs;

[Severity: High]
Does this structure need fields to record the identity of the lock held upon
frame entry? Without this, it might be difficult to accurately enforce the
correct lock state on unwind.

Additionally, is it necessary to update func_states_equal() to compare these
new entry_* fields? Without comparing them, could the verifier unsafely prune
states with different entry locking conditions?

[ ... ]
> diff --git a/kernel/bpf/exception.c b/kernel/bpf/exception.c
> index c1779d2d02f05..c0b0b8478af51 100644
> --- a/kernel/bpf/exception.c
> +++ b/kernel/bpf/exception.c
> @@ -12,6 +12,72 @@
>  BTF_ID_LIST_SINGLE(bpf_unwind_id, func, bpf_unwind)
>  BTF_ID_LIST_SINGLE(bpf_unwind_resume_id, func, bpf_unwind_resume)
>  
> +void bpf_exc_record_frame_entry(const struct bpf_verifier_state *state,
> +				struct bpf_func_state *frame, u32 id_gen)
> +{
[ ... ]
> +int bpf_exc_check_frame_balance(struct bpf_verifier_env *env, const char *prefix)
> +{
> +	const struct bpf_verifier_state *state = env->cur_state;
> +	const struct bpf_func_state *frame = cur_func(env);
> +	u32 i, held;
> +	const char *what;
> +
> +	if (state->active_rcu_locks != frame->entry_rcu_locks)
> +		what = "bpf_rcu_read_lock";
> +	else if (state->active_preempt_locks != frame->entry_preempt_locks)
> +		what = "bpf_preempt_disable";
> +	else if (state->active_irq_id != frame->entry_irq_id)
> +		what = "bpf_local_irq_save";
> +	else if (state->active_locks != frame->entry_active_locks)
> +		what = "bpf_spin_lock";

[Severity: High]
Does this code adequately verify lock identity? While it checks the count of
active locks, it appears to miss checking the identity of the lock
(active_lock_id and active_lock_ptr).

If a BPF program acquires lock A, passes it to a subprogram which unlocks A,
locks B, and then throws an exception (bpf_unwind()), could the unwinder
accept the frame balance since the active_locks count is still 1?

The exception would jump back to the caller's cleanup pad, which assumes
lock A is still held. The cleanup pad unlocking A could potentially cause a
double-unlock of lock A and leak lock B forever.

> +	else
> +		what = NULL;

[ ... ]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ee074d4a936b5..4bdee3f02fe9d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -19512,6 +19554,10 @@ static int do_check_insn(struct bpf_verifier_env *env, bool *do_print_state)
>  			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);

[Severity: Critical]
Does taking a pre-call snapshot of the verifier state for the cleanup pad
incorrectly ignore valid memory modifications made by the callee before
unwinding?

For example, if a caller spills a valid pointer to its stack and passes a
pointer to this stack slot to a subprogram:

- The verifier schedules the caller's cleanup pad branch by pushing a snapshot
  of the verifier state (including the stack) taken BEFORE the callee runs.
- The subprogram overwrites the caller's stack slot with an attacker-controlled
  scalar and calls bpf_unwind().
- At runtime, the exception jumps to the cleanup pad, which loads the scalar.

Because the cleanup pad uses the pre-call snapshot, does the verifier
incorrectly assume the loaded value is still a valid pointer, potentially
leading to type confusion and memory corruption?

> +			if (err)
> +				return err;
> +			/* Or, with no pad, leaves the frame for good. */
> +			err = check_unwind_through_call(env, env->insn_idx);
>  			if (err)
>  				return err;

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