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

[sashiko-bot@kernel.org]

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.

[ ... ]

> @@ -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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910164635.459558-1-vineet.gupta@linux.dev?part=9