Re: [PATCH bpf-next v3 21/45] bpf: Verify static subprogs on their own

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] bpf: bpf_flow_return() drops speculative state flag on callee exit
- [Medium] bpf: call_func_on_its_own() loses async callback context in frame[0]
- [Medium] bpf: add_insns_of_callees() double-counts insns and misses callx edges
- [Medium] bpf: bpf_flow_return() lazy fallback leaves stale post-call states
--

commit 34bdfd07d4f78d7029c50c8aec27497f8117e123
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Verify static subprogs on their own

This commit verifies eligible static subprograms in a single standalone frame
rather than in the call frames of their callers, joining states at the
subprogram entry and applying exit states to all waiting call sites.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 103f368035638..b61166f670945 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -11840,6 +11843,180 @@ static void account_current_path(struct bpf_verifier_env *env);

[ ... ]

> +static bool func_is_on_its_own(struct bpf_verifier_env *env, struct bpf_verifier_state *state,
> +			       int subprog)
> +{
> +	struct bpf_func_state *caller = state->frame[state->curframe];
> +	struct bpf_subprog_info *sub = &env->subprog_info[subprog];
> +	u16 live = env->insn_aux_data[sub->start].live_regs_before;
> +	u32 i;
> +
> +	if (sub->changes_pkt_data || sub->has_tail_call || sub->has_ld_abs ||
> +	    sub->is_cb || sub->is_async_cb || sub->is_exception_cb)
> +		return false;
> +	if (state->speculative)
> +		return false;
> +	if (sub->releases_inherited &&
> +	    (state->acquired_refs || state->active_rcu_locks || state->active_preempt_locks))
> +		return false;

[Severity: Medium]
What happens if a static subprogram verified on its own has multiple exit
paths, or is called from two sites whose entries are merged, where one exit
leaves caller-held references, locks, or packet data intact while a later
exit releases an inherited resource or changes packet data?

In kernel/bpf/flow.c:flow_apply_exit(), each exit state is applied to waiting
callers as it arrives:

	err = bpf_copy_verifier_state(&st, &w->st);
	if (!err)
		err = bpf_flow_return(env, &st, &x->st, call_idx);
	if (!err)
		err = bpf_flow_deliver(env, &st, call_idx + 1, call_idx);
	bpf_free_verifier_state(&st, false);
	if (err != 1)
		return err;

When an earlier non-releasing exit reaches flow_apply_exit() (or is applied
via flow_do_united() when two entry states are joined by flow_merge()), a
post-call state is delivered to call_idx + 1 while w remains on entry->calls.

When a subsequent exit reaches bpf_flow_return() and sets
sub->releases_inherited = true (or sub->changes_pkt_data = true), returning 1
re-queues w at call_idx for in-frame verification, but any post-call states
already delivered to call_idx + 1 from earlier exits are not retracted. Can
those stale post-call states continue executing in the caller and trigger
false check_resource_leak() failures at program exit?

[ ... ]

> +static int call_func_on_its_own(struct bpf_verifier_env *env, int subprog, int insn_idx)
> +{
> +	u16 live = env->insn_aux_data[env->subprog_info[subprog].start].live_regs_before;
> +	struct bpf_verifier_state *state = env->cur_state;
> +	struct bpf_func_state *caller = state->frame[state->curframe], *callee;
> +	struct bpf_verifier_state entry = {};
> +	int i, err;
> +
> +	callee = kzalloc_obj(*callee, GFP_KERNEL_ACCOUNT);
> +	if (!callee)
> +		return -ENOMEM;
> +	entry.frame[0] = callee;
> +	entry.in_sleepable = state->in_sleepable;
> +	entry.callers_unknown = state->callers_unknown;
> +	init_func_state(env, callee, BPF_MAIN_FUNC, 0, subprog);
> +	/* the frame that says so is not there when the function is verified */
> +	callee->in_rcu_callback_fn = in_rcu_callback(env);

[Severity: Medium]
As reported by Sashiko on v2 [1], when a non-sleepable async callback
scheduled from a global subprogram verified with env->no_rcu_read_lock == true
calls a static subprogram verified on its own, call_func_on_its_own() replaces
entry.frame[0] with callee and only sets callee->in_rcu_callback_fn.

Because push_async_cb() sets async_cb->frame[0]->in_async_callback_fn = true
and async_cb->frame[0]->callback_ret_range rather than in_rcu_callback_fn,
has_rcu_read_lock() relies on state->frame[0]->in_async_callback_fn:

	if (state->active_rcu_locks || in_rcu_callback(env))
		return true;
	if (in_sleepable(env))
		return false;
	return !env->no_rcu_read_lock || state->frame[0]->in_async_callback_fn;

And check_return_code() (which check_kfunc_call() invokes for bpf_throw()
during do_check_all()) also checks frame->in_async_callback_fn and
frame->callback_ret_range on env->cur_state->frame[0].

Should call_func_on_its_own() also preserve state->frame[0]->in_async_callback_fn
and state->frame[0]->callback_ret_range on callee so RCU read-lock tracking
and async callback return-range checks are not lost inside the standalone
callee frame?

[ ... ]

> +/* The current state is at exit of a function that is verified on its own. */
> +static int exit_func_on_its_own(struct bpf_verifier_env *env)
> +{
> +	struct bpf_verifier_state *state = env->cur_state;
> +	struct bpf_func_state *callee = state->frame[0];
> +	u32 i, nregs;
> +	int err;
> +
> +	nregs = bpf_ret_reg_pair(env, callee->subprogno) ? 2 : 1;
> +	for (i = 0; i < nregs; i++) {
> +		if (callee->regs[ret_regs[i]].type != PTR_TO_STACK)
> +			continue;
> +		verbose(env, "cannot return stack pointer to the caller\n");
> +		return -EINVAL;
> +	}
> +	state->unwound |= bpf_is_unwind_exit(&env->prog->insnsi[env->insn_idx]);
> +
> +	/* Invalidate callee-local dynptrs and their slices before the frame goes away. */
> +	err = destroy_dynptrs_in_stack_slots(env, callee, 0,
> +					     callee->allocated_stack / BPF_REG_SIZE - 1);
> +	if (err)
> +		return err;
> +
> +	account_current_path(env);
> +	err = bpf_flow_exit(env, state, env->insn_idx);
> +	return err ?: PROCESS_BPF_EXIT;
> +}

[Severity: High]
As reported by bot+bpf-ci@kernel.org on v2 [2], when speculation mitigation is
active (!env->bypass_spec_v1 or !env->bypass_spec_v4), a static subprogram
entered with state->speculative == false can spawn speculative paths inside
the callee via check_cond_jmp_op(), sanitize_ptr_alu(), or
check_unwind_goto().

When such a speculative path reaches BPF_EXIT, process_bpf_exit_full() routes
it through exit_func_on_its_own() -> bpf_flow_exit() -> flow_apply_exit() ->
bpf_flow_return(). In bpf_flow_return(), caller->unwound is copied from
exit->unwound, but exit->speculative is not propagated to caller->speculative:

	if (exit->unwound && !bpf_is_unwind_goto(&env->prog->insnsi[call_idx + 1])) {
		verbose(env, "unwind_exit, but no unwind_goto after the call at insn %d\n",
			call_idx);
		return -EINVAL;
	}
	caller->unwound = exit->unwound;

In addition, in kernel/bpf/flow.c:flow_merge(), a non-speculative state t->st
can subsume a speculative state s->st via flow_within() ->
bpf_state_subsumes() and unite their flow_entry sets via flow_unite().

Could this cause speculative return states to resume in the caller as
non-speculative architectural paths (marking dead instructions as seen in
sanitize_mark_insn_seen() and failing the state->speculative check on
recoverable errors in do_check_block()) or cross-unite distinct call entries?

[ ... ]

> @@ -23452,6 +23641,37 @@ static int do_check_all(struct bpf_verifier_env *env)
>  	}
>  }
>  
> +/*
> + * What it took to verify a static subprog on its own is not in insns_total of its callers.
> + * Add it, once per caller. The main prog and global subprogs have all of it already.
> + */
> +static void add_insns_of_callees(struct bpf_verifier_env *env)
> +{
> +	struct bpf_subprog_info *si = env->subprog_info;
> +	struct bpf_insn *insn = env->prog->insnsi;
> +	int *added, i, j, s, c;
> +
> +	added = kvcalloc(env->subprog_cnt, sizeof(*added), GFP_KERNEL_ACCOUNT);
> +	if (!added)
> +		return;
> +	/* callees first */
> +	for (i = 0; i < env->subprog_cnt; i++) {
> +		s = env->subprog_topo_order[i];
> +		if (!s || bpf_subprog_is_global(env, s))
> +			continue;
> +		for (j = si[s].start; j < si[s + 1].start; j++) {
> +			if (!bpf_pseudo_call(&insn[j]))
> +				continue;
> +			c = bpf_find_subprog(env, j + insn[j].imm + 1);
> +			if (c <= 0 || c == s || !si[c].on_its_own || added[c] == s)
> +				continue;
> +			added[c] = s;
> +			si[s].insns_total += si[c].insns_total;
> +		}
> +	}
> +	kvfree(added);
> +}

[Severity: Medium]
As reported by bot+bpf-ci@kernel.org on v2 [3], if a static subprogram c is
verified in-frame for one call from s (or falls back to in-frame verification
when bpf_flow_return() returns 1, or across a do_check_all() restart where
on_its_own is not cleared) and is also verified on its own for another call,
prepare_func_exit() -> account_processed_insns() has already added the
in-frame instructions of c into both si[c].insns_total and si[s].insns_total.
When add_insns_of_callees() then adds the full si[c].insns_total to
si[s].insns_total, won't the in-frame portion be counted twice in
si[s].insns_total?

Also, since check_func_callx() calls check_static_func_call() and can verify
indirect targets on their own via call_func_on_its_own(), does
add_insns_of_callees() need to account for callx edges in env->callx_edges in
addition to bpf_pseudo_call() instructions?

[1] https://lore.kernel.org/r/20261105204513.2230344-22-alexei.starovoitov@gmail.com
[2] https://lore.kernel.org/r/fbc6a76bd8cb0b115a602fe1f8064612f09afac8924a23ae5d64cf4e7fe68b43@mail.kernel.org
[3] https://lore.kernel.org/r/fbc6a76bd8cb0b115a602fe1f8064612f09afac8924a23ae5d64cf4e7fe68b43@mail.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106035111.3315285-1-alexei.starovoitov@gmail.com?part=21