Re: [PATCH bpf-next v5 16/16] selftests/bpf: Add verifier tests for stack argument validation

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [Medium] Negative verifier tests for stack arguments use inherently invalid memory accesses, causing them to pass even if the verifier has bugs.
- [Medium] `stack_arg_read_uninitialized` reads from `r11 + 8` instead of a valid negative offset, failing to properly test uninitialized stack argument slots.
- [Low] Unused `subprog_8args` function defined in the test file.
--

commit 24b1eb352e7d04e7c35a9fbc88c6b91d08b17832
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Add verifier tests for stack argument validation

This commit adds inline-asm based verifier tests to directly exercise
stack argument validation logic.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_arg.c b/tools/testing/selftests/bpf/progs/verifier_stack_arg.c
> new file mode 100644
> index 0000000000000..d212b6c3cac75
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_stack_arg.c

[ ... ]

> +__noinline __used
> +static int subprog_8args(int a, int b, int c, int d, int e, int f, int g, int h)
> +{
> +	return a + b + c + d + e + f + g + h;
> +}

Is subprog_8args intended to be used in any of the tests? It does not seem
to be called anywhere in this file.

[ ... ]

> +SEC("tc")
> +__description("stack_arg: read from uninitialized stack arg slot")
> +__failure
> +__arch_x86_64
> +__msg("invalid read from stack arg")
> +__naked void stack_arg_read_uninitialized(void)
> +{
> +	asm volatile (
> +		"r0 = *(u64 *)(r11 + 8);"
> +		"r0 = 0;"
> +		"exit;"
> +		::: __clobber_all
> +	);
> +}

Should this read from r11 - 8 instead? Since outgoing stack arguments are
at negative offsets from r11, reading from r11 + 8 is out of bounds and
might be rejected for the wrong reason, rather than testing an uninitialized
but valid stack argument slot.

[ ... ]

> +__noinline __used
> +static long subprog_deref_arg6(long a, long b, long c, long d, long e, long *f)
> +{
> +	return *f;
> +}

[ ... ]

> +SEC("tc")
> +__description("stack_arg: release_reference invalidates stack arg slot")
> +__failure
> +__arch_x86_64
> +__msg("R1 invalid sock access")
> +__naked void stack_arg_release_ref(void)
> +{
> +	asm volatile (
[ ... ]
> +		/* Release the reference — invalidates the stack arg slot */
> +		"r1 = r0;"
> +		"call %[bpf_sk_release];"
> +		/* Call subprog that dereferences arg6 — should fail */
> +		"r1 = 1;"
> +		"r2 = 2;"
> +		"r3 = 3;"
> +		"r4 = 4;"
> +		"r5 = 5;"
> +		"call subprog_deref_arg6;"

Can this test pass even if the verifier fails to invalidate the stack slot?

subprog_deref_arg6() performs an 8-byte read on the argument. Since BPF
sockets only allow up to 4-byte reads, an 8-byte read is always invalid.
If the reference isn't correctly invalidated, the valid socket pointer
will still be rejected with "R1 invalid sock access".

To properly test the invalidation, should the subprogram perform a valid
4-byte read instead?

[ ... ]

> +SEC("tc")
> +__description("stack_arg: pkt pointer in stack arg slot invalidated after pull_data")
> +__failure
> +__arch_x86_64
> +__msg("invalid access to packet")
> +__naked void stack_arg_stale_pkt_ptr(void)
> +{
> +	asm volatile (
> +		"r6 = r1;"
> +		"r7 = *(u32 *)(r6 + %[__sk_buff_data]);"
> +		"r8 = *(u32 *)(r6 + %[__sk_buff_data_end]);"
> +		/* check pkt has at least 1 byte */
> +		"r0 = r7;"
> +		"r0 += 1;"
> +		"if r0 > r8 goto l0_%=;"
> +		/* Store valid pkt pointer to outgoing arg6 slot */
> +		"*(u64 *)(r11 - 8) = r7;"
> +		/* bpf_skb_pull_data invalidates all pkt pointers */
> +		"r1 = r6;"
> +		"r2 = 0;"
> +		"call %[bpf_skb_pull_data];"
> +		/* Call subprog that dereferences arg6 — should fail */
> +		"r1 = 1;"
> +		"r2 = 2;"
> +		"r3 = 3;"
> +		"r4 = 4;"
> +		"r5 = 5;"
> +		"call subprog_deref_arg6;"

Similarly, does this properly test packet pointer invalidation? The test
verifies 1 byte of the packet pointer, but subprog_deref_arg6() reads 8 bytes.
If bpf_skb_pull_data() fails to invalidate the pointer, the 8-byte read would
still be rejected as out-of-bounds, yielding the expected "invalid access to
packet" message.

Should the packet bounds check verify at least 8 bytes so that a valid read is
guaranteed if the pointer isn't invalidated?

[ ... ]

> +SEC("tc")
> +__description("stack_arg: null propagation rejects deref on null branch")
> +__failure
> +__arch_x86_64
> +__msg("R1 invalid mem access")
> +__naked void stack_arg_null_propagation_fail(void)
> +{
> +	asm volatile (
[ ... ]
> +		"call %[bpf_map_lookup_elem];"
> +		/* Store PTR_TO_MAP_VALUE_OR_NULL to outgoing arg6 slot */
> +		"*(u64 *)(r11 - 8) = r0;"
> +		/* null check on r0 */
> +		"if r0 != 0 goto l0_%=;"
> +		/*
> +		 * On null branch, outgoing slot is SCALAR(0).
> +		 * Call subprog that dereferences arg6 — should fail.
> +		 */
> +		"r1 = 0;"
> +		"r2 = 0;"
> +		"r3 = 0;"
> +		"r4 = 0;"
> +		"r5 = 0;"
> +		"call subprog_deref_arg6;"

Does the __msg() substring match accept the wrong failure mode here?
Dereferencing an unrefined PTR_TO_MAP_VALUE_OR_NULL yields "R1 invalid mem
access 'map_value_or_null'", while dereferencing SCALAR yields "R1 invalid mem
access 'scalar'".

The substring match "R1 invalid mem access" accepts both, allowing the test to
pass even if null propagation fails and the register is still tracked as a map
pointer. Should it match the exact scalar error, or perhaps test the non-null
branch for success instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260417034658.2625353-1-yonghong.song@linux.dev?part=16