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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Cross-compilation breakage due to `gen_sig` built with target compiler but executed on the host.
- [High] Cross-compilation breakage due to hardcoded host paths for `vmlinux.h` and includes.
- [High] Incorrect helper used to read user-space memory in the BPF program.
- [High] Makefile custom targets lack `$(OUTPUT)/` prefix, breaking out-of-tree builds.
- [High] Unconditional dependency on kernel signing keys breaks the global selftests build.
- [Medium] Selftest links against the system's `libbpf` instead of the locally built in-tree version.
- [Medium] Violation of the BPF selftest macro guidelines.
--

commit 7071c10449f31b01df208a3d3f4cf023f39c5b06
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/Makefile b/tools/testing/selftests/Makefile
> index 450f13ba4cca9..4e2d1cd88c825 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
[ ... ]
> @@ -44,6 +44,7 @@ TARGETS += ftrace
>  TARGETS += futex
>  TARGETS += gpio
>  TARGETS += hid
> +TARGETS += hornet

Will this break the global selftests build if module signing keys haven't
been generated? The hornet Makefile seems to unconditionally depend on
$(CERTDIR)/signing_key.pem, which might not exist in all build environments.

>  TARGETS += intel_pstate
>  TARGETS += iommu
>  TARGETS += ipc

> 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
[ ... ]
> +BPF_CFLAGS := -target bpf \
> +	-D__TARGET_ARCH_$(ARCH) \
> +	-I/usr/include/$(shell uname -m)-linux-gnu \
> +	$(KHDR_INCLUDES)

Is it intentional to use $(shell uname -m) here?

When cross-compiling, this will yield the host architecture instead of the
target architecture, which might include the wrong host structures.

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

Does this read the host kernel's BTF instead of the target kernel's BTF?
If cross-compiling, /sys/kernel/btf/vmlinux is the host machine's BTF.

Also, are these targets missing the $(OUTPUT)/ prefix? The Makefile includes
../lib.mk which automatically prepends $(OUTPUT)/ to TEST_GEN_FILES and
TEST_GEN_PROGS.

When building out-of-tree with O=..., make might fail to find the rules for
these targets without the $(OUTPUT)/ prefix.

> +
> +trivial.bpf.o: trivial.bpf.c vmlinux.h
> +	$(CLANG) $(CFLAGS) $(BPF_CFLAGS) -c $< -o $@
> +
> +loader.h: trivial.bpf.o
> +	$(BPFTOOL) gen skeleton -S -k $(CERTDIR)/signing_key.pem -i $(CERTDIR)/signing_key.x509 \
> +		-L $< name trivial > $@
[ ... ]
> +$(OUTPUT)/gen_sig: ../../../../scripts/hornet/gen_sig.c
> +	$(call msg,GEN_SIG,,$@)
> +	$(Q)$(CC) $(shell $(PKG_CONFIG) --cflags libcrypto 2> /dev/null) \
> +		  $< -o $@ \
> +		  $(shell $(PKG_CONFIG) --libs libcrypto 2> /dev/null || echo -lcrypto)
> +
> +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

Since $(OUTPUT)/gen_sig is executed during the build process to generate
sig.bin, shouldn't it be built with $(HOSTCC) instead of the target $(CC)?

During cross-compilation, building it with the target compiler and running
it on the host could result in an 'Exec format error'.

> +
> +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

Does this link against the system's dynamic libbpf.so instead of the locally
built in-tree version?

Typically, kernel selftests statically link against the in-tree libbpf.a to
test the current kernel's library and avoid build failures on systems missing
the package.

> +
> +
> +EXTRA_CLEAN = $(OUTPUT)/gen_sig
> 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
[ ... ]
> +int main(int argc, char **argv)
> +{
> +	struct trivial *skel;
> +
> +	skel = trivial__open_and_load();
> +	if (!skel)
> +		return -1;

Should this use the BPF testing framework and modern ASSERT_*() macros like
ASSERT_OK_PTR() to provide structured TAP output?

New BPF tests are expected to follow this standard.

> +
> +	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);

Because args[1] of sys_enter_unlinkat is a user-space pointer to the pathname,
does this need to use bpf_probe_read_user_str() instead of
bpf_probe_read_str()?

On architectures with strict boundary protections or split address spaces,
reading user-space memory with bpf_probe_read_str() could fail or read
garbage.

> +	task = (struct task_struct *)bpf_get_current_task();
> +	start_time = BPF_CORE_READ(task, start_time);

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