Re: [PATCH bpf-next v1 21/29] bpf: Support callx when a callee releases resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] bpf: resource leak via bypassed lent check in
  bpf_frame_desc_record_callx()
- [Medium] bpf: hidden slot cleared without release in
  bpf_frame_desc_record_callx()
--

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

bpf: Support callx when a callee releases resources

This commit extends frame descriptor tracking to support callx instructions
whose target subprogs transfer or release caller-held resources, rejecting
programs when targets disagree on whether an argument reference is consumed.

> diff --git a/kernel/bpf/frame_desc.c b/kernel/bpf/frame_desc.c
> index 6679f134b14ee..11b2546bc179f 100644
> --- a/kernel/bpf/frame_desc.c
> +++ b/kernel/bpf/frame_desc.c
[ ... ]
> @@ -981,6 +983,56 @@ int bpf_frame_desc_record_call(struct bpf_verifier_env *env, struct bpf_verifier
>  	return 0;
>  }
>  
> +/*
> + * The callx at insn @callsite of the current frame calls @subprog, one of the
> + * subprogs it may call. What the frame puts in front of the callx runs
> + * whichever it calls: the insns that take what it hands over out of its slot,
> + * and the one that empties the slot of what a callee releases without being
> + * handed it, see foreign_cleared(). So note what this subprog leaves with the
> + * frame:
> + *
> + *  - What the frame holds and passes it in an argument that it does not
> + *    release through. Another subprog may be handed it in that argument, and
> + *    the slot would be emptied for this one as well, see slot_can_be_kept().
> + *  - All that the frame holds, when the subprog releases nothing of its
> + *    callers'. It is not verified in their context then, so its return has
> + *    no frame to say so, see bpf_frame_desc_record_exit().
> + */
> +int bpf_frame_desc_record_callx(struct bpf_verifier_env *env, int subprog, int callsite)
> +{
> +	const struct bpf_subprog_info *sub = &env->subprog_info[subprog];
> +	struct bpf_verifier_state *st = env->cur_state;
> +	const struct bpf_func_state *frame = st->frame[st->curframe];
> +	const struct bpf_reference_state *ref;
> +	struct bpf_res_call *c;
> +	int i, k;
> +	u8 lent;
> +	s16 adj;
> +
> +	if (!env->describe_frames || st->speculative)
> +		return 0;
> +	for (i = 0; i < st->acquired_refs; i++) {
> +		ref = &st->refs[i];
> +		if (ref->frameno != st->curframe || !ref->acq)
> +			continue;
> +		lent = 0;
> +		for (k = BPF_REG_1; k <= BPF_REG_5; k++)
> +			if (!(sub->consumes & BIT(k)) && ref->type == REF_TYPE_PTR &&
> +			    !(env->res_acqs[ref->acq - 1].flags & BPF_RES_F_ARG) &&
> +			    reg_ref_off(&frame->regs[k], ref->id, &adj))
> +				lent |= BIT(k);

[Severity: High]
Can a caller's hidden slot still be cleared before a callx to a subprog that
does not consume Rk, leaking the reference on a later bpf_throw()?

When one target of a callx consumes Rk, emit_release() emits instructions
before the callx that compare the 64-bit runtime value of Rk with the caller's
hidden slot and zero the slot if they match.  However,
bpf_frame_desc_record_callx() only sets BIT(k) in lent when reg_ref_off()
succeeds in the verifier state at the callx.

If another path reaches the same callx to invoke a non-consuming subprog while
Rk still holds the handle address at runtime, but its verifier state fails
reg_ref_off() (for instance, Rk is NOT_INIT after merging with a path that did
not set Rk, or after an earlier call to a no-op subprog cleared caller-saved
registers in the verifier while leaving Rk intact in hardware), BIT(k) is not
set in c->lent.  slot_can_be_kept() then accepts the program, the pre-callx
sequence zeroes the caller's hidden slot at runtime before the non-consuming
subprog runs, and a subsequent bpf_throw() fails to release the resource.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1f41095f4f1fa..51896b2001614 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11451,9 +11451,10 @@ static int setup_func_entry(struct bpf_verifier_env *env, int subprog, int calls
>  	err = set_callee_state_cb(env, caller, callee, callsite);
>  	if (err)
>  		goto err_out;
> -	/* a plain call hands the callee what it releases of the caller's, see frame_desc.c */
> +	/* the call of a subprog hands it what it releases of the caller's, see frame_desc.c */
>  	err = bpf_frame_desc_record_call(env, set_callee_state_cb == set_callee_state &&
> -					 bpf_pseudo_call(&env->prog->insnsi[callsite]) ? state : NULL,
> +					 (bpf_pseudo_call(&env->prog->insnsi[callsite]) ||
> +					  bpf_is_callx(&env->prog->insnsi[callsite])) ? state : NULL,
>  					 callee, callsite);

[Severity: Medium]
What happens if a callx invokes a subprog with sub->consumes & BIT(k) set on a
path where Rk holds the runtime address of a caller-held reference without
satisfying reg_ref_off()?

Because sub->consumes & BIT(k) is set, bpf_frame_desc_record_callx() skips Rk.
In bpf_frame_desc_record_call(), if one callx path passes the caller's
reference in Rk (recording rel_add() at callsite, with the callee releasing it
without needing its own hidden slot) and a second path passes the same runtime
pointer in Rk in a form that fails reg_ref_off() (such as a pointer with a
variable offset in [0, 1] that is 0 at runtime, or an untrusted pointer from
bpf_rdonly_cast()) on a branch where the callee does not release it,
bpf_frame_desc_record_call() only sets sub->entry_none |= BIT(k) and leaves
the reference with the caller.

Because the callee's BPF_RES_F_ENTRY acquisition does not need a hidden slot
of its own, bpf_frame_desc_resolve() never checks sub->entry_none, and
handed_back is not set on exit.  The emit_release() instructions placed before
the callx still zero the caller's hidden slot at runtime on the second path,
leaking the reference if the caller later throws.

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