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/11/26 11:37 AM, Vineet Gupta wrote:
> 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.

What I meant was first site has comment, and this site has the exact fix 
which sashiko identified.
They are both part of the same bug.

Thx