Re: [PATCH bpf 1/2] bpf: Keep fastcall spills for helper stack reads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] check_stack_read_fixed_off passes the wrong execution frame to check_fastcall_stack_contract
- [High] fastcall_stack_off minimum bound logic leaves longer fastcall patterns unprotected
--

commit 8cfed53d5c0d5c6188360232f775d8b94b125599
Author: Nuoqi Gui <gnq25@mails.tsinghua.edu.cn>

bpf: Keep fastcall spills for helper stack reads

The fastcall spill/fill rewrite is only sound while the stack slots used by
the pattern are not accessed outside the pattern. Direct stack loads and
stores already call check_fastcall_stack_contract() to enforce this.

Helper and kfunc memory-argument checks can validate PTR_TO_STACK reads
through check_stack_range_initialized() without applying the same contract.
When such a read overlaps a fastcall spill slot,
bpf_remove_fastcall_spills_fills() can still remove the spill/fill pair.
It can then shrink the subprogram stack depth even though a helper or kfunc
reads that stack address.

Apply check_fastcall_stack_contract() from check_stack_range_initialized()
after the concrete stack range is known. Zero-sized accesses do not read or
write memory, so leave the fastcall optimization unchanged for those.

Fixes: 5b5f51bff1b66 ("bpf: no_caller_saved_registers attribute for helper calls")
Signed-off-by: Nuoqi Gui <gnq25@mails.tsinghua.edu.cn>

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 69d75515ed3f7..592da530bdc5a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6874,6 +6874,10 @@ static int check_stack_range_initialized(
>  		max_off = reg->smax_value + off;
>  	}
>  
> +	if (access_size)
> +		check_fastcall_stack_contract(env, state, env->insn_idx,
> +					      min_off);
> +

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this change I
noticed a pre-existing issue in check_stack_read_fixed_off() with how it
passes execution frames to check_fastcall_stack_contract().

In check_stack_read_fixed_off():

	struct bpf_verifier_state *vstate = env->cur_state;
	struct bpf_func_state *state = vstate->frame[vstate->curframe];
	...
	check_fastcall_stack_contract(env, state, env->insn_idx, off);

Here, the local state variable evaluates to the current execution frame
(the callee), instead of reg_state which is the frame where the stack memory
actually resides (and can be the caller).

If a BPF subprogram is passed a pointer to its caller's stack and reads a
stack slot that overlaps with a fastcall spill slot in the caller, couldn't
the fastcall bounds be checked against the wrong subprogram's metadata?

Could this cause the verifier to fail to disable the fastcall rewrite for
the caller, resulting in uninitialized stack memory reads when
bpf_remove_fastcall_spills_fills() removes the caller's spill instructions?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential problem in
mark_fastcall_pattern_for_call() with how it tracks fastcall stack boundaries:

	subprog->fastcall_stack_off = min(subprog->fastcall_stack_off, off);

Because off is a negative stack offset that moves closer to 0 for longer
patterns, using min() incorrectly shrinks the protected stack region to the
upper bound of the shortest fastcall pattern.

If a BPF subprogram contains multiple fastcall helper patterns of different
lengths (for example, Call A with 1 spill ending at offset -24, and Call B
with 2 spills ending at offset -16), couldn't this expose the upper spill
slots of any longer fastcall patterns?

If a program performs a direct memory access to the highest spill slot (-16)
of the longer pattern outside of the fastcall block, the verifier might
mistakenly preserve the fastcall optimization, removing the spills and
causing uninitialized memory reads at runtime.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-f01-12-fastcall-helper-stack-read-v1-0-e0a86085ef77@mails.tsinghua.edu.cn?part=1