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

On 4/16/26 9:38 PM, [email protected] wrote:
> 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 <[email protected]>
>
> 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.

Dead code. Will remove.

>
> [ ... ]
>
>> +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.

The read must be 'r11 + 8' and store will be 'r11 - <positive_off>'.

>
> [ ... ]
>
>> +__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?

4-byte stack argument is not supported.

>
> [ ... ]
>
>> +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?

yes, I can do r0 += 8 instead.

>
> [ ... ]
>
>> +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?

Will fix.