Re: [PATCH v5 09/10] selftests/hornet: Add a selftest for the Hornet LSM

[sashiko-bot@kernel.org]

Sashiko AI review found 7 potential issue(s):
- [High] Use of deprecated `bpf_probe_read_str` to read a user-space pointer.
- [High] Hardcoded dependency on the host's `/sys/kernel/btf/vmlinux`.
- [High] Custom Makefile rules are bypassed due to `$(OUTPUT)/` prefix mismatch.
- [High] Linking against the host system's `libbpf` instead of the in-tree library.
- [High] Unconditional hardcoded dependency on kernel signing keys.
- [Medium] The selftest fails to verify Hornet's negative enforcement capabilities.
- [Medium] BPF selftest `loader.c` uses raw conditionals instead of `ASSERT_*()` macros.
--

commit 162ecc15ea5091af09f42092fc7aa704f31709fa
Author: Blaise Boscaccy <bboscaccy@linux.microsoft.com>

selftests/hornet: Add a selftest for the Hornet LSM
    
This selftest contains a testcase that utilizes light skeleton eBPF
loaders and exercises hornet's map validation.

> diff --git a/tools/testing/selftests/hornet/Makefile b/tools/testing/selftests/hornet/Makefile
> new file mode 100644
> index 0000000000000..432bce59f54e7
> --- /dev/null
> +++ b/tools/testing/selftests/hornet/Makefile

[ ... ]

> +TEST_GEN_PROGS := loader
> +TEST_GEN_FILES := vmlinux.h loader.h trivial.bpf.o map.bin sig.bin insn.bin signed_loader.h
> +$(TEST_GEN_PROGS): LDLIBS += -lbpf

Could this link against the host operating system's libbpf.so instead of the
locally built in-tree library?

It looks like it lacks the -L$(BPFDIR) library search path. If the host
machine doesn't have libbpf-dev installed, the build might fail, or it could
link against an outdated version.

[ ... ]

> +vmlinux.h:
> +	$(BPFTOOL) btf dump file /sys/kernel/btf/vmlinux format c > vmlinux.h

Does this hardcode a dependency on the host's running kernel instead of the
target kernel being compiled?

This would break cross-compilation and fail the build entirely on host
machines without CONFIG_DEBUG_INFO_BTF enabled in their running kernel.
Should it use the in-tree $(VMLINUX_BTF) provided by the build infrastructure?

[ ... ]

> +sig.bin: insn.bin map.bin $(OUTPUT)/gen_sig
> +	$(OUTPUT)/gen_sig --key $(CERTDIR)/signing_key.pem --cert $(CERTDIR)/signing_key.x509 \
> +		--data insn.bin --add map.bin:0 --out sig.bin

Will this unconditionally fail if the kernel configuration lacks
CONFIG_MODULE_SIG?

The Makefile hardcodes the use of signing_key.pem and signing_key.x509, which
might not exist. Should the test gracefully detect the absence of the required
keys and skip the build or execution instead of failing the entire pipeline?

> +
> +signed_loader.h: sig.bin
> +	$(SCRIPTSDIR)/write-sig.sh loader.h sig.bin > $@
> +
> +loader: loader.c signed_loader.h
> +	$(CC) $(CFLAGS) -I$(LIBDIR) -I$(APIDIR) $< -o $@ -lbpf

Could this custom rule be bypassed because it lacks the $(OUTPUT)/ prefix?

The include ../lib.mk transforms TEST_GEN_PROGS by prefixing it with
$(OUTPUT)/. When make attempts to build $(OUTPUT)/loader, it might fall back
to its default pattern rule.

This would omit the -I include paths, the -lbpf linker flag, and the
dependency on signed_loader.h, leading to compilation failures.

Also, similar to the LDLIBS addition above, this uses -lbpf without specifying
the in-tree path.

> diff --git a/tools/testing/selftests/hornet/loader.c b/tools/testing/selftests/hornet/loader.c
> new file mode 100644
> index 0000000000000..f27580c7262b3
> --- /dev/null
> +++ b/tools/testing/selftests/hornet/loader.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <stddef.h>
> +#include <sys/resource.h>
> +#include <bpf/libbpf.h>
> +#include <errno.h>
> +#include  "signed_loader.h"
> +
> +int main(int argc, char **argv)
> +{
> +	struct trivial *skel;
> +
> +	skel = trivial__open_and_load();

The commit message states this "exercises hornet's map validation", but this
solely loads a valid, signed skeleton.

Are there any negative tests to verify that the LSM actually blocks invalid
loads? If the Hornet LSM fails open or is disabled, this test might falsely
pass.

> +	if (!skel)
> +		return -1;

Should this use the modern ASSERT_*() macros like ASSERT_OK_PTR() instead of
raw conditionals?

Using raw conditionals silently bypasses the standard testing reporting
infrastructure.

> +
> +	trivial__destroy(skel);
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/hornet/trivial.bpf.c b/tools/testing/selftests/hornet/trivial.bpf.c
> new file mode 100644
> index 0000000000000..d38c5b53ff932
> --- /dev/null
> +++ b/tools/testing/selftests/hornet/trivial.bpf.c

[ ... ]

> +SEC("tracepoint/syscalls/sys_enter_unlinkat")
> +int handle_enter_unlink(struct trace_event_raw_sys_enter *ctx)
> +{
> +	char filename[128] = { 0 };
> +	struct task_struct *task;
> +	unsigned long start_time = 0;
> +	int pid = bpf_get_current_pid_tgid() >> 32;
> +	char *pathname_ptr = (char *) BPF_CORE_READ(ctx, args[1]);
> +
> +	bpf_probe_read_str(filename, sizeof(filename), pathname_ptr);

Can this silently fail on architectures with strict user/kernel address space
separation (like x86 with SMAP enabled)?

The sys_enter_unlinkat tracepoint receives pathname from user space in
args[1], but bpf_probe_read_str() is deprecated and meant for kernel-space
pointers. Should this use bpf_probe_read_user_str() instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420212653.438685-1-bboscaccy@linux.microsoft.com?part=9