Re: [PATCH bpf-next 4/4] selftests/bpf: Verify classic uprobe attach types

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: classic uprobes with expected_attach_type 0 still use private stack
- [High] bpf: unchecked kprobe expected_attach_type breaks uprobe feature probe
--

commit 9ee5232694f8a364dae370cb864edf0a6d7ff502
Author: Jiri Olsa <jolsa@kernel.org>

selftests/bpf: Verify classic uprobe attach types

This commit adds assertions to the uprobe selftest to check that loaded
classic uprobe and uretprobe programs keep BPF_TRACE_UPROBE as their
expected attach type.

> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe.c b/tools/testing/selftests/bpf/prog_tests/uprobe.c
> index 86404476c1daf..7d372552f5eab 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe.c
> @@ -44,6 +44,10 @@ static void test_uprobe_attach(void)
>  	skel = test_uprobe__open_and_load();
>  	if (!ASSERT_OK_PTR(skel, "skel_open"))
>  		return;
> +	ASSERT_EQ(bpf_program__expected_attach_type(skel->progs.test1),
> +		  BPF_TRACE_UPROBE, "uprobe_attach_type");
> +	ASSERT_EQ(bpf_program__expected_attach_type(skel->progs.test3),
> +		  BPF_TRACE_UPROBE, "uretprobe_attach_type");

[Severity: High]
Does the feature probe behind this expected_attach_type assignment actually
detect kernel support for BPF_TRACE_UPROBE?

In tools/lib/bpf/libbpf.c, libbpf_prepare_prog_load() sets
expected_attach_type when FEAT_UPROBE_ATTACH_TYPE is supported:

	/* set uprobe attach_type if supported */
	if ((def & SEC_UPROBE) && kernel_supports(prog->obj, FEAT_UPROBE_ATTACH_TYPE)) {
		prog->expected_attach_type = BPF_TRACE_UPROBE;
		opts->expected_attach_type = BPF_TRACE_UPROBE;
	}

In tools/lib/bpf/features.c, probe_kern_uprobe_attach_type() probes this by
calling bpf_prog_load() with BPF_PROG_TYPE_KPROBE and
opts.expected_attach_type = BPF_TRACE_UPROBE:

	fd = bpf_prog_load(BPF_PROG_TYPE_KPROBE, NULL, "GPL", insns,
			   ARRAY_SIZE(insns), &opts);
	return probe_fd(fd);

However, in kernel/bpf/syscall.c, bpf_prog_load_check_attach() does not have
a case for BPF_PROG_TYPE_KPROBE and falls into the default case:

	case BPF_PROG_TYPE_SYSCALL:
	case BPF_PROG_TYPE_EXT:
		if (expected_attach_type)
			return -EINVAL;
		fallthrough;
	default:
		return 0;

Because BPF_PROG_LOAD does not validate expected_attach_type for
BPF_PROG_TYPE_KPROBE, wouldn't probe_kern_uprobe_attach_type() always
succeed on older kernels too, causing libbpf to unconditionally set
BPF_TRACE_UPROBE while current kernels accept arbitrary invalid
expected_attach_type values for kprobe programs? Also, since
test_uprobe_attach() only checks the userspace getter
bpf_program__expected_attach_type(), should the test also verify that the
kernel rejects attaching a BPF_TRACE_UPROBE program to a non-uprobe perf
event?

[Severity: Critical]
In addition, can a classic uprobe program loaded with expected_attach_type
set to 0 still be assigned a per-CPU private stack and corrupt stack memory?

In kernel/bpf/verifier.c, bpf_enable_priv_stack() only disables the private
stack when expected_attach_type is explicitly set to one of the uprobe
attach types:

	case BPF_PROG_TYPE_KPROBE:
		/* Disable private stack for uprobe programs. */
		if (prog->expected_attach_type == BPF_TRACE_UPROBE ||
		    prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI ||
		    prog->expected_attach_type == BPF_TRACE_UPROBE_SESSION)
			return NO_PRIV_STACK;
		fallthrough;

And in kernel/events/core.c, __perf_event_set_bpf_prog() still allows
attaching a BPF_PROG_TYPE_KPROBE program with expected_attach_type == 0 (and
jits_use_priv_stack == true) to a uprobe perf event:

	if (prog->expected_attach_type == BPF_TRACE_UPROBE && !is_uprobe)
		return -EINVAL;

	if (prog->type == BPF_PROG_TYPE_KPROBE && prog->sleepable && !is_uprobe)
		/* only uprobe programs are allowed to be sleepable */
		return -EINVAL;

If an existing userspace loader (or a sleepable BPF_PROG_TYPE_KPROBE program
loaded with expected_attach_type == 0) loads a program with stack depth >=
BPF_PRIV_STACK_MIN_SIZE and attaches it to a uprobe event, won't it still
receive PRIV_STACK_ADAPTIVE and overwrite the per-CPU private stack if
preempted on the same CPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103093400.112178-1-jolsa@kernel.org?part=4