Re: [PATCH bpf-next v2 12/13] bpf: track low-32 scalar equality across narrowing stack spills

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

On 9/10/26 10:05 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] Sign-extending loads from 32-bit narrowing spills incorrectly inherit and preserve a `SUBREG_ZEXT` link, causing a critical discrepancy between verifier bounds and runtime values.
> --
>
> commit 3396576ba1c56571ccd9a040716058594f558da8
> Author: Vineet Gupta <vineet.gupta@linux.dev>
>
> bpf: track low-32 scalar equality across narrowing stack spills
>
> This commit allows the verifier to track a SUBREG_ZEXT relationship when a
> 64-bit scalar is narrowed during a 4-byte stack spill. This preserves the
> knowledge of the lower 32 bits for subsequent loads.
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 6cb35fc0d0fb0..9e63eddb9023b 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>> @@ -3678,8 +3687,15 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
>>   			 * it came from.
>>   			 */
>>   			coerce_reg_to_size(&state->stack[spi].spilled_ptr, size);
>> -			/* Break the relation on a narrowing spill. */
>> -			clear_scalar_id(&state->stack[spi].spilled_ptr);
>> +			if (subreg_link && reg->id)
>> +				state->stack[spi].spilled_ptr.subreg = SUBREG_ZEXT;
> [Severity: Critical]
> Can this lead to verifier state corruption when the spilled value is later
> read with a 4-byte sign-extending load (BPF_MEMSX)?
>
> If a BPF program performs 64-bit arithmetic to create an unknown scalar and
> spills it to the stack via a 4-byte narrowing store, it records a SUBREG_ZEXT
> link here and coerces its bounds to 32 bits.
>
> If it then reads it back via a 4-byte sign-extending load, when processing
> the read in check_stack_read_fixed_off(), narrowing evaluates to false since
> the bounds were already truncated during the spill:
>
> 	bool narrowing = get_reg_width(reg) > size * BITS_PER_BYTE;
>
> As a result, subreg_link is false, and the verifier skips the
> is_ldsx ? SUBREG_SEXT : SUBREG_ZEXT correction logic. The destination
> register blindly inherits the stack slot's SUBREG_ZEXT link.
>
> In check_mem_access(), coerce_reg_to_size_sx() properly sign-extends the
> loaded value's bounds, but because the subreg link is non-zero, it skips
> breaking the scalar ID:
>
> 	if (!no_sext && !regs[value_regno].subreg)
> 		clear_scalar_id(&regs[value_regno]);
>
> The register now has sign-extended bounds but a SUBREG_ZEXT link to the
> original 64-bit scalar.
>
> If the original scalar is later refined, sync_linked_regs() will invoke
> reconstruct_zext32(), completely overwriting the sign-extended bounds with
> zero-extended bounds. The verifier will incorrectly assume the register holds
> a positive value, while at runtime it holds a negative sign-extended value.
>
> Does this permit arbitrary out-of-bounds kernel memory accesses?

Yes there's a bug here. A narrowing spill records SUBREG_ZEXT, 
truncating the slot, and a later 4-byte ldsx has narrowing == false, 
inherits the slot's SUBREG_ZEXT  leaving sign-extended bounds under a 
zero-extending link.

Fixed by making the sign-extension path drop any link that is not 
already SUBREG_SEXT:

   -   if (!no_sext && !regs[value_regno].subreg)
   +   if (!no_sext && regs[value_regno].subreg != SUBREG_SEXT)
               clear_scalar_id(&regs[value_regno]);

Also added a test to that effect.

Thx,
-Vineet

>
>> +			else
>> +				/*
>> +				 * Nothing to relate: either the source has no
>> +				 * id to share, or the store is narrower than
>> +				 * the 32 bits a link can describe.
>> +				 */
>> +				clear_scalar_id(&state->stack[spi].spilled_ptr);
>>   		}