Re: [PATCH bpf-next v2 6/6] selftests/bpf: Add tests for fault prone loads out of RCU pointers

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added BPF programs do not execute the vulnerable instructions at runtime and lack explicit verifier log assertions, failing to provide robust regression coverage.
--

--- Patch [6]: [PATCH bpf-next v2 6/6] selftests/bpf: Add tests for fault prone loads out of RCU pointers ---
commit 6d523ae9972a5903a07a65fd29336ef6414702f5
Author: Daniel Borkmann <[email protected]>

selftests/bpf: Add tests for fault prone loads out of RCU pointers

This commit adds new BPF tests for ensuring the verifier correctly
applies the BPF_PROBE_MEM rewrite to fault-prone loads reached from an
RCU read-side critical section. Two new subprograms are added and
autoloaded in the rcu_read_lock test.

> diff --git a/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c b/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
> --- a/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
> +++ b/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
> @@ -34,6 +34,8 @@ static void test_success(void)
>  	bpf_program__set_autoload(skel->progs.rcu_read_lock_global_subprog, true);
>  	bpf_program__set_autoload(skel->progs.rcu_read_lock_subprog_lock, true);
>  	bpf_program__set_autoload(skel->progs.rcu_read_lock_subprog_unlock, true);
> +	bpf_program__set_autoload(skel->progs.non_own_ref_untrusted_ld, true);
> +	bpf_program__set_autoload(skel->progs.rcu_untrusted_union_ld, true);
>  	err = rcu_read_lock__load(skel);

[Severity: Medium]
Is `rcu_untrusted_union_ld` reliably triggered by the test harness? 

Looking at `test_success()`, the harness executes `syscall(SYS_getpgid)` to
trigger the fentry program, but there doesn't seem to be a trigger for the
`tp_btf/tcp_probe` program.

Also, would it be safer to use `test_loader` for these tests so we can
statically assert the verifier log output contains the `BPF_PROBE_MEM`
rewrite, rather than relying only on runtime execution?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/rcu_read_lock.c b/tools/testing/selftests/bpf/progs/rcu_read_lock.c
> --- a/tools/testing/selftests/bpf/progs/rcu_read_lock.c
> +++ b/tools/testing/selftests/bpf/progs/rcu_read_lock.c

[ ... ]

> +SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
> +int non_own_ref_untrusted_ld(void *ctx)
> +{
> +	struct rcu_node_stash *stash;
> +	struct rcu_node_data *node;
> +	int key = 0;
> +
> +	stash = bpf_map_lookup_elem(&node_stash, &key);
> +	if (!stash)
> +		return 0;
> +	bpf_rcu_read_lock();
> +	node = stash->node;
> +	if (!node) {
> +		bpf_rcu_read_unlock();
> +		return 0;
> +	}

[Severity: Medium]
Does this program end up returning early here on every run?

Since the `node_stash` map is never populated from the userspace test
harness, `stash->node` appears to always be NULL. If it returns early,
the test never actually reaches the fault-prone load below to trigger a
kernel panic if the rewrite were to fail.

> +	bpf_rcu_read_unlock();
> +	/*
> +	 * The unlock leaves node as PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED
> +	 * | NON_OWN_REF, and the load below has to get the BPF_PROBE_MEM
> +	 * rewrite for it, otherwise a bad address panics the kernel.
> +	 */
> +	non_own_ref_key = node->key;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6