Re: [PATCH bpf-next v3 12/12] [do not merge] selftests/bpf: Cover the low-32 link for narrowing stack spills

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests/bpf: fill before narrow in zext_narrowing_spill_byte_no_link()
- [Medium] selftests/bpf: tautological test in sext_fill_of_narrowing_spill_drops_zext()
- [Low] selftests/bpf: stale inline comment in zext_narrowing_spill_drops_link()
--

commit a341902044eb0b749136bbae6f269759e4c0ffcd
Author: Vineet Gupta <vineet.gupta@linux.dev>

selftests/bpf: Cover the low-32 link for narrowing stack spills

This patch adds six verifier selftests to verifier_linked_scalars.c to test
low-32 register linking behavior across narrowing stack spills and subsequent
fills.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> index ab78285153fb1..8ade4836e1b2f 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> @@ -1217,6 +1217,128 @@ __naked void zext_fill_byte_forms_no_link(void)
>  	: __clobber_all);
>  }
>  

[ ... ]

> +/*
> + * A narrowing spill stores only the source's low 32 bits, so the slot is their
> + * zero-extension. Filling it back is a mov, though, and that drops the link
> + * for the same reason zext_chain_drops_link does, so a later narrowing of the
> + * source does not reach the filled register.
> + */
> +SEC("socket")
> +__failure
> +__msg("div by zero")
> +__naked void zext_narrowing_spill_drops_link(void)
> +{
> +	asm volatile ("						\
> +	call %[bpf_get_prandom_u32];				\
> +	r6 = r0;						\
> +	call %[bpf_get_prandom_u32];				\
> +	r0 <<= 32;						\
> +	r6 |= r0;		/* r6 = full 64-bit unknown */	\
> +	*(u32 *)(r10 - 8) = r6;	/* narrowing spill, forms the link */ \
> +	r2 = *(u32 *)(r10 - 8);	/* fill it back */		\
> +	if w6 != 0 goto 1f;	/* narrows r6, propagates to r2 */ \

[Severity: Low]
This isn't a bug, but is this inline comment in
zext_narrowing_spill_drops_link() left over from the earlier version of the
test? Since the preceding fill into r2 drops BPF_REG_LINK_ZEXT, narrowing w6
here does not propagate to r2.

> +	if r2 == 0 goto 1f;					\
> +	r0 /= 0;						\
> +1:								\
> +	r0 = 0;							\
> +	exit;							\
> +"	:
> +	: __imm(bpf_get_prandom_u32)
> +	: __clobber_all);
> +}
> +

[ ... ]

> +/*
> + * A sub-word spill is below the low-32 model, so no link is formed there.
> + * The fill is a byte load too, so this pins the spill-side and fill-side
> + * size gates together rather than either on its own.
> + */
> +SEC("socket")
> +__failure __msg("div by zero")
> +__naked void zext_narrowing_spill_byte_no_link(void)
> +{
> +	asm volatile ("						\
> +	call %[bpf_get_prandom_u32];				\
> +	r6 = r0;						\
> +	call %[bpf_get_prandom_u32];				\
> +	r0 <<= 32;						\
> +	r6 |= r0;						\
> +	*(u8 *)(r10 - 8) = r6;	/* 1-byte spill: no link */	\
> +	r2 = *(u8 *)(r10 - 8);					\
> +	if w6 != 0 goto 1f;					\
> +	if r2 == 0 goto 1f;	/* not deduced */		\

[Severity: Medium]
Does zext_narrowing_spill_byte_no_link() actually test the size gate in
check_stack_write_fixed_off()?

Here, r2 is filled from fp-8 before w6 is narrowed:

	*(u8 *)(r10 - 8) = r6;	/* 1-byte spill: no link */
	r2 = *(u8 *)(r10 - 8);
	if w6 != 0 goto 1f;

As demonstrated by zext_narrowing_spill_drops_link() above, filling a
BPF_REG_LINK_ZEXT slot into r2 before narrowing w6 drops the zero-extending
link on the fill, so narrowing w6 afterwards would not propagate to r2 even if
the 1-byte spill had formed a link.

Should the narrowing branch "if w6 != 0 goto 1f" happen while the value is
still on the stack before filling r2, matching the instruction order in
zext_narrowing_spill_reaches_fill()?

> +	r0 /= 0;						\
> +1:								\
> +	r0 = 0;							\
> +	exit;							\
> +"	:
> +	: __imm(bpf_get_prandom_u32)
> +	: __clobber_all);
> +}
> +
>  #ifdef CAN_USE_MOVSX

[ ... ]

> @@ -1800,6 +1952,35 @@ __naked void sext_byte_fill_drops_sext_link(void)
>  }
>  #endif /* CAN_USE_MOVSX */
>  
> +/*
> + * The same, with the zero-extending link coming from a narrowing spill rather
> + * than a mov. The spill truncated the slot to 32 bits, so the fill is not
> + * narrowing and assigns no link kind of its own; the slot's zero-extending
> + * one must not survive the sign-extending load.
> + */
> +SEC("socket")
> +__failure __msg("div by zero")
> +__naked void sext_fill_of_narrowing_spill_drops_zext(void)
> +{
> +	asm volatile ("						\
> +	call %[bpf_get_prandom_u32];				\
> +	r6 = r0;						\
> +	call %[bpf_get_prandom_u32];				\
> +	r0 <<= 32;						\
> +	r6 |= r0;		/* r6 = full 64-bit unknown */	\
> +	*(u32 *)(r10 - 8) = r6;	/* narrowing spill: slot is ZEXT */ \
> +	r2 = *(s32 *)(r10 - 8);	/* sign-extending, not narrowing */ \
> +	if w6 != -1 goto 1f;	/* slot low 32 bits are all ones */ \
> +	if r2 s>= 0 goto 1f;	/* taken only if r2 was rebuilt zext */ \
> +	r0 /= 0;						\

[Severity: Medium]
Can sext_fill_of_narrowing_spill_drops_zext() ever reach the is_ldsx link
clearing in check_mem_access() with BPF_REG_LINK_ZEXT still intact?

In check_stack_write_fixed_off(), the 4-byte narrowing spill
"*(u32 *)(r10 - 8) = r6" truncates fp-8 to 32 bits via
zext_32_to_64(&ss->spilled_ptr) and sets BPF_REG_LINK_ZEXT.

When "r2 = *(s32 *)(r10 - 8)" then runs in check_stack_read_fixed_off(),
get_reg_width(reg) is at most 32, so narrowing is false. Because size == 4,
spill_size == 4, and !narrowing all hold, check_stack_read_fixed_off() calls
assign_scalar_id_before_mov(env, reg) before copying *reg to
state->regs[dst_regno]:

kernel/bpf/verifier.c:check_stack_read_fixed_off() {
    ...
	if (env->bpf_capable && size == 4 &&
	    (subreg_link || (spill_size == 4 && !narrowing)))
		assign_scalar_id_before_mov(env, reg);
	state->regs[dst_regno] = *reg;
    ...
}

And assign_scalar_id_before_mov() immediately clears BPF_REG_LINK_ZEXT and
assigns a fresh scalar ID unlinked from r6:

kernel/bpf/verifier.c:assign_scalar_id_before_mov() {
	if (src_reg->link_kind && src_reg->link_kind != BPF_REG_LINK_SEXT)
		bpf_clear_scalar_id(src_reg);
	if (!src_reg->id && !tnum_is_const(src_reg->var_off))
		src_reg->id = ++env->id_gen;
}

So BPF_REG_LINK_ZEXT is already cleared by the normal 4-byte fill path (the
same path tested by zext_narrowing_spill_drops_link()) before the is_ldsx
handling in check_mem_access() runs.

Additionally, even if r2 kept r6's ID and BPF_REG_LINK_ZEXT past
check_mem_access(), coerce_reg_to_size_sx() restricts r2's 64-bit bounds to
[S32_MIN, S32_MAX]. When "if w6 != -1 goto 1f" calls sync_linked_regs(),
rebuilding r2 with zext_32_to_64() yields 0xffffffff, which is disjoint from
[S32_MIN, S32_MAX], causing range_bounds_violation() to revert r2 to its prior
bounds and still hit "r0 /= 0".

> +1:								\
> +	r0 = 0;							\
> +	exit;							\
> +"	:
> +	: __imm(bpf_get_prandom_u32)
> +	: __clobber_all);
> +}
> +
>  #endif /* CAN_USE_LDSX */
>  
>  char _license[] SEC("license") = "GPL";

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