Re: [PATCH bpf-next v4 29/46] bpf: Keep the first iteration of a counted loop apart

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: false rejection of counted loops due to expr_is_fresh()
  on loads
--

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

bpf: Keep the first iteration of a counted loop apart

This commit verifies the first iteration of a counted loop separately when
live registers are computed anew in each iteration, using the state returning
to the loop header after the first iteration to stand for the remaining
iterations.

> diff --git a/kernel/bpf/scev.c b/kernel/bpf/scev.c
> index 9377b0907f215..d8c847efbd7f3 100644
> --- a/kernel/bpf/scev.c
> +++ b/kernel/bpf/scev.c
[ ... ]
> @@ -3578,7 +3604,23 @@ int bpf_compute_loop_iters(struct bpf_verifier_env *env, struct bpf_verifier_sta
>  			walk = true;
>  			continue;
>  		}
> +		/*
> +		 * What is loaded from memory in every iteration is the same kind of thing in all
> +		 * of them. One iteration tells what. There is nothing more to find out when
> +		 * iterations are looked at one by one.
> +		 */
> +		if (expr_is_fresh(env, header_env->reg2expr[r])) {
> +			__set_bit(r, iters->fresh_regs);
> +			continue;
> +		}

[Severity: Medium]
As reported by bot+bpf-ci@kernel.org on v3 [1], can continuing here without
setting walk = true cause a regression where the verifier falsely rejects
valid counted-loop programs?

Every non-FP memory load gets OPAQUE_EXPR_ID in maybe_load_fp(), so
expr_is_fresh() marks any loaded register as fresh and bypasses the walk
check below, returning 1 instead of 2 from bpf_compute_loop_iters().

For a loop that indexes a BPF_F_RDONLY_PROG map (for example, idx =
rodata_table[idx]), the offset in the first iteration (entry->first == true)
is still a known constant, so check_map_mem_read() constant-folds idx to
rodata_table[0] instead of [0, 255].  When the first iteration returns to
the loop header, bpf_flow_loop_enter() clears entry->first and entry->apart
and leaves idx at that constant.  The second iteration then joins against
that state with widening enabled in flow_merge() -> bpf_states_join(), which
widens the upper bound of idx to S32_MAX whenever the new value exceeds
rodata_table[0] and causes subsequent bounds-checked array accesses to be
rejected.

Similarly, for a short counted loop that loads a nullable kptr into t and
tracks it with a scalar flag (have = 0; if (t && t->y > 100) have = 1;),
skipping the per-iteration walk causes bpf_widen_scev_regs() to widen have to
[0, 1] via union_any_reg() on the t == NULL path after the first iteration,
so a subsequent if (have) sum += t->x in the next iteration fails on a
NULL pointer dereference.

[1] https://lore.kernel.org/r/19e3280680aa046388274873789d649eac600b99eff2633376ba2a073fd30acd@mail.kernel.org

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