Re: [PATCH bpf-next v3 11/12] [do not merge] bpf: Track low-32 scalar equality across narrowing stack spills

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

On 10/20/26 9:42 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] bpf: zext_32_to_64() on 4-byte spill loses signed bounds on ldsx fill
> --
>
> commit b63f0039bbb8ede3ae56c9d69df1283e246bde61
> Author: Vineet Gupta <vineet.gupta@linux.dev>
>
> bpf: Track low-32 scalar equality across narrowing stack spills
>
> This patch tracks low-32 scalar equality across 4-byte narrowing stack
> spills by assigning a scalar ID, truncating the spilled slot with
> zext_32_to_64(), and setting link_kind to BPF_REG_LINK_ZEXT.
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 083b5bd70d3fa..de2a5c048a29a 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>> @@ -3927,15 +3928,46 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
>>   	mark_stack_slot_scratched(env, spi);
>>   	if (reg && !(off % BPF_REG_SIZE) && reg->type == SCALAR_VALUE && env->bpf_capable) {
>>   		bool reg_value_fits;
>> +		bool subreg_link;
>>   
>>   		reg_value_fits = get_reg_width(reg) <= BITS_PER_BYTE * size;
>> +		/*
>> +		 * A narrowing spill stores the low 32 bits of the source, so
>> +		 * the slot is their zero-extension: record a low-32 link
>> +		 * rather than dropping the relation, as a 32-bit mov does.
>> +		 * A store never sign-extends, so there is only one link kind here.
>> +		 */
>> +		subreg_link = !reg_value_fits && size == 4;
>> +
>>   		/* Make sure that reg had an ID to build a relation on spill. */
>> -		if (reg_value_fits)
>> +		if (reg_value_fits || subreg_link)
>>   			assign_scalar_id_before_mov(env, reg);
>>   		save_register_state(env, state, spi, reg, size);
>> -		/* Break the relation on a narrowing spill. */
>> -		if (!reg_value_fits)
>> -			bpf_clear_scalar_id(&ss->spilled_ptr);
>> +		if (!reg_value_fits) {
>> +			if (subreg_link && reg->id) {
>> +				/*
>> +				 * save_register_state() copies the source
>> +				 * whole, but the store only wrote its low 32
>> +				 * bits, so truncate to those before recording
>> +				 * the kind: a slot asserting BPF_REG_LINK_ZEXT
>> +				 * over the source's wider bounds is a register
>> +				 * state that contradicts itself.
>> +				 *
>> +				 * Keep the source's delta, though: the slot
>> +				 * holds (u32)(base + delta), so resetting it to
>> +				 * 0 would record a relation off by that delta.
>> +				 */
>> +				zext_32_to_64(&ss->spilled_ptr);
>> +				ss->spilled_ptr.link_kind = BPF_REG_LINK_ZEXT;
> [Severity: Medium]
> Does calling zext_32_to_64() on ss->spilled_ptr cause a precision
> regression when the 4-byte slot is subsequently filled by a sign-extending
> load (ldsx)?

Yes precision regression and it also changes a verdict.

Say r6 is given [-16, 15], a small signed range straddling zero, spilled 
4 bytes and filled back with ldsx.
On a patch-10 kernel the program verifies; with 11/12 applied it is 
rejected, because the fill no longer knows r2 <= 15:

     p10      sext_fill_keeps_small_signed_range   success
     p10+11   sext_fill_keeps_small_signed_range   failure ("div by zero")

Thx,
-Vineet

> When spilling a scalar register whose signed range straddles zero (for
> example [-10, 10], where reg_umax(reg) is U64_MAX and get_reg_width(reg)
> is 64) with a 4-byte store (*(u32 *)(r10 - 8) = rX), subreg_link is true
> and zext_32_to_64(&ss->spilled_ptr) is called:
>
> static void zext_32_to_64(struct bpf_reg_state *reg)
> {
> 	reg_step_check_unsigned(reg, 32);
> 	reg->var_off = tnum_subreg(reg->var_off);
> 	reg_set_urange64(reg, reg_u32_min(reg), reg_u32_max(reg));
> }
>
> Because r32 = [-10, 10] straddles zero in unsigned 32-bit arithmetic
> (u32_min = 0, u32_max = 0xffffffff), zext_32_to_64() overwrites
> ss->spilled_ptr.r64 with [0, 0xffffffff] while leaving [-10, 10] only in
> r32.
>
> If the slot is then reloaded with a sign-extending load (*(s32 *)(r10 - 8),
> *(s16 *)(r10 - 8), or *(s8 *)(r10 - 8)), check_stack_read_fixed_off()
> copies ss->spilled_ptr to the destination register and check_mem_access()
> calls coerce_reg_to_size_sx():
>
> static void coerce_reg_to_size_sx(struct bpf_reg_state *reg, int size)
> {
> 	...
> 	/* If every value fits the signed N-bit range sign extension is a noop. */
> 	if (reg_smin(reg) >= -limit && reg_smax(reg) < limit)
> 		return;
>
> Because coerce_reg_to_size_sx() inspects reg_smin(reg) and reg_smax(reg)
> from r64 rather than r32, smax is 0xffffffff (>= limit), so the early
> return no longer triggers and coerce_reg_to_size_sx() falls through to
> set_sext64_default_val(), resetting both r64 and r32 to [S32_MIN, S32_MAX]
> (or the full S16/S8 range).