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

[Vineet Gupta <vineet.gupta@linux.dev>]

On 10/20/26 9:57 AM, sashiko-bot@kernel.org wrote:
> 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?

Yes and actually two things:

  - tnum_intersect() has no tnum_overlap() precheck, so two disagreeing 
known bits become a known 1 rather than a detected contradiction.
  - range_bounds_violation() tests only cnum32/64_is_empty, and it runs 
*before* reg_bounds_sync(). A contradiction that only materialises 
inside the sync through __update_reg_bounds() or 
deduce_bounds_32_from_64() is therefore not caught by the restore that 
4/12 added.

So the guard added in 4/12 is narrower than the problem it is named for.

The reproducer in your 4/12 report does not fire, though, and the log 
says why. Built as a socket filter on a patch-10 kernel it verifies 
clean, and the walk goes 0..7 then straight to 10:

       5: R7=scalar(id=1+0,...,var_off=(0x4; 0x1))
       6: (45) if r6 & 0x2 goto pc+1
       6: R6=scalar(id=1,...,var_off=(0x4; 0xffffffff00000001))
       7: (05) goto pc+2
      10: (b7) r0 = 0

The "r9 = 0; r7 %= r9" pair at 8 and 9 is never walked. r6's var_off has 
bit 1 known zero: value 0x4, and bit 1 is not in the mask, so "if r6 & 
2" is resolved statically and the contradicting path is pruned.

r6 knows that because of 8/12 itself: its reverse arm pinned the base's 
low half from r7, which is why R6 carries smin32/umin32=4, 
smax32/umax32=5 and the low var_off (0x4; 0x1) by instruction 6. The 
same issue hid the original 4/12 bug, 8/12 keeps masking things rather 
than fixing them.

Rather than keep on chasing which reproducers survives patch 8's 
masking, v4 does what you suggested at the end of the 4/12 report: ask 
tnum_overlap() before tnum_intersect(), and check 
range_bounds_violation() after reg_bounds_sync() as well as before. Both 
arms now go through one helper:

     static void sync_bounds_or_revert(struct bpf_reg_state *reg,
                       const struct bpf_reg_state *prev)
     {
         if (range_bounds_violation(reg)) {
             *reg = *prev;
             return;
         }
         reg_bounds_sync(reg);
         if (range_bounds_violation(reg))
             *reg = *prev;
     }

Thx,
-Vineet



> 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
> [ ... ]
>