Re: [PATCH bpf-next v3 08/12] bpf: Recover the low 32 bits on the reverse link direction

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: missing contradiction check in sync_linked_regs() reverse-direction arm
--

commit 651910c4895680e635057947cc88b56f4edbf401
Author: Vineet Gupta <vineet.gupta@linux.dev>

bpf: Recover the low 32 bits on the reverse link direction

This commit updates sync_linked_regs() in the BPF verifier to recover the low
32-bit bounds and subreg tnum when propagating from a narrow linked register
(BPF_REG_LINK_ZEXT or BPF_REG_LINK_SEXT) back to a linked register wider than
32 bits.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4621b476b206e..ac50252cec1f7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18785,29 +18785,54 @@ static void sync_linked_regs(struct bpf_verifier_env *env, struct bpf_verifier_s
>  		if (reg->id != known_reg->id)
>  			continue;
>  		/*
> -		 * The reverse direction does not work for a narrow known_reg:
> -		 * it knows only its low 32 bits, which say nothing about a
> -		 * wider reg's high half. Neither (u32)(base + delta) nor
> -		 * (s32)(base + delta) inverts once the base is wider than 32
> -		 * bits.
> +		 * The reverse direction, from a narrow known_reg to a wider
> +		 * reg, does not invert: neither (u32)(base + delta) nor
> +		 * (s32)(base + delta) says anything about the base's high
> +		 * half. It does still pin the low half, though -- both sides
> +		 * are (base + delta) mod 2^32, differing only by their deltas
> +		 * -- so recover that much and leave the high half alone.
>  		 */
>  		if ((known_reg->link_kind == BPF_REG_LINK_ZEXT ||
>  		     known_reg->link_kind == BPF_REG_LINK_SEXT) &&
>  		    reg->link_kind != BPF_REG_LINK_ZEXT &&
>  		    reg->link_kind != BPF_REG_LINK_SEXT &&
> -		    get_reg_width(reg) > 32)
> -			continue;
> +		    get_reg_width(reg) > 32) {
> +			/*
> +			 * Only @low's 32-bit half is meaningful: its var_off
> +			 * is advanced over all 64 bits while r64 and the
> +			 * base/step line are not, and nothing below reads
> +			 * either. It is a scratch value, not a register state.
> +			 */
> +			struct bpf_reg_state low = *known_reg;
> +			struct tnum low32;
> +
> +			fake_reg.type = SCALAR_VALUE;
> +			__mark_reg_known(&fake_reg,
> +					 (s64)reg->delta - (s64)known_reg->delta);
> +			scalar32_min_max_add(&low, &fake_reg);
> +			low.var_off = tnum_add(low.var_off, fake_reg.var_off);
> +
> +			cnum32_intersect_with(&reg->r32, low.r32);
> +			low32 = tnum_intersect(tnum_subreg(reg->var_off),
> +					       tnum_subreg(low.var_off));
> +			reg->var_off = tnum_with_subreg(reg->var_off, low32);
> +			reg_bounds_sync(reg);
> +			goto scratched;
> +		}

[Severity: Medium]
Can this reverse-direction arm leave reg with an empty range (CNUM32_EMPTY or
CNUM64_EMPTY) or conflicting constant subreg bits when the low-32 bounds,
var_off, or step progression of reg and known_reg contradict on an
unreachable path?

In the sibling differencing arm further down in sync_linked_regs(), the code
saves prev = *reg before intersecting and restores *reg = prev if
range_bounds_violation(reg) holds:

kernel/bpf/verifier.c:sync_linked_regs() {
	...
	cnum64_intersect_with(&reg->r64, prev.r64);
	cnum32_intersect_with(&reg->r32, prev.r32);
	reg->var_off = tnum_intersect(reg->var_off, prev.var_off);
	if (range_bounds_violation(reg))
		*reg = prev;
	reg_bounds_sync(reg);
}

In the new arm above, cnum32_intersect_with(&reg->r32, low.r32) and
tnum_intersect() are applied directly without saving prev or checking for
contradictions. If reg->r32 becomes CNUM32_EMPTY, reg_bounds_sync() exits
early on range_bounds_violation(reg), and even when cnum32_intersect_with()
is non-empty, reg_bounds_sync() can still empty reg->r32 or reg->r64 via
__update_reg_bounds(), __reg_deduce_bounds(), or __update_bounds_from_step()
when the narrowed low 32 bits contradict reg's 64-bit bounds, subreg tnum, or
base/step progression. Similarly, if tnum_subreg(reg->var_off) and
tnum_subreg(low.var_off) do not overlap (!tnum_overlap()), tnum_intersect()
ORs conflicting constant bits together.

Contradictions can reach this arm in several ways during check_cond_jmp_op()
-> sync_linked_regs():

1. Mixed link kinds: if r6 is a wide unknown scalar (BPF_REG_LINK_NONE),
   w7 = w6 (BPF_REG_LINK_ZEXT), and r8 = r6; r8 += 2 (BPF_REG_LINK_FULL),
   narrowing r8 with if w8 != 7 goto out syncs r6->r32 to [5, 5] while
   skipping r7 because r7->link_kind != r8->link_kind. A subsequent
   if w7 != 9 goto out then enters the new arm for r6 and r8, intersecting
   [5, 5] with [9, 9] and [7, 7] with [11, 11] to leave r6->r32 and r8->r32
   as CNUM32_EMPTY.

2. Sign-extending mov (BPF_REG_LINK_SEXT): if a wide r6 has its low 32 bits
   pinned by if w6 != 5 goto out (so r6->r32 is [5, 5]), r7 = (s32)r6 links
   r7 with BPF_REG_LINK_SEXT while coerce_reg_to_size_sx() resets r7->r32 to
   [S32_MIN, S32_MAX]. A following if w7 != 8 goto out enters the new arm
   and intersects r6->r32 ([5, 5]) with [8, 8].

3. BPF_JSET fallthrough: in regs_refine_cond_op(), the BPF_JSET | BPF_X
   fallthrough path calls __mark_reg_unbounded() before masking var_off.
   After w7 = w6; if w6 == 0 goto out; (where r6->r32 is [1, U32_MAX]),
   if w7 & -1 goto out refines r7 to 0 on fallthrough and syncs back to r6,
   intersecting [1, U32_MAX] with [0, 0].

4. Step or tnum conflict inside reg_bounds_sync(): if a wide r6 has step > 1
   (for example step == 3) and w7 = w6 resets r7->step to 1 via
   zext_32_to_64(), narrowing w7 to a value in r6->r32 that is not a
   multiple of 3 passes cnum32_intersect_with() and then becomes empty in
   __update_bounds_from_step(r6) at the end of reg_bounds_sync().

When reg is left in this state, any subsequent instruction that checks it via
reg_bounds_sanity_check() triggers verifier_bug() ("range bounds violation"),
emitting a BPF_WARN_ONCE() and returning -EFAULT when
BPF_F_TEST_REG_INVARIANTS is enabled.

Should this arm save struct bpf_reg_state prev = *reg and restore *reg = prev
if !tnum_overlap(tnum_subreg(reg->var_off), tnum_subreg(low.var_off)) or if
range_bounds_violation(reg) is true after reg_bounds_sync(reg)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020161907.2115379-1-vineet.gupta@linux.dev?part=8