Re: [PATCH bpf-next v1 20/29] bpf: Transfer a reference to the callee that releases it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] bpf: reference transfer via dead args in
  bpf_frame_desc_record_call()
- [Medium] bpf: wrong consumed arg in foreign_release() when regs share
  ref->id
--

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

bpf: Transfer a reference to the callee that releases it

This commit transfers references passed in argument registers from the caller
to a subprog that releases them, tracking the acquired reference in the
callee's frame descriptor or hidden stack slot and returning it if unreleased.

> diff --git a/kernel/bpf/frame_desc.c b/kernel/bpf/frame_desc.c
> index d2ad02925f061..6679f134b14ee 100644
> --- a/kernel/bpf/frame_desc.c
> +++ b/kernel/bpf/frame_desc.c
[ ... ]
> @@ -736,7 +742,7 @@ static int foreign_release(struct bpf_verifier_env *env, const struct bpf_verifi
>  	const struct bpf_func_state *below = st->frame[ref->frameno + 1];
>  	struct bpf_subprog_info *sub;
>  	struct bpf_res_call *c;
> -	int f;
> +	int f, k;
>  
>  	for (f = ref->frameno + 1; f <= st->curframe; f++) {
>  		sub = &env->subprog_info[st->frame[f]->subprogno];
> @@ -745,6 +751,21 @@ static int foreign_release(struct bpf_verifier_env *env, const struct bpf_verifi
>  		sub->releases_inherited = true;
>  		env->frame_desc_redo = true;
>  	}
> +	/* every frame it was passed down through in a register is handed it by the one below */
> +	for (f = ref->frameno + 1; f <= st->curframe; f++) {
> +		if (!ref->id || ref->type != REF_TYPE_PTR || (acq->flags & BPF_RES_F_ARG))
> +			break;
> +		for (k = BPF_REG_1; k <= BPF_REG_5; k++)
> +			if (st->frame[f]->entry_arg_id[k - 1] == ref->id)
> +				break;
> +		if (k > BPF_REG_5)
> +			break;
> +		sub = &env->subprog_info[st->frame[f]->subprogno];
> +		if (sub->consumes & BIT(k))
> +			continue;
> +		sub->consumes |= BIT(k);
> +		env->frame_desc_redo = true;
> +	}

[Severity: Medium]
Can foreign_release() mark the wrong argument register in sub->consumes when
two arguments at subprog entry share ref->id?

bpf_frame_desc_record_call() records reg->id in callee->entry_arg_id[k - 1]
for every non-scalar argument without checking reg_ref_off(), and
foreign_release() picks the lowest register k matching ref->id and breaks. If
a caller passes a pointer sharing ref->id in R1 that fails reg_ref_off() (such
as a bpf_skc_to_mptcp_sock() result or a variable-offset ringbuf pointer) and
passes the base reference in R2 that the subprog actually releases,
foreign_release() marks R1 instead of R2. On the next round,
bpf_frame_desc_record_call() fails reg_ref_off() on R1, sets entry_none on R1,
and never transfers R2.

Even when reg_ref_off() succeeds on both registers (for example, one call passes
(&p1->field, p1) where the subprog releases R2, and another call passes
(&p2->field, p3) while keeping p2 in a hidden slot), matching R1 first causes
the second call to transfer and hand back p2 on R1, and once both R1 and R2
are in sub->consumes, transferring p1 on R1 at the first call moves
ref->frameno to the callee so R2 finds no caller reference and sets
entry_none. bpf_frame_desc_resolve() then rejects the program.

[ ... ]
> @@ -869,6 +916,71 @@ int bpf_frame_desc_record_exit(struct bpf_verifier_env *env)
>  	return 0;
>  }
>  
> +/*
> + * Frame @callee of @st is entered with its arguments in place, by the call at
> + * insn @callsite of the frame below it when @st is given; a callback, a
> + * callx and a function verified on its own come with none.
[ ... ]
> + */
> +int bpf_frame_desc_record_call(struct bpf_verifier_env *env, struct bpf_verifier_state *st,
> +			       struct bpf_func_state *callee, int callsite)
> +{
> +	struct bpf_subprog_info *sub = &env->subprog_info[callee->subprogno];
> +	struct bpf_reference_state *ref;
> +	int i, k, acq, err;
> +	s16 adj;
> +
> +	if (!env->describe_frames || (st && st->speculative))
> +		return 0;
> +	for (k = BPF_REG_1; k <= BPF_REG_5; k++) {
> +		struct bpf_reg_state *reg = &callee->regs[k];
> +
> +		callee->entry_arg_id[k - 1] = 0;
> +		callee->entry_arg_acq[k - 1] = 0;
> +		if (reg->type == SCALAR_VALUE || reg->type == NOT_INIT) {
> +			if ((sub->consumes & BIT(k)) && !bpf_register_is_null(reg))
> +				sub->entry_none |= BIT(k);
> +			continue;
> +		}
> +		callee->entry_arg_id[k - 1] = reg->id;
> +		if (!(sub->consumes & BIT(k)))
> +			continue;
> +		for (i = 0, ref = NULL; st && i < st->acquired_refs; i++) {
> +			ref = &st->refs[i];
> +			if (ref->frameno == callee->frameno - 1 && ref->acq && ref->type == REF_TYPE_PTR &&
> +			    !(env->res_acqs[ref->acq - 1].flags & BPF_RES_F_ARG) &&
> +			    reg_ref_off(reg, ref->id, &adj))
> +				break;
> +			ref = NULL;
> +		}
> +		if (!ref) {
> +			sub->entry_none |= BIT(k);
> +			continue;
> +		}
> +		err = rel_add(env, callsite, ref->acq - 1, k, 0, adj, 0);
> +		if (err)
> +			return err;
> +		acq = acq_add(env, sub->start, ref->res - 1, k, BPF_RES_F_ENTRY, adj, 0);
> +		if (acq < 0)
> +			return acq;
> +		callee->entry_arg_acq[k - 1] = ref->acq;
> +		ref->frameno = callee->frameno;
> +		ref->acq = acq + 1;
> +	}
> +	return 0;
> +}

[Severity: High]
Can bpf_frame_desc_record_call() transfer a reference through an argument
register that is dead at sub->start?

In setup_func_entry(), set_callee_state() copies R1..R5 from the caller to the
callee without filtering by env->insn_aux_data[sub->start].live_regs_before,
and bpf_frame_desc_record_call() runs before bpf_clean_verifier_state() scrubs
dead registers at sub->start. If an unused argument register Rk still holds a
copy of the caller's referenced pointer at the callsite (for example, R1 still
holds a copy of the pointer while the subprog only reads and releases R2),
foreign_release() sees entry_arg_id[k - 1] == ref->id and sets BIT(k) in
sub->consumes. On the next round, bpf_frame_desc_record_call() transfers the
reference via the dead register Rk, emitting an ITEM_RELEASE before the call
that reads Rk and an entry store at sub->start that writes Rk into the
callee's hidden slot.

Because Rk is not in live_regs_before at the callsite or sub->start and is not
in env->frame_desc_regs, bpf_do_misc_fixups() can pick Rk as a scratch
register when expanding a preceding timed_may_goto in the caller, overwriting
Rk at runtime with the per-CPU struct bpf_timed_may_goto pointer:

kernel/bpf/fixups.c:bpf_do_misc_fixups() {
    ...
			u16 live = env->insn_aux_data[i].live_regs_before | env->frame_desc_regs;
			int reg, save_off = 0;

			/* a register that is dead here holds the address of the count */
			for (reg = BPF_REG_0; reg <= BPF_REG_9; reg++)
				if (!(live & BIT(reg)))
					break;
    ...
}

At runtime, the caller's ITEM_RELEASE compares its hidden slot against the
clobbered Rk and fails to clear the caller's slot, while the callee's entry
stores the per-CPU timed_may_goto address into the callee's hidden slot. When
the callee later releases the real pointer via R2 and throws, the unwinder
passes the per-CPU timed_may_goto pointer from the callee's slot to the
resource destructor and then double frees the object from the caller's
uncleared slot. Checking env->insn_aux_data[sub->start].live_regs_before &
BIT(k) in bpf_frame_desc_record_call() would ignore registers that the callee
never reads.

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