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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] bpf: stale reg->delta on scalar args in call_func_on_its_own()
- [High] bpf: bpf_flow_return() drops speculative state flag on callee exit
- [Medium] bpf: call_func_on_its_own() loses async callback RCU lock state
--

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

bpf: Verify static subprogs on their own

This commit verifies eligible static subprograms in a single-frame entry
state without their caller frames, mapping held references and locks to
positional inherited IDs and delivering exit states to all waiting callers.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8080920378063..901f4c7620e9e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -11794,6 +11797,175 @@ static void account_current_path(struct bpf_verifier_env *env);
>   * BPF_INHERITED_ID + the position for the function, and put back at the return.
>   */

[ ... ]

> +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]
Does setting callee->in_rcu_callback_fn = in_rcu_callback(env) lose the RCU
read lock state when called from a non-sleepable async callback within a
BPF_FUNC_CTX_NO_RCU_LOCK global subprogram?

When do_check_common() verifies a global subprogram with
env->no_rcu_read_lock = true and a non-sleepable async callback is queued via
push_async_cb(), the callback frame has frame[0]->in_async_callback_fn = true
and in_rcu_callback_fn = false. Inside that callback, has_rcu_read_lock()
returns true because of state->frame[0]->in_async_callback_fn:

static bool has_rcu_read_lock(struct bpf_verifier_env *env)
{
	struct bpf_verifier_state *state = env->cur_state;

	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;
}

When the async callback invokes a static subprogram via
call_func_on_its_own(), entry.frame[0] is replaced by callee where both
in_rcu_callback_fn and in_async_callback_fn are false while
env->no_rcu_read_lock remains true.

Wouldn't has_rcu_read_lock() then return false inside the static subprogram,
causing check_map_func_compatibility() to reject valid map helpers,
check_func_call() to record BPF_FUNC_CTX_NO_RCU_LOCK instead of
BPF_FUNC_CTX_NON_SLEEPABLE for global callees, and entry states with and
without RCU read lock protection to be merged?

> +	for (i = BPF_REG_1; i <= BPF_REG_5; i++)
> +		if (live & BIT(i))
> +			callee->regs[i] = caller->regs[i];
> +	err = grow_stack_arg_slots(env, callee, bpf_in_stack_arg_cnt(&env->subprog_info[subprog]));
> +	if (err) {
> +		bpf_free_verifier_state(&entry, false);
> +		return err;
> +	}
> +	for (i = 0; i < callee->out_stack_arg_cnt; i++)
> +		callee->stack_arg_regs[i] = caller->stack_arg_regs[i];
> +
> +	err = copy_reference_state(&entry, state);
> +	if (err) {
> +		bpf_free_verifier_state(&entry, false);
> +		return err;
> +	}
> +	for (i = 0; i < entry.acquired_refs; i++) {
> +		/* the id of a lock is the id of the pointer to it, which may have none */
> +		entry.refs[i].id = inherited_id(state, entry.refs[i].id);
> +		if (entry.refs[i].type == REF_TYPE_PTR)
> +			entry.refs[i].parent_id = inherited_id(state, entry.refs[i].parent_id);
> +	}
> +	entry.active_irq_id = inherited_id(state, entry.active_irq_id);
> +	entry.active_lock_id = inherited_id(state, entry.active_lock_id);
> +	callee->entry_refs = entry.acquired_refs;
> +	callee->entry_rcu_locks = entry.active_rcu_locks;
> +	callee->entry_preempt_locks = entry.active_preempt_locks;
> +	for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> +		callee->regs[i].id = inherited_id(state, callee->regs[i].id);
> +		callee->regs[i].parent_id = inherited_id(state, callee->regs[i].parent_id);
> +		callee->regs[i].map_uid = inherited_id(state, callee->regs[i].map_uid);
> +	}
> +	for (i = 0; i < callee->out_stack_arg_cnt; i++) {
> +		callee->stack_arg_regs[i].id = inherited_id(state, callee->stack_arg_regs[i].id);
> +		callee->stack_arg_regs[i].parent_id =
> +			inherited_id(state, callee->stack_arg_regs[i].parent_id);
> +		callee->stack_arg_regs[i].map_uid =
> +			inherited_id(state, callee->stack_arg_regs[i].map_uid);
> +	}

[Severity: Critical]
When clearing scalar argument IDs in call_func_on_its_own(), can leaving
reg->delta non-zero corrupt scalar bounds tracking in sync_linked_regs()?

If a caller passes a SCALAR_VALUE argument that was offset from a linked
scalar (for example r1 = r6; r1 += 10, which sets r1.id = base_id |
BPF_ADD_CONST64 and r1.delta = 10), inherited_id() returns 0 for r1.id,
leaving callee->regs[1].id == 0 and callee->regs[1].delta == 10 (and
similarly for callee->stack_arg_regs[i]).

In flow_arrive(), bpf_clear_singular_ids() skips registers where !reg->id.
When the callee later copies r1 to another register (r2 = r1),
assign_scalar_id_before_mov() only calls clear_scalar_id() if src_reg->id &
BPF_ADD_CONST:

	if (src_reg->id & BPF_ADD_CONST)
		clear_scalar_id(src_reg);
	if (!src_reg->id && !tnum_is_const(src_reg->var_off))
		src_reg->id = ++env->id_gen;

Because r1.id is 0, r1 and r2 receive a new base ID without BPF_ADD_CONST
while retaining delta == 10. If r2 is then offset (for example r2 += 4,
setting r2.id |= BPF_ADD_CONST64 and r2.delta = 4) and a conditional branch
refines r1 or r2, sync_linked_regs() assumes the register without
BPF_ADD_CONST has delta == 0 and computes (s64)reg->delta -
(s64)known_reg->delta using the stale delta of 10 (or if r2 += 10 so
reg->delta == known_reg->delta, copies *reg = *known_reg with no offset
adjustment).

In addition, reg_value_equal(), regs_exact(), and regsafe() (under
explore_alu_limits) memcmp up to offsetof(struct bpf_reg_state, id), which
includes s32 delta. Should call_func_on_its_own() also zero reg->delta (or
call clear_scalar_id()) when clearing IDs on arguments?

> +
> +	account_current_path(env);
> +	env->subprog_info[subprog].on_its_own = true;
> +	err = bpf_flow_call(env, state, &entry, subprog, insn_idx);
> +	bpf_free_verifier_state(&entry, false);
> +	/* the call waits, there is nothing to go on with */
> +	return err ?: PROCESS_BPF_EXIT;
> +}
> +
> +/* 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]
What happens when a speculative path spawned inside a static subprogram
verified on its own reaches exit_func_on_its_own()?

A non-speculative caller passes func_is_on_its_own() and waits in
bpf_flow_call() with w->st.speculative == false. While verifying the callee,
check_cond_jmp_op(), sanitize_ptr_alu(), or check_unwind_goto() can push a
speculative branch with branch->speculative = true and branch->flow_entry =
id. When that speculative state reaches BPF_EXIT:

exit_func_on_its_own()
  bpf_flow_exit()
    flow_apply_exit()
      bpf_flow_return(env, &st, &x->st, call_idx)

In bpf_flow_return(), caller->unwound is copied from exit->unwound, but
exit->speculative is not propagated to caller->speculative. As a result, st
is delivered back to the caller at call_idx + 1 with st.speculative == false.

In the caller, wouldn't this cause sanitize_mark_insn_seen() to mark
architecturally unreachable instructions as seen (bypassing
sanitize_dead_code()) and prevent do_check_block() from inserting nospec
barriers on recoverable errors because state->speculative is false? Also, in
flow_merge(), can a non-speculative state t->st from another call entry
subsume a speculative state s->st via bpf_state_subsumes() and call
flow_unite(flow, s->st.flow_entry, t->st.flow_entry), cross-delivering exits
between the two calls?

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