Re: [PATCH bpf-next v1 18/29] bpf: Allow a callee to leave resources with its caller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] bpf: cross-frame unwind ordering bug in
  bpf_frame_desc_record_acquire()
- [Medium] bpf: multi-resource tracking bypass in
  bpf_frame_desc_record_return()
--

commit 48230df87bf3a57835f6fce92009993e3192e84d
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Allow a callee to leave resources with its caller

This commit allows BPF subprograms in programs that throw to leave held
resources, caller-stack objects, or RCU and preemption section deltas with
their caller unless a hidden stack slot is required to track them.

> diff --git a/kernel/bpf/frame_desc.c b/kernel/bpf/frame_desc.c
> index 0ef7fda410ee1..9854be9ab4781 100644
> --- a/kernel/bpf/frame_desc.c
> +++ b/kernel/bpf/frame_desc.c
[ ... ]
> @@ -547,30 +554,37 @@ int bpf_frame_desc_record_acquire(struct bpf_verifier_env *env, int idx, int reg
>  	/* no insns can put it into a slot then: R10 stands for no register */
>  	if (regno < BPF_REG_0 || regno > BPF_REG_5)
>  		regno = BPF_REG_FP;
>  	acq = acq_add(env, env->insn_idx, res, regno, flags, adj, obj_off);
> -	return acq < 0 ? acq : ref_set_acq(env, st, idx, acq);
> +	if (acq < 0)
> +		return acq;
> +	/*
> +	 * A stack object is held by the frame that has it in its stack, which
> +	 * its sites find it in: the slot of a callee that created it there
> +	 * would be gone with the callee.
> +	 */
> +	if (owner != st->curframe) {
> +		ref->frameno = owner;
> +		if (!env->res_acqs[acq].elsewhere)
> +			env->res_acqs[acq].elsewhere = env->insn_idx + 1;
> +	}
> +	return ref_set_acq(env, st, idx, acq);
>  }

[Severity: High]
Can assigning ref->frameno = owner here reverse the unwind release order
when the callee throws before returning?

In bpf_unwind_release_frame(), the ordering passes (releasing
BPF_FRAME_ENT_FIRST iterators and file dynptrs before regular objects in
BPF_UNWIND_PHASE_OBJS, and restoring disabled IRQ flags before enabled IRQ
flags in BPF_UNWIND_PHASE_LOCKS) run per frame as the unwinder walks from
the innermost callee frame to the outermost caller frame:

kernel/bpf/unwind.c:bpf_unwind_release_frame() {
    ...
	for (pass = 0; pass < passes; pass++) {
		for (first = 1; first >= 0; first--) {
			bpf_unwind_release_desc(ctx, pool, d, fp, first, pass);
			bpf_unwind_release_desc(ctx, pool, site, fp, first, pass);
		}
	}
}

If a callee acquires a referenced object (such as a cgroup via
bpf_cgroup_from_id()) in its own frame, initializes a borrowing iterator
(such as bpf_iter_css_task_new()) on the caller's stack, and throws before
returning, the callee frame is unwound first and drops the backing cgroup
reference before the caller frame unwinds and destroys the iterator,
causing a use-after-free.  Similarly, if a callee saves IRQ flags on its
own stack while interrupts are enabled and then saves nested IRQ flags on
the caller's stack while interrupts are disabled before throwing, unwinding
the callee frame first enables interrupts and unwinding the caller frame
second restores the disabled state, leaving local CPU interrupts disabled.

[ ... ]

> @@ -748,71 +762,89 @@ int bpf_frame_desc_record_section(struct bpf_verifier_env *env, u8 kind, s8 delt
[ ... ]
>  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;
> +	struct bpf_res_acq *held;
>  	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;
> +	if (ref && !ref->res)
> +		ref = NULL;
> +	if (ref && ref->type == REF_TYPE_PTR && ref->id &&
> +	    !(env->res_acqs[ref->acq - 1].flags & BPF_RES_F_ARG) &&
> +	    reg_ref_off(r0, ref->id, &adj)) {
> +		acq = acq_add(env, callsite, ref->res - 1, BPF_REG_0, 0, adj, 0);
> +		return acq < 0 ? acq : ref_set_acq(env, st, idx, acq);
>  	}
> -	if (!reg_ref_off(r0, ref->id, &adj)) {
> -		verbose(env, "frame_desc: the subprog called at insn %d returns holding what R0 does not carry, which is not supported in a program that throws\n",
> -			callsite);
> -		return -EINVAL;
> +	if (r0->type != SCALAR_VALUE || !tnum_equals_const(r0->var_off, 0))
> +		env->insn_aux_data[callsite].res_none = true;
> +	if (ref) {
> +		held = &env->res_acqs[ref->acq - 1];
> +		if (!held->elsewhere)
> +			held->elsewhere = callsite + 1;
>  	}
> -	acq = acq_add(env, callsite, ref->res - 1, BPF_REG_0, 0, adj, 0);
> -	return acq < 0 ? acq : ref_set_acq(env, st, idx, acq);
> +	return 0;
>  }

[Severity: Medium]
When a subprog returns holding a non-R0 resource, does
bpf_frame_desc_record_return() miss setting held->multi if the caller frame
already holds a resource from the same acquisition?

If a caller invokes a lock-taking subprog twice, the first lock's
ref->frameno is already updated to the caller when the second call runs
ref_set_acq() in the callee frame, so ref_set_acq() does not set
env->res_acqs[acq].multi.  When the second call returns, the caller frame
holds two references with the same ref->acq, and in site_visit() both
references match the same env->res_locs[j] entry and increment loc->visits
twice for a single increment of site->visits:

kernel/bpf/frame_desc.c:site_visit() {
    ...
		for (j = 0; j < env->res_loc_cnt; j++)
			if (env->res_locs[j].site == s && env->res_locs[j].acq == ref->acq - 1)
				break;
		if (j == env->res_loc_cnt) {
    ...
			cur.visits = 1;
			loc[j] = cur;
			env->res_loc_cnt = j + 1;
			continue;
		}
		/* the places this visit finds the handle in as well */
		loc = &env->res_locs[j];
		loc->visits++;
}

If the resource has loc->fixed set (such as a resilient spin lock in a
1-element array map acquired via two distinct map-lookup pointers on one
path) and another path reaches the same throw site holding neither,
site->visits (2) equals loc->visits (2) with loc->cnt == 1, so
bpf_frame_desc_resolve() leaves a->slot false, skips the a->elsewhere and
a->multi checks, and emits a site descriptor that unlocks an unheld lock if
the second path throws.  Calling ref_set_acq(env, st, idx, ref->acq - 1) on
the non-R0 return path and checking a->multi even when !a->slot would catch
this.

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