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

[sashiko-bot@kernel.org]

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.

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?

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