Re: [PATCH bpf-next 2/2] selftests/bpf: pointer operands for 32-bit recurrences/latches

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: 32-bit linear SCEVs on pointers not rejected outside latch check
--

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

selftests/bpf: pointer operands for 32-bit recurrences/latches

This commit adds two selftests checking that loop bounds are not inferred when
latch operands are pointers involved in a 32-bit comparison or a 32-bit
recurrence.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_scev.c b/tools/testing/selftests/bpf/progs/verifier_scev.c
> index 661ce9ca8f938..6f8c3980299c4 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_scev.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_scev.c
> @@ -2971,6 +2971,62 @@ __naked void ptr_stack_other_frame(void)
>  "	::: __clobber_all);
>  }
>  

[ ... ]

> +SEC("xdp")
> +__failure
> +__log_level(2)
> +__msg("scev: incompatible latch operands r6 (map_value), r7 (map_value)")
> +__msg("loop header at 11, can't compute iterations count at 14")
> +__naked void ptr_map_value_alu32(void)
> +{
> +	asm volatile ("					\
> +	*(u64 *)(r10 - 8) = 0;				\
> +	r2 = r10;					\
> +	r2 += -8;					\
> +	r1 = %[map] ll;					\
> +	call %[bpf_map_lookup_elem];			\
> +	if r0 == 0 goto 2f;				\
> +	r6 = r0;					\
> +	r7 = r6;					\
> +	r7 += 4;					\
> +	r9 = 0;						\
> +1:"	LIMIT_ITERATIONS				\
> +"	w6 += 1;					\
> +	if r6 < r7 goto 1b;				\
> +2:	r0 = 0;						\
> +	exit;						\
> +"	:
> +	: __imm(bpf_map_lookup_elem), __imm_addr(map)
> +	: __clobber_all);
> +}

[Severity: High]
In ptr_map_value_alu32(), w6 += 1 is applied directly to a pointer register
while r6 is also the latch operand. The companion commit ("bpf: reject pointer
operands for 32-bit recurrences/latches") rejects this via !latch->zext32 in
scev_value_compatible():

kernel/bpf/scev.c:scev_value_compatible() {
	...
	ra = scev_reg_for_read(env, st, a->ptr_reg);
	rb = scev_reg_for_read(env, st, b->ptr_reg);
	if (ra && rb && bpf_same_memory_origin(ra, rb) && !latch->zext32 && !latch->jmp32)
		return true;
}

However, what happens when a pointer register has a 32-bit linear recurrence
(such as r6 += 1; w6 = w6; or w6 = w6; w6 += 1;, which simplify() folds into
(zext32 (linear r6 1))) while a separate scalar register controls the loop
latch?

When allow_ptr_leaks is enabled (as in SEC("xdp") with CAP_BPF), check_alu_op()
permits 32-bit BPF_MOV on a pointer without calling adjust_ptr_min_max_vals(),
converting r6 into a SCALAR_VALUE holding the low 32 bits of the runtime
pointer address.

Because scev_value_compatible() only checks the latch operands,
bpf_compute_loop_iters() accepts r6 as widenable even when coefs.zext32 is
true:

kernel/bpf/scev.c:bpf_compute_loop_iters() {
		...
		r_scev = header_env->reg2scev[r];
		reg = scev_reg_for_linear(cur_func, r);
		/* If SCEV for r is (linear <reg> <slope>) */
		if (reg && is_simple_linear(scev, r_scev, &coefs) && is_widenable_reg_type(reg)) {
		...
}

Subsequently, bpf_widen_scev_regs() widens the pointer offset using 32-bit
linear_bounds(), and on the backedge bpf_clamp_scev_regs() computes 32-bit
linear_bounds() from entry_reg (the loop-entry PTR_TO_MAP_VALUE with offset 0)
and intersects that small offset range with the backedge register reg (now a
SCALAR_VALUE holding the truncated 32-bit runtime kernel address):

kernel/bpf/scev.c:bpf_clamp_scev_regs() {
		...
		linear_bounds(entry_reg, max_header_count, &coefs, &bounds);
		bounds.r64 = cnum64_intersect(reg->r64, bounds.r64);
		bounds.r32 = cnum32_intersect(reg->r32, bounds.r32);
		...
}

Can this corrupt the verifier's scalar bounds for r6 by clamping a runtime
32-bit kernel address to a small offset range like [0, 1], and should
coefs.zext32 on pointer registers also be rejected in bpf_compute_loop_iters(),
bpf_widen_scev_regs(), and bpf_clamp_scev_regs() (with corresponding selftests
covering w6 = w6 and non-latch pointer recurrences)?

>  
>  /* Metadata and packet data have different origins. */
>  SEC("xdp")
>  __success

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020234705.375067-1-eddyz87@gmail.com?part=2