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

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

On 9/10/26 10:34 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] Sign-extending loads incorrectly preserve conflicting SUBREG_ZEXT metadata.
> --
>
> commit f939550a777ae3d2a7392dec744bffdc9fabcdda
> Author: Vineet Gupta <vineet.gupta@linux.dev>
>
> bpf: track low-32 scalar equality across narrowing stack fills
>
> This commit records a low-32 link for stack fills narrower than the wider
> source, allowing the verifier to deduce relations between 32-bit narrows
> and their wider origins. It also adjusts check_mem_access() to not clear
> the id of a ->subreg register on a sign-extending load.
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 308ff53232f06..89be1240c99ae 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>
>> @@ -3968,18 +3969,34 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
>>   
>>   			if (size <= spill_size &&
>>   			    bpf_stack_narrow_access_ok(off, size, spill_size)) {
>> -				if (env->bpf_capable && size == 4 && spill_size == 4 &&
>> -				    get_reg_width(reg) <= 32)
>> +				bool narrowing = get_reg_width(reg) > size * BITS_PER_BYTE;
>> +				/*
>> +				 * A narrowing fill keeps only the slot's low 32 bits,
>> +				 * so record a low-32 link rather than dropping the
>> +				 * relation, as a 32-bit mov from a wide source does.
>> +				 * Which kind depends on how the load fills the high
>> +				 * half, hence is_ldsx.
>> +				 */
>> +				bool subreg_link = narrowing && size == 4;
>> +
>> +				if (env->bpf_capable && size == 4 &&
>> +				    (subreg_link || (spill_size == 4 && !narrowing)))
>>   					/* Ensure stack slot has an ID to build a relation
>>   					 * with the destination register on fill.
>>   					 */
>>   					assign_scalar_id_before_mov(env, reg);
>>   				state->regs[dst_regno] = *reg;
>>   
>> -				/* Break the relation on a narrowing fill.
>> -				 * coerce_reg_to_size will adjust the boundaries.
>> -				 */
>> -				if (get_reg_width(reg) > size * BITS_PER_BYTE)
>> +				if (subreg_link && reg->id)
>> +					state->regs[dst_regno].subreg =
>> +						is_ldsx ? SUBREG_SEXT : SUBREG_ZEXT;
>> +				else if (narrowing)
>> +					/*
>> +					 * Nothing to relate: either the slot has
>> +					 * no id to share, or the fill is narrower
>> +					 * than the 32 bits a link can describe.
>> +					 * coerce_reg_to_size adjusts the bounds.
>> +					 */
>>   					clear_scalar_id(&state->regs[dst_regno]);
> [Severity: Critical]
> When narrowing is false, does this preserve a stale SUBREG_ZEXT property for
> a sign-extending load?
>
> If a register with SUBREG_ZEXT is spilled to the stack and then read back
> via a 4-byte sign-extending load, narrowing evaluates to false. This allows
> the destination register to inherit the SUBREG_ZEXT property directly from
> the stack slot, even though it is being sign-extended.
>
> [ ... ]

Indeed if narrowing is false, wrong metadata can potentially be 
introduced, a comment to that effect is added here.

    /*
      * A fill that is not narrowing copies the slot's
      * kind verbatim, which a sign-extending load can
      * contradict. check_mem_access() runs after this
      * and drops any kind but SUBREG_SEXT once it knows
      * the extension was not a no-op.
      */


See the second site below for actual fix...

>> @@ -6736,13 +6753,15 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, struct b
>>   			 * Sign-extension can change the register value relative
>>   			 * to a scalar it is linked with by id (e.g. a zero-
>>   			 * extending fill of the same spilled stack slot), thus
>> -			 * drop the shared id in that case.
>> +			 * drop the shared id in that case. A ->subreg link is
>> +			 * the exception: it already records that only the low
>> +			 * 32 bits are shared, and how the high half follows.
>>   			 */
>>   			bool no_sext = reg_umax(&regs[value_regno]) <
>>   					(1ULL << (size * BITS_PER_BYTE - 1));
>>   
>>   			coerce_reg_to_size_sx(&regs[value_regno], size);
>> -			if (!no_sext)
>> +			if (!no_sext && !regs[value_regno].subreg)
>>   				clear_scalar_id(&regs[value_regno]);
> [Severity: Critical]
> Does this check wrongly preserve the ID for SUBREG_ZEXT registers that
> have just been sign-extended?
>
> Because SUBREG_ZEXT is non-zero, !regs[value_regno].subreg evaluates to
> false, skipping the clear_scalar_id() call for a register that might hold
> a sign-extended negative value.
>
> Later, if a conditional branch triggers sync_linked_regs(), it will see
> SUBREG_ZEXT and invoke reconstruct_zext32(). This would forcefully set the
> bounds to a zero-extended positive value.
>
> At runtime, the discrepancy between the verifier's zero-extended positive
> bounds and the actual sign-extended negative value could bypass bounds checks,
> potentially leading to out-of-bounds memory reads or writes.
>
> Should this explicitly check for SUBREG_SEXT rather than just allowing any
> non-zero ->subreg value?
Exactly that. And the first path leads here so the comment above suffices.

Thx,
-Vineet