Re: [PATCH bpf-next v3 06/12] bpf: Track low-32 scalar equality across sign-extending movs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: empty range after reg_bounds_sync() in sync_linked_regs()
--

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

bpf: Track low-32 scalar equality across sign-extending movs

This commit adds BPF_REG_LINK_SEXT to track low-32 scalar equality across
32-bit sign-extending moves, using sext_32_to_64() and tnum_sext() to
propagate bounds in sync_linked_regs().

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6afd718ede647..4621b476b206e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -6090,6 +6090,25 @@ static void zext_32_to_64(struct bpf_reg_state *reg)
>  	reg_set_urange64(reg, reg_u32_min(reg), reg_u32_max(reg));
>  }
>  
> +/*
> + * The sign-extending counterpart. Signed bounds carry over directly because
> + * sign extension is monotonic over the signed 32-bit range.
> + */
> +static void sext_32_to_64(struct bpf_reg_state *reg)
> +{
> +	/*
> +	 * See comment at reg_step_check_unsigned(). Sign extension subtracts
> +	 * 2^32 from every value with bit 31 set, which preserves a residue
> +	 * mod step only when step divides 2^32, so the line cannot be kept.
> +	 * The zero-extending twin calls reg_step_check_unsigned(reg, 32) for
> +	 * the same reason; the unsigned form does not apply here because the
> +	 * result is not the low 32 bits but the low 32 bits moved.
> +	 */
> +	reg_step_reset(reg);
> +	reg->var_off = tnum_sext(reg->var_off, 4);
> +	reg_set_srange64(reg, reg_s32_min(reg), reg_s32_max(reg));
> +}
[ ... ]
> @@ -18795,9 +18843,26 @@ static void sync_linked_regs(struct bpf_verifier_env *env, struct bpf_verifier_s
>  			scalar32_min_max_add(reg, &fake_reg);
>  			scalar_min_max_add(reg, &fake_reg);
>  			reg->var_off = tnum_add(reg->var_off, fake_reg.var_off);
> -			if (reg->link_kind == BPF_REG_LINK_ZEXT ||
> -			    known_reg->link_kind == BPF_REG_LINK_ZEXT)
> +			if (reg->link_kind == BPF_REG_LINK_SEXT) {
> +				sext_32_to_64(reg);
> +			} else if (reg->link_kind == BPF_REG_LINK_ZEXT ||
> +				   known_reg->link_kind == BPF_REG_LINK_ZEXT ||
> +				   known_reg->link_kind == BPF_REG_LINK_SEXT) {
> +				/*
> +				 * Either @reg is itself the zero extension, or
> +				 * @reg has no 32-bit kind and @known_reg does
> +				 * -- the reverse direction, where the guard
> +				 * above has already proved @reg fits 32 bits.
> +				 * Then @reg is @known_reg's low half with
> +				 * nothing above it, whichever way @known_reg
> +				 * filled its own high half, so the zero
> +				 * extension is the reconstruction for both
> +				 * kinds. BPF_REG_LINK_FULL is not here on
> +				 * purpose: it relates all 64 bits and must not
> +				 * be truncated.
> +				 */
>  				zext_32_to_64(reg);
> +			}
>  			/*
>  			 * Intersect rather than overwrite. A BPF_REG_LINK_ZEXT
>  			 * link over a wide base does not propagate back, so the

[Severity: Medium]
Can sync_linked_regs() leave reg with CNUM64_EMPTY and CNUM32_EMPTY after
reg_bounds_sync(), triggering a register invariants violation on the next
instruction that inspects the register?

At the end of the differencing block in 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);

range_bounds_violation(reg) is only checked before reg_bounds_sync(reg),
and tnum_intersect() is called without checking tnum_overlap(). When a
contradiction occurs across domains (between r64, r32, and var_off) rather
than within r64 or r32 individually, the pre-sync range_bounds_violation()
check passes, and reg_bounds_sync(reg) then turns the cross-domain
disjointness into CNUM64_EMPTY / CNUM32_EMPTY without restoring prev.

This can be triggered in two ways with BPF_REG_LINK_SEXT:

1. Cross-domain contradiction between r64 and r32 via sext_32_to_64():

   Suppose r6 is a 64-bit unknown scalar and r7 = (s32)r6 links r7 to r6
   with BPF_REG_LINK_SEXT.

   - Narrow r7 with signed 64-bit jumps to [-0x7ffffff0, 0x7ffffff0], so
     r7.r64 is [-0x7ffffff0, 0x7ffffff0] and r7.r32 is the arc
     [0x80000010, 0x7ffffff0].
   - Execute if w6 < 0x7ffffff0 goto out. Because [0x7ffffff0, 0xffffffff]
     and [0x80000010, 0x7ffffff0] overlap in two disjoint arcs,
     cnum32_intersect() over-approximates by keeping the smaller arc
     [0x7ffffff0, 0xffffffff] for r6.r32 and r7.r32, while r7.r64 remains
     [-0x7ffffff0, 0x7ffffff0].
   - Execute if w6 != w8 goto out where w8 has r32 = [0x7ffffff8, 0x80000007].
   - In sync_linked_regs() for r7, reg->r32 is [0x7ffffff8, 0x80000007],
     which straddles S32_MAX / S32_MIN. sext_32_to_64(reg) sets reg->r64 to
     [S32_MIN, S32_MAX].
   - Intersecting reg->r64 with prev.r64 gives [-0x7ffffff0, 0x7ffffff0]
     (non-empty) and intersecting reg->r32 with prev.r32 gives
     [0x7ffffff8, 0x80000007] (non-empty).
   - range_bounds_violation(reg) is false, so *reg = prev is skipped.
   - reg_bounds_sync(reg) calls deduce_bounds_32_from_64(reg), which
     intersects [0x7ffffff8, 0x80000007] with [0x80000010, 0x7ffffff0] and
     leaves reg->r32 = CNUM32_EMPTY and reg->r64 = CNUM64_EMPTY.

2. Contradiction between var_off and r64/r32 via tnum_intersect():

   Suppose r6 is a 64-bit unknown scalar, r7 = (s32)r6, and r7 is narrowed
   to [-4, 3].

   - Execute if w6 & 8 goto out. On the fallthrough branch,
     regs_refine_cond_op() calls __mark_reg_unbounded(&r6) and clears bit 3
     in var_off; sync_linked_regs() narrows r7 to [0, 3] with
     var_off = TNUM(0, 3).
   - Execute if w6 & 4 goto target. On the taken branch, r6 gets low-32
     var_off = TNUM(4, 0xfffffff3) and r32 = [-0x7ffffffc, 0x7ffffff7].
   - In sync_linked_regs() on r7, cnum64_intersect_with() and
     cnum32_intersect_with() both produce [0, 3] (non-empty), while
     tnum_intersect() without a tnum_overlap() check combines
     TNUM(4, 0xfffffffffffffff3) and TNUM(0, 3) into TNUM(4, 3) ([4, 7]).
   - range_bounds_violation(reg) is false before reg_bounds_sync(reg), and
     __update_reg_bounds() inside reg_bounds_sync(reg) then intersects
     [0, 3] with [4, 7], leaving r7 with CNUM32_EMPTY and CNUM64_EMPTY.

Should sync_linked_regs() check !tnum_overlap(reg->var_off, prev.var_off)
and also check range_bounds_violation(reg) after reg_bounds_sync(reg) before
committing the updated state?

>  			/*
>  			 * Intersect rather than overwrite. A BPF_REG_LINK_ZEXT
>  			 * link over a wide base does not propagate back, so the

[ ... ]

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