Re: [RFC bpf-next 6/6] selftests/bpf: cover 32-bit sign-extension low-32 links

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The failure tests `sext_dest_driven_does_not_narrow_base` and `zext_dest_driven_does_not_narrow_base` contain backwards branch conditions, making them unconditionally pass regardless of whether the verifier bug is present.
--

--- Patch [6]: [RFC bpf-next 6/6] selftests/bpf: cover 32-bit sign-extension low-32 links ---
commit f8b3195655d59866de20fa031cd9d9e1bbfbde94
Author: Vineet Gupta <[email protected]>
Subject: selftests/bpf: cover 32-bit sign-extension low-32 links

This commit adds selftests for the BPF_FLAG_SUBREG_SEXT link in the BPF
verifier. The tests verify various sign-extension behaviors, including
deduction through links, zero-extension interaction, and in-loop behavior.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> index 2cc6f9e45aff3..ff71e168d4cc4 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c

[ ... ]

> @@ -826,4 +826,416 @@ l_out_%=:							\
>  	: __clobber_all);
>  }
>  

[ ... ]

> +SEC("socket")
> +__failure __msg("div by zero")
> +__flag(BPF_F_TEST_STATE_FREQ)
> +__naked void sext_dest_driven_does_not_narrow_base(void)
> +{
> +	asm volatile ("						\
> +	call %[bpf_get_prandom_u32];				\
> +	r6 = r0;		/* r6 low = unknown u32 */	\
> +	call %[bpf_get_prandom_u32];				\
> +	r0 <<= 32;						\
> +	r6 |= r0;		/* r6 = full 64-bit unknown (base) */ \
> +	r7 = (s32)r6;		/* low-32 SEXT link */		\
> +	if r7 != 0 goto l_out_%=;/* r7 == 0: low 32 bits are 0 */ \
> +	if r6 != 0 goto l_out_%=;/* r6 may still have high bits set */ \
> +	r0 /= 0;		/* must stay reachable */	\
> +l_out_%=:							\
> +	r0 = 0;							\
> +	exit;							\
> +"	:
> +	: __imm(bpf_get_prandom_u32)
> +	: __clobber_all);
> +}

[Severity: Medium]
Are these branch conditions backward for a __failure test that expects a
"div by zero" rejection?

Since the test expects the division to be reachable for rejection, if the
buggy verifier deduces r6 == 0, "if r6 != 0 goto l_out_%=" will evaluate as
false and fall through to the division by zero, causing a rejection and
making the test pass.

If the verifier is correct, the fall-through is also reachable, causing a
rejection and passing the test.

Does this mean the test passes unconditionally regardless of whether the
verifier bug is present, causing a regression in test coverage?

Should this check instead be "if r6 == 0 goto l_out_%="? This way, if the
verifier makes the buggy deduction that r6 == 0, it takes the branch and
the div becomes unreachable, allowing the verifier to accept the program and
fail the test as intended.

It looks like the identical condition also exists in the pre-existing
zext_dest_driven_does_not_narrow_base() test just above this:

	w7 = w6;		/* low-32 ZEXT link */		\
	if r7 != 0 goto l_out_%=;/* r7 == 0: low 32 bits are 0 */ \
	if r6 != 0 goto l_out_%=;/* r6 may still have high bits set */ \
	r0 /= 0;		/* must stay reachable */	\

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6