Re: [PATCH v2 bpf 14/19] bpf: Reset the range of a packet pointer that scalar evolution clamps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: out-of-bounds packet access via step tightening in bpf_clamp_scev_regs()
- [Medium] bpf: bpf_widen_scev_regs() does not reset negative packet pointer range
--

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

bpf: Reset the range of a packet pointer that scalar evolution clamps

This patch resets a clamped packet pointer's range to max(entry_reg->range, 0)
in bpf_clamp_scev_regs() so that packet bounds gained from pkt_end comparisons
inside a counted loop do not remain attached to the widened offset range across
all iterations.

> diff --git a/kernel/bpf/scev.c b/kernel/bpf/scev.c
> index 89f96682af3ff..7b82fd79f9cc9 100644
> --- a/kernel/bpf/scev.c
> +++ b/kernel/bpf/scev.c
> @@ -2914,6 +2914,13 @@ int bpf_clamp_scev_regs(struct bpf_verifier_env *env, struct bpf_func_state *cur
>  			bpf_log(log, "\n");
>  		}
>  		scratch_widened_reg_id(env, reg);
> +		/*
> +		 * The range of a packet pointer goes together with its id and with the offsets
> +		 * that it had in the compare with pkt_end. Both are replaced here. What holds for
> +		 * the new offsets is the range at the entry.
> +		 */
> +		if (type_is_pkt_pointer(reg->type))
> +			reg->range = max(entry_reg->range, 0);

[Severity: Medium]
Should bpf_widen_scev_regs() also clamp reg->range with max(reg->range, 0)
when widening a packet pointer at loop entry?

When a packet pointer fails a pkt_end comparison before a counted loop,
mark_pkt_end() sets entry_reg->range to AT_PKT_END (-1) or BEYOND_PKT_END (-2).
At loop entry, bpf_widen_scev_regs() replaces the packet pointer's id and
offsets across all iterations without clearing a negative reg->range:

kernel/bpf/scev.c:bpf_widen_scev_regs() {
	...
	scratch_widened_reg_id(env, reg);
	err = bpf_set_reg_range(env, reg, bounds.r64, bounds.r32,
				bounds.base, bounds.step);
	...
}

If the loop has a negative slope that moves the pointer backwards into the
packet on subsequent iterations, is_pkt_ptr_branch_taken() in
kernel/bpf/verifier.c will still fold pkt_end comparisons on the widened
register using the stale negative range marker.

In addition, when the backedge reaches the loop header, bpf_clamp_scev_regs()
sets rcur->range to 0 via max(entry_reg->range, 0) while the widened header
state still has rold->range < 0, causing regsafe() in kernel/bpf/states.c to
reject state pruning at the loop header:

kernel/bpf/states.c:regsafe() {
	...
	if (rold->range < 0 || rcur->range < 0) {
		/* special case for [BEYOND|AT]_PKT_END */
		if (rold->range != rcur->range)
			return false;
	}
	...
}

>  		err = bpf_set_reg_range(env, reg, bounds.r64, bounds.r32,
>  					bounds.base, bounds.step);

[Severity: Critical]
Can preserving entry_reg->range across bpf_set_reg_range() here (and in
bpf_widen_scev_regs()) allow out-of-bounds packet reads and writes past
pkt_end when bounds.step tightens reg_umax(reg)?

In find_good_pkt_pointers(), comparing a derived packet pointer against
pkt_end sets reg->range = reg_umax(dst_reg), and check_packet_access() ->
check_mem_region_access() allows accesses of size bytes when
reg_umax(reg) + off + size <= reg->range. As seen in commit b88822d2584f
("bpf: Fix packet range of pointers sharing an id"), tightening the umax of a
packet pointer via var_off alignment while keeping its range is unsafe.

Before a counted loop, a packet pointer can have an unaligned umax paired with
an aligned var_off and step == 1:

  r4 &= 0x38
  if r4 > 50 goto exit      ; r4: r64=[0, 50], var_off=(0x0; 0x38), step=1
  r6 = pkt_data
  r6 += r4                  ; r6: pkt(id=1, r=0, r64=[0, 50], var_off=(0x0; 0x38), step=1)
  r5 = r6
  r5 += 8                   ; r5: pkt(id=1, r=0, r64=[8, 58], var_off=(0x0; 0x78), step=1)
  if r5 > pkt_end goto exit ; r6: pkt(id=1, r=58, r64=[0, 50], var_off=(0x0; 0x38), step=1)

When entering or clamping a 2-iteration loop (max_header_count = 2) that
increments r6 by 8 (slope = 8), linear_bounds() in kernel/bpf/scev.c computes
bounds.r64 = [0, 58] and promotes tnum_alignment(entry_reg->var_off) into
bounds.step:

kernel/bpf/scev.c:linear_bounds() {
	...
	if (cnum64_is_const(reg->r64) && !overflow) {
		out->step = slope_abs;
		out->base = imod(reg_smin(reg), out->step);
	} else {
		out->step = 1u << min_t(u32, tnum_alignment(reg->var_off), __ffs(slope_abs));
		out->base = 0;
	}
}

Here bounds.step becomes 8. Calling bpf_set_reg_range() -> reg_bounds_sync()
-> __update_bounds_from_step() -> cnum64_intersect_linear() then rounds
reg_umax(reg) down from 58 to 56 while reg->range remains 58 (and similarly
rounds [0, 50] down to [0, 48] with range = 50 for a negative slope r6 -= 8).

At runtime, if r4 == 48 and pkt_len == 56, on the second iteration r6 reaches
pkt_data + 56 == pkt_end, but check_packet_access() will permit a 2-byte
access at r6 + 0 because reg_umax(reg) + 2 <= 58.

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