Re: [PATCH bpf-next v3 35/51] bpf: avoid widening registers that hinder exact stack-slot tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: call_params_mask clobbers callee-saved registers for kfuncs with >5 args
- [Medium] bpf: bpf_needs_fixed_stack_off ignores module kfuncs and stack dynptrs
--

commit c21aed6872d1b94060c148c7501830216020d7e4
Author: Eduard Zingerman <eddyz87@gmail.com>

bpf: avoid widening registers that hinder exact stack-slot tracking

This commit adds logic to avoid widening registers used by stack-accessing
instructions during SCEV-based loop analysis. This prevents replacing
constant-offset stack accesses with varying-offset accesses, preserving
verifier precision for spills, fills, and calls requiring fixed stack
offsets.

> diff --git a/kernel/bpf/scev.c b/kernel/bpf/scev.c
> index 05dd8e439609b..adad32d8cb124 100644
> --- a/kernel/bpf/scev.c
> +++ b/kernel/bpf/scev.c
[ ... ]
> @@ -1100,6 +1100,70 @@ static void reset_scevs_at_indirect_writes(struct bpf_verifier_env *env, struct
[ ... ]
> +/* Mask of argument registers (R1..R5) a call at 'idx' passes by register. */
> +static u16 call_params_mask(struct bpf_verifier_env *env, int idx)
> +{
> +	struct bpf_insn *insn = &env->prog->insnsi[idx];
> +	struct bpf_call_summary cs;
> +	int n = bpf_get_call_summary(env, insn, &cs) ? cs.arg_slot_cnt : MAX_BPF_FUNC_REG_ARGS;
> +
> +	return n ? GENMASK(BPF_REG_1 + n - 1, BPF_REG_1) : 0;
> +}

[Severity: Medium]
Does this code overflow the bitmask into callee-saved registers when a
kfunc takes arguments on the stack?

bpf_get_call_summary() returns the total number of arguments in
cs.arg_slot_cnt, which can exceed 5 (MAX_BPF_FUNC_REG_ARGS) for kfuncs
that accept stack arguments.

Since GENMASK(BPF_REG_1 + n - 1, BPF_REG_1) does not cap 'n', an 'n'
greater than 5 will set bits for callee-saved registers (R6-R10).

If one of these unrelated registers happens to hold a stack pointer,
collect_store_base_regs() might erroneously record it as a base register
for the call. This could falsely prevent SCEV from widening unrelated
loops, causing the verifier to unnecessarily unroll the loop and
potentially hit complexity limits.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a597a8c189723..2f190abe34489 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -14257,6 +14257,55 @@ static bool kfunc_spin_allowed(struct bpf_verifier_env *env, s32 func_id, s16 of
[ ... ]
> +bool bpf_needs_fixed_stack_off(struct bpf_verifier_env *env, int insn_idx)
> +{
> +	const struct bpf_insn *insn = &env->prog->insnsi[insn_idx];
[ ... ]
> +	/* Both initialized and uninitialized stack dynptrs need a fixed offset. */
> +	for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++)
> +		if (arg_type_is_dynptr(fn->arg_type[i]))
> +			return true;

[Severity: Medium]
Does this loop miss dynptrs that are passed on the stack?

Because fn->arg_type is strictly bounded to 5 elements, this iteration
will skip checking a dynptr if it is passed as a 6th argument (or later)
on the stack.

If missed, collect_store_base_regs() fails to record the base registers,
allowing the SCEV pass to widen the stack pointer and turn its constant
offset into a variable range. The verifier will then reject the dynptr
call because it strictly enforces a constant offset.

> +
> +	/* vmlinux kfuncs only */
> +	if (!bpf_pseudo_kfunc_call(insn) || insn->off != 0)
> +		return false;

[Severity: Medium]
Will this check skip BPF module iterators?

By returning false if insn->off != 0, this explicitly ignores any kfuncs
exposed by BPF modules. Modules can provide their own iterators (like
bpf_iter_testmod_seq_new with KF_ITER_NEW).

Similar to the dynptr issue above, skipping these iterators means the
base registers won't be recorded. This causes the stack pointer to be
widened and leads to valid programs utilizing module iterators being
falsely rejected by the verifier.

> +	btf_id = insn->imm;
> +
> +	flags = btf_kfunc_flags(btf_vmlinux, btf_id, env->prog);
> +	if (flags && (*flags & (KF_ITER_NEW | KF_ITER_NEXT | KF_ITER_DESTROY)))
> +		return true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-scev-minimal-rebase-v3-0-bcba9bc53ac9@gmail.com?part=35