Re: [PATCH bpf-next v1 19/29] bpf: Allow a callee to release its caller's resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] bpf: missing releases_inherited mark in release_context_state()
--

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

bpf: Allow a callee to release its caller's resources

This commit allows a subprogram in a program that throws to release
resources acquired by its caller, clearing the caller's hidden slot before
the call when needed and marking intermediate subprograms with
releases_inherited at the release site so they are verified in the context
of their callers.

> diff --git a/kernel/bpf/frame_desc.c b/kernel/bpf/frame_desc.c
> index 9854be9ab4781..d2ad02925f061 100644
> --- a/kernel/bpf/frame_desc.c
> +++ b/kernel/bpf/frame_desc.c

[ ... ]

> @@ -689,12 +690,76 @@ static int rel_add(struct bpf_verifier_env *env, u32 insn, u16 acq, u8 reg, u8 f
>  	return 0;
>  }
>  
> +/*
> + * What the call @insn of a subprog means for what acquisition @acq acquired
> + * is noted in the record of the two, see struct bpf_res_call. NULL when there
> + * is no memory for one.
> + */
> +static struct bpf_res_call *call_note(struct bpf_verifier_env *env, u32 insn, u16 acq)
> +{
> +	u32 i, cnt = env->res_call_cnt;
> +	struct bpf_res_call *c;
> +
> +	for (i = 0; i < cnt; i++)
> +		if (env->res_calls[i].insn == insn && env->res_calls[i].acq == acq)
> +			return &env->res_calls[i];
> +	c = kvrealloc(env->res_calls, array_size(cnt + 1, sizeof(*c)), GFP_KERNEL_ACCOUNT);
> +	if (!c)
> +		return NULL;
> +	c[cnt] = (struct bpf_res_call) { .insn = insn, .acq = acq };
> +	env->res_calls = c;
> +	env->res_call_cnt = cnt + 1;
> +	return &c[cnt];
> +}
> +
> +/*
> + * The current insn releases @ref, which a frame below the current one owns.
> + * Nothing tells a callee where the slot of its caller is, so it cannot clear
> + * one. There are two ways out:
> + *
> + *  - What the caller holds without a slot needs none cleared: its sites say
> + *    where the handle is on the visits that find it held, and say nothing on
> + *    those that find it released.
> + *  - The caller clears its slot in front of the call when the callee releases
> + *    the resource on every path and no throw finds the caller holding it at
> + *    the call, see bpf_frame_desc_resolve().
> + *
> + * The functions between the owner and the current frame are verified in the
> + * context of their callers from now on, so that every throw sees the frames
> + * below it as they are. The round of the main pass that finds one out is done
> + * again, since it recorded the frames of its callers at the call, holding
> + * what they held then.
> + */
> +static int foreign_release(struct bpf_verifier_env *env, const struct bpf_verifier_state *st,
> +			   const struct bpf_reference_state *ref, struct bpf_res_acq *acq)
> +{
> +	const struct bpf_func_state *below = st->frame[ref->frameno + 1];
> +	struct bpf_subprog_info *sub;
> +	struct bpf_res_call *c;
> +	int f;
> +
> +	for (f = ref->frameno + 1; f <= st->curframe; f++) {
> +		sub = &env->subprog_info[st->frame[f]->subprogno];
> +		if (sub->releases_inherited)
> +			continue;
> +		sub->releases_inherited = true;
> +		env->frame_desc_redo = true;
> +	}

[Severity: High]
Does release_context_state() also need to set sub->releases_inherited and
env->frame_desc_redo when a frame leaves an inherited RCU read or
preemption-disabled section?

foreign_release() marks sub->releases_inherited when a callee releases an
entry in st->refs[] so that a subprog that throws without returning is
verified in the context of its caller.  However, RCU and preemption-disabled
sections are tracked via entry_rcu_locks and entry_preempt_locks rather
than st->refs[]:

kernel/bpf/verifier.c:release_context_state() {
	for (fr = 0; fr <= state->curframe; fr++) {
		frame = state->frame[fr];
		if (frame->entry_rcu_locks > state->active_rcu_locks)
			frame->rcu_released_below += frame->entry_rcu_locks - state->active_rcu_locks;
		if (frame->entry_preempt_locks > state->active_preempt_locks)
			frame->preempt_released_below += frame->entry_preempt_locks -
							 state->active_preempt_locks;
    ...
}

For those sections, releases_inherited is only set in bpf_flow_return() when
the callee returns.  If a static subprogram calls bpf_rcu_read_unlock() on
its caller's RCU section and then calls bpf_throw() without returning, it
remains verified on its own without the caller's frames in env->cur_state.
When bpf_rcu_read_unlock() calls invalidate_rcu_protected_refs(), it does
not see STACK_ITER slots with MEM_RCU on the caller's stack, allowing an
RCU-protected iterator to outlive its RCU section across a sleepable call
before bpf_throw() unwinds the stack and invokes the iterator destructor
(use-after-free).

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