Re: [PATCH bpf-next v1 17/29] bpf: Apply hidden slot limits only when a slot is needed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] bpf: multi-resource check bypass for fixed locks in
  bpf_frame_desc_resolve()
--

commit 744e9327531c3db28e3b5167eeaeeddb7a4859e0
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Apply hidden slot limits only when a slot is needed

This commit defers hidden stack slot capability checks from the main
verification pass to bpf_frame_desc_resolve(), rejecting incompatible
acquisitions and releases only when the resource actually requires a hidden
slot.

> diff --git a/kernel/bpf/frame_desc.c b/kernel/bpf/frame_desc.c
> index 72dc6597321a9..0ef7fda410ee1 100644
> --- a/kernel/bpf/frame_desc.c
> +++ b/kernel/bpf/frame_desc.c
[ ... ]
> @@ -507,7 +511,9 @@ static int acq_add(struct bpf_verifier_env *env, u32 insn, u16 res, u8 reg, u8 f
>  
>  /*
>   * refs[@idx] is what acquisition @acq acquired, held by the frame that owns
> - * it: its handle is in the slot of the acquisition, and the slot holds one.
> + * it. The slot of an acquisition holds one handle: a frame that holds two of
> + * what it acquired at once is fine as long as the acquisition gets no slot,
> + * see bpf_frame_desc_resolve().
>   */
>  static int ref_set_acq(struct bpf_verifier_env *env, struct bpf_verifier_state *st, int idx,
>  		       int acq)
> @@ -518,9 +524,8 @@ static int ref_set_acq(struct bpf_verifier_env *env, struct bpf_verifier_state *
>  	for (i = 0; i < st->acquired_refs; i++) {
>  		if (i == idx || st->refs[i].acq != acq + 1 || st->refs[i].frameno != ref->frameno)
>  			continue;
> -		verbose(env, "frame_desc: a frame holds two resources of insn %d at once, which is not supported in a program that throws\n",
> -			env->res_acqs[acq].insn);
> -		return -EINVAL;
> +		env->res_acqs[acq].multi = true;
> +		break;
>  	}
>  	ref->res = env->res_acqs[acq].res + 1;
>  	ref->acq = acq + 1;
[ ... ]
> @@ -1149,6 +1214,15 @@ int bpf_frame_desc_resolve(struct bpf_verifier_env *env)
>  		}
>  		if (!a->slot)
>  			continue;
> +		ret = slot_can_be_kept(env, i);
> +		if (ret)
> +			break;
> +		if (a->multi) {
> +			verbose(env, "frame_desc: a frame holds two resources of insn %d at once, which is not supported in a program that throws\n",
> +				a->insn);
> +			ret = -EINVAL;
> +			break;
> +		}
>  		/* some visit of the insn has nothing a slot can hold where this one has */
>  		if (!(a->flags & BPF_RES_F_CTX) && acquires_sometimes(env, new_of[a->insn], a)) {
>  			verbose(env, "frame_desc: insn %d acquires a resource on some paths only, or one of a type that cannot be released\n",

[Severity: High]
Can a fixed-address lock bypass the a->multi check when a throw site sees
two references on one path and none on another?

For a bpf_res_spin_lock in a single-element array map, two lookups give
distinct reg->id values for the same lock address, so calling
bpf_res_spin_lock() at the same instruction with both pointers records two
references with the same ref->acq and sets a->multi = true.  When that
state reaches a throw site, ref_locations() sets cur.fixed = true and
cur.cnt = 1 for both references without checking ref->id, and site_visit()
increments loc->visits once per matching reference in st->acquired_refs:

kernel/bpf/frame_desc.c:site_visit() {
    ...
	site->visits++;
	for (i = 0; i < st->acquired_refs; i++) {
    ...
		loc = &env->res_locs[j];
		loc->visits++;
}

If a second path reaches the same throw site holding neither reference,
loc->visits and site->visits are both 2 with loc->cnt == 1, so a->slot
stays false.  bpf_frame_desc_resolve() then skips the a->multi check, and
build_site() emits a BPF_FRAME_ENT_FIXED descriptor entry that unlocks the
unheld lock when unwinding from that site on the second path.

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