Re: [PATCH bpf-next v1 13/29] bpf: Track held resources in hidden stack slots

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] bpf: emit_entries() hides arena stack adjustment from recursion
  check
- [High] bpf: RCU iterator outlives section when subprog unlocks and
  throws
- [High] bpf: double-free on unwind due to wrong slot cleared in
  emit_release()
- [High] bpf: non-LIFO lock unwind in bpf_frame_desc_build() corrupts
  IRQ and lock state
- [High] bpf: hidden slot overwritten when subprog call returns NULL in
  frame_desc
- [Medium] bpf: duplicate check_max_stack_depth() inflates recursive
  frame depth
--

commit 197c825766b69a79fd4f8ec64eb1bed1f952135c
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Track held resources in hidden stack slots

This commit tracks held kernel resources, locks, and RCU or preemption
sections in hidden stack slots and builds per-subprogram frame descriptors so
bpf_throw() can release them during unwinding.

> diff --git a/kernel/bpf/frame_desc.c b/kernel/bpf/frame_desc.c
> new file mode 100644
> index 0000000000000..b0db3feeb9b0f
> --- /dev/null
> +++ b/kernel/bpf/frame_desc.c

[ ... ]

> +int bpf_frame_desc_record_return(struct bpf_verifier_env *env, struct bpf_verifier_state *st,
> +				 int idx, int callsite, const struct bpf_reg_state *r0)
> +{
> +	struct bpf_reference_state *ref;
> +	s16 adj = 0;
> +	int acq;
> +
> +	if (!env->describe_frames || st->speculative)
> +		return 0;
> +	ref = idx < 0 ? NULL : &st->refs[idx];
> +	if (!ref || !ref->res) {
> +		if (r0->type != SCALAR_VALUE || !tnum_equals_const(r0->var_off, 0))
> +			env->insn_aux_data[callsite].res_none = true;
> +		return 0;
> +	}

[Severity: High]
Can a second call at the same callsite that returns NULL overwrite a live
handle in the callsite's hidden stack slot?

emit_acquire_after() unconditionally stores R0 into the callsite's slot
after every return.  If a subprog can return either an acquired reference or
NULL (R0 == 0, idx < 0), and the caller invokes that callsite a second time
while still holding the reference from the first call,
bpf_frame_desc_record_return() returns 0 without checking whether st->refs[]
already holds a reference from that callsite.  At runtime, the second return
writes 0 into the hidden slot, so a later bpf_throw() sees NULL and leaks the
object from the first call.

[ ... ]

> +static int emit_release(struct bpf_verifier_env *env, struct bpf_insn *buf,
> +			const struct res_item *it, int n)
> +{
> +	const struct bpf_res_rel *r = &env->res_rels[it->rec];
> +	int i, cnt = 0, reg = r->reg, per = r->adj ? 5 : 4;
> +
> +	if (r->flags & BPF_RES_F_DEREF) {
> +		buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_0, r->reg, 0);
> +		reg = BPF_REG_0;
> +	}
> +	for (i = 0; i < n; i++) {
> +		s16 off = env->res_acqs[env->res_rels[it[i].rec].acq].off;
> +		bool last = i == n - 1;
> +
> +		buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_AX, BPF_REG_FP, off);
> +		if (r->adj)
> +			buf[cnt++] = BPF_ALU64_IMM(BPF_ADD, BPF_REG_AX, r->adj);
> +		buf[cnt++] = BPF_JMP_REG(BPF_JNE, BPF_REG_AX, reg, last ? 1 : 2);
> +		buf[cnt++] = BPF_ST_MEM(BPF_DW, BPF_REG_FP, off, 0);
> +		/* past the slots that follow: the last one has no jump */
> +		if (!last)
> +			buf[cnt++] = BPF_JMP_A((n - 1 - i) * per - 1);
> +	}
> +	return cnt;
> +}

[Severity: High]
Can emit_release() clear the wrong hidden slot when a frame holds two
references to the same underlying pointer?

Suppose a frame acquires a = bpf_obj_new(...) (acq 0, slot0) and
b = bpf_refcount_acquire(a) (acq 1, slot1), so both slots hold the same
pointer value p.  If a first release call R_A takes a on one path and b on
another, R_A is recorded with both acq 0 and acq 1.  On the path that passes
b to R_A and then passes a to a second release call R_B, the verifier removes
b's ref at R_A and records only acq 0 at R_B.

At runtime on that path, R_A checks slot0 first, sees slot0 == p, clears
slot0, and jumps past slot1.  R_B then only checks slot0 (already 0), leaving
p in slot1 after both references have been dropped, so a subsequent
bpf_throw() releases p a third time and causes a double free or refcount
underflow.

[ ... ]

> +static int emit_entries(struct bpf_verifier_env *env)
> +{

[ ... ]

> +		for (k = 0; k < ARRAY_SIZE(sub->section_off); k++)
> +			if (sub->section_off[k])
> +				buf[cnt++] = BPF_ST_MEM(BPF_DW, BPF_REG_FP, sub->section_off[k], 0);
> +		if (!cnt)
> +			continue;
> +		ret = bpf_patch_list_insert(env, sub->start, buf, cnt);
> +		env->frame_desc_insns += cnt;
> +	}
> +	ret = ret ?: bpf_patch_list_commit(env);

[Severity: High]
Does inserting slot-zeroing instructions at sub->start hide a recursive
subprog's arena stack adjustment from bpf_insert_recursion_checks()?

bpf_frame_desc_emit() runs before bpf_insert_recursion_checks() and commits
the prologue stores at sub->start, which moves sub->start in front of the
subprog's leading is_arena_sp_adj() instruction:

kernel/bpf/fixups.c:bpf_insert_recursion_checks() {
    ...
		/* The exit drops the frame in the stack in arena. Take it before the check. */
		if (is_arena_sp_adj(&env->prog->insnsi[si[s].start])) {
			buf[n++] = env->prog->insnsi[si[s].start];
    ...
}

Because si[s].start is now a BPF_ST_MEM instruction instead of
is_arena_sp_adj(), bpf_insert_recursion_checks() does not hoist the arena
frame allocation ahead of the recursion check's bailout exit (and also looks
up live_regs_before at the wrong offset).  bpf_insert_arena_stack() still
prepends r12 -= arena_stack_depth to every BPF_EXIT in the subprog, including
the bailout exit, so hitting the recursion or may_goto budget check decrements
BPF_REG_ARENA_SP without the matching entry increment and corrupts the
caller's arena stack pointer.

[ ... ]

> +int bpf_frame_desc_build(struct bpf_verifier_env *env)
> +{

[ ... ]

> +		for (i = 0; i < 2 * env->res_acq_cnt; i++) {
> +			const struct bpf_res_acq *a = &env->res_acqs[i % env->res_acq_cnt];
> +			bool first = i < env->res_acq_cnt;
> +
> +			if (acq_subprog(env, a, new_of) != s ||
> +			    resource_goes_first(&env->resources[a->res]) != first)
> +				continue;
> +			d->ent[d->nent++] = (struct bpf_frame_desc_entry) { .res = a->res, .off = a->off };
> +		}

[Severity: High]
Should bpf_frame_desc_build() emit lock and IRQ flag entries in reverse
acquisition order?

d->ent[] is populated in forward env->res_acqs[] order, and
bpf_unwind_release_frame() walks d->ent[] forward during
BPF_UNWIND_PHASE_LOCKS.  If a frame entered with interrupts enabled calls
bpf_spin_lock() (saving IF=1 in per-CPU irqsave_flags and disabling IRQs),
then calls bpf_res_spin_lock_irqsave() (saving IF=0 into the stack flag slot),
and throws, pass 0 of BPF_UNWIND_PHASE_LOCKS first runs bpf_spin_unlock(),
which restores IF=1 while the resilient spinlock is still held, and then runs
bpf_local_irq_restore() on the stack slot (since arch_irqs_disabled_flags()
is true, late is false), leaving local CPU interrupts disabled during sleeping
destructors in BPF_UNWIND_PHASE_OBJS and after bpf_throw() returns.

When a frame holds two nested resilient spinlocks, unlocking them in forward
order also pops the wrong entry in res_spin_unlock() (which clears
rqh->locks[rqh->cnt - 1]), corrupting the per-CPU held-lock stack.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index cf3cfc734ed4d..4d85a42a18d5f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -11232,21 +11321,34 @@ static void invalidate_non_owning_refs(struct bpf_verifier_env *env)
>  	}));
>  }
>  
> -static void invalidate_rcu_protected_refs(struct bpf_verifier_env *env)
> +static int invalidate_rcu_protected_refs(struct bpf_verifier_env *env)
>  {
>  	struct bpf_stack_state *stack;
>  	struct bpf_func_state *state;
>  	struct bpf_reg_state *reg;
>  	u32 clear_mask = (1 << STACK_SPILL) | (1 << STACK_ITER);
> +	bool iter = false;
>  
>  	bpf_for_each_reg_in_vstate_mask(env->cur_state, state, reg, stack, clear_mask, ({
>  		if (reg->type & MEM_RCU) {
> +			iter |= stack && stack->slot_type[BPF_REG_SIZE - 1] == STACK_ITER;
>  			bpf_diag_mod_begin(env, reg, NULL, BPF_DIAG_MOD_WRITE);
>  			reg->type &= ~(MEM_RCU | PTR_MAYBE_NULL | NON_OWN_REF);
>  			reg->type |= PTR_UNTRUSTED;
>  			bpf_diag_mod_end(env);
>  		}
>  	}));
> +	/*
> +	 * Nothing may destroy an RCU protected iterator once its section ended:
> +	 * not the program, which is refused when it leaks one, and not the
> +	 * unwinder, which destroys what a frame holds at a throw, see
> +	 * frame_desc.c.
> +	 */
> +	if (iter && env->describe_frames) {
> +		verbose(env, "an RCU protected iterator may not outlive its section in a program that throws\n");
> +		return -EINVAL;
> +	}
> +	return 0;
>  }

[Severity: High]
Does this miss RCU-protected iterators on caller frames when a subprog is
verified on its own and calls bpf_rcu_read_unlock() followed by bpf_throw()?

When call_func_on_its_own() verifies a subprog, the caller frames are not in
env->cur_state->frame[], so bpf_for_each_reg_in_vstate_mask() does not see a
caller's STACK_ITER slot (while env->cur_state->refs[] carries the inherited
iterator reference with frameno == -1).  If the subprog then calls
bpf_throw() instead of returning normally, process_bpf_exit_full() skips
exit_func_on_its_own(), so bpf_frame_desc_record_exit() never checks
rcu_released_below and bpf_flow_return() never sets releases_inherited.  The
subprog can then drop the caller's RCU lock (and sleep in a sleepable prog)
before throwing, causing the unwinder to destroy the caller's RCU-protected
iterator after its RCU section has ended.

[ ... ]

> @@ -25600,6 +25809,21 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr,
>  	if (ret == 0)
>  		ret = check_max_stack_depth(env);
>  
> +	/*
> +	 * The pass has seen what every frame holds and the stack is final:
> +	 * reserve the slots the frames keep their handles in and emit the insns
> +	 * that keep them up to date, with the insns still those the main pass
> +	 * recorded.
> +	 */
> +	if (ret == 0)
> +		ret = bpf_frame_desc_resolve(env);
> +	if (ret == 0)
> +		ret = bpf_frame_desc_emit(env);
> +
> +	/* Account for the slots reserved by frame descriptor resolution. */
> +	if (ret == 0)
> +		ret = check_max_stack_depth(env);

[Severity: Medium]
Does calling check_max_stack_depth() both before and after
bpf_frame_desc_resolve() inflate stack_depth for recursive subprogs that use
the arena stack?

During the first check_max_stack_depth() call, pad_recursive_frames()
overwrites si[s].stack_depth in place with max(si[s].stack_depth, frame).
bpf_frame_desc_resolve() -> bpf_reserve_stack_slots() then reserves hidden
slots on top of that already-padded stack_depth instead of the verified stack
depth, which can push stack_depth past env->stack_limit on the second
check_max_stack_depth() call and cause false verifier rejections.

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