Re: [PATCH bpf-next v2 09/11] selftests/bpf: Test trampolines on more BPF functions with stack args

[Yonghong Song <yonghong.song@linux.dev>]

On 10/13/26 2:51 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] bpf: check_unreliable_func_args incorrectly counts slots, allowing corrupting trampoline
> --
>
> commit c87f5bcaff5cc54c864fa434bbfefe8d6bff2bbc
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> selftests/bpf: Test trampolines on more BPF functions with stack args
>
> Add two functions to the BPF target program of the int128_tgt_prog test.
> callee_split() takes a struct holding an __int128 after five u64s, so
> on x86-64 it is passed split between r9 and the stack.
>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
>> index ad46eabe784f8..e68fd5627ec1c 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
> [ ... ]
>> +/*
>> + * A static function whose BTF is unreliable gets a trampoline that only knows
>> + * of the register arguments, so one taking stack arguments must be refused.
>> + */
>> +static void test_tgt_prog_unreliable(void)
>> +{
> [ ... ]
>> +	err = bpf_program__set_attach_target(tracer->progs.unreliable_entry, fd,
>> +					     "callee_unreliable");
>> +	if (!ASSERT_OK(err, "set_attach_target"))
>> +		goto out;
>> +
>> +	err = tracing_struct_int128_tracer__load(tracer);
>> +	ASSERT_ERR(err, "tracer__load");
>> +	ASSERT_HAS_SUBSTR(log_buf, "unreliable BTF and stack arguments", "log");
> [Severity: High]
> While this test accurately checks callee_unreliable() which takes 6 scalar
> arguments, is there a potential issue in the verifier's
> check_unreliable_func_args() when dealing with struct arguments that occupy
> multiple slots?
>
> In kernel/bpf/verifier.c:check_unreliable_func_args(), if
> btf_distill_func_proto() fails (e.g. because an argument is a struct larger
> than 16 bytes), the fallback path counts the number of arguments instead
> of 8-byte slots:
>
> 	if (btf_distill_func_proto(NULL, btf, func_proto, tname, &m)) {
> 		/* at least a slot for each argument */
> 		nslots = btf_type_vlen(func_proto);
> 	}
>
> For a BPF subprogram taking a 32-byte struct by value and 4 other arguments
> (5 arguments total), the actual slot count is 1 + 1 + 1 + 1 + 4 = 8 slots
> (meaning 3 slots are on the stack). But the fallback calculates nslots = 5.

The compiler should already change 32-byte struct to a pointer to struct,
so the actual slot count should be 5. So we should be okay.

>
> Since 5 <= MAX_BPF_FUNC_REG_ARGS (5), check_unreliable_func_args()
> incorrectly returns 0, permitting attachment.
>
> The generated trampoline will not save or restore the stack arguments.
>
> Does this cause the target BPF subprogram to read uninitialized stack
> memory from the trampoline frame as its arguments, leading to data corruption
> and undefined behavior?
>