Re: [PATCH bpf-next v3 10/12] selftests/bpf: Cover the low-32 link for narrowing stack fills

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4456 bytes --]

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

TL;DR: Test issue: sext_fill_does_not_inherit_zext_link appears never to put a
zero-extending link on the stack slot, so it can't distinguish the new
check_mem_access() condition from the old one; its comment and commit message
look inaccurate.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> index 69efe1381eca..ab78285153fb 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c

[ ... ]

> @@ -1494,6 +1591,215 @@ __naked void sext_drops_stale_step(void)

[ ... ]

> +/*
> + * A sign-extending fill must not come away with a zero-extending link. The
> + * slot below already fits 32 bits, so the fill is not narrowing and assigns
> + * no link kind of its own, and "state->regs[dst] = *reg" copies the slot's
> + * verbatim; check_mem_access() is what has to drop it, since it would leave
> + * r2 recording a zero-extension of a value it has just sign-extended.
> + * Asserted on the register dump rather than on a guarded branch, so it pins
> + * the rule itself and not a downstream effect.
> + */
> +SEC("socket")
> +__success __log_level(2)
> +/* a copied link would print as R2=scalar(id=N+0,smin=... */
> +__msg("(81) r2 = *(s32 *)(r10 {{.*}} R2=scalar(smin=")
> +__naked void sext_fill_does_not_inherit_zext_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 */	\
> +	w7 = w6;		/* r7 = zext32(r6), fits 32 bits */ \
> +	*(u64 *)(r10 - 8) = r7;					\

Does this test ever put a zero-extending link on the slot?

The w7 = w6 above does give r7 BPF_REG_LINK_ZEXT, but the spill that follows
clears it.  In check_stack_write_fixed_off(), r7 fits in 32 bits, so
assign_scalar_id_before_mov() runs before save_register_state():

    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;

The slot ends up with a fresh id and link_kind == BPF_REG_LINK_NONE.  The
4-byte ldsx is not narrowing, so the fill copies a plain id, and
check_mem_access() drops it through the existing !no_sext clear.  The
link_kind == BPF_REG_LINK_SEXT part of the new condition never sees a ZEXT
kind here.

This leaves two problems.

First, the in-test comment looks wrong:

    /* a copied link would print as R2=scalar(id=N+0,smin=... */

print_reg_state() prints +delta only when link_kind is set, so a surviving
plain id would print as R2=scalar(id=N,smin=...

Second, the commit message says "The last two pin the link-kind checks in
check_mem_access()", but this test cannot tell the new check from the old
one.  It passes with the pre-series:

    if (!no_sext)
        bpf_clear_scalar_id(...)

It also passes with a relaxed condition such as:

    !(size == 4 && regs[value_regno].link_kind != BPF_REG_LINK_NONE)

which would let a ZEXT link survive a sign-extending fill.

At this commit no spill/fill sequence appears able to bring a
BPF_REG_LINK_ZEXT kind into the ldsx arm at all.  Spills strip every non-SEXT
kind via assign_scalar_id_before_mov(), and a 4-byte fill of a 4-byte slot
does the same on the slot.  A narrower fill goes through the narrowing arm
instead.

That holds even after 3649346488fc ("bpf: Track low-32 scalar equality across
narrowing stack spills"), because the fill-side
assign_scalar_id_before_mov(reg) clears the slot's new ZEXT kind before the
copy.  No later commit in the range (3649346488fc, 73fbac05c8bd) changes this
test.

Should the comment, the test name and the commit message describe what is
actually pinned here (a plain id dropped across a range-changing 4-byte
ldsx)?  Or should the test be reworked once a ZEXT-carrying slot is
reachable?

> +"
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +"	r2 = *(s32 *)(r10 - 8);	/* sign-extending, not narrowing */ \
> +"
> +#else
> +"	r2 = *(s32 *)(r10 - 4);					\
> +"

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39458500836