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

[sashiko-bot@kernel.org]

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

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).

> +			} 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.
> +				 */
> +				bpf_clear_scalar_id(&ss->spilled_ptr);
> +			}
> +		}
>  	} else if (!reg && !(off % BPF_REG_SIZE) && is_bpf_st_mem(insn) &&
>  		   env->bpf_capable) {
>  		struct bpf_reg_state *tmp_reg = &env->fake_reg[0];
[ ... ]

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