Re: [PATCH bpf-next v3 5/6] selftests/bpf: Verify load-time signed loader metadata

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] In `signed_btf_fd_array_rejected()`, if `gen_loader_fixture_init(&f)` fails partway (e.g., `bpf_object__gen_loader()` fails), it returns non-zero and the code jumps to the `out` label. Because `have_fixture` is set to `true` only *after* the function call, the cleanup block `if (have_fixture) gen_loader_fixture_fini(&f);` is bypassed. This causes a memory leak of resources like `f.skel` which were partially allocated by `gen_loader_fixture_init()`. Setting `have_fixture = true` before the call (as correctly done in `lsm_signature_verdict()`) would prevent this leak, since `gen_loader_fixture_init()` zeroes the struct before allocation.
--

commit 029153a8d1d1ffbf9c42e3430f45b09e6533827c
Author: Daniel Borkmann <daniel@iogearbox.net>
Subject: selftests/bpf: Verify load-time signed loader metadata

This commit updates the signed gen_loader selftests to verify load-time
metadata enforcement. The kernel now folds the loader's frozen exclusive
fd_array maps into the signature at load time instead of checking it from
within BPF. The patch adds multiple test cases to ensure the signature
logic and bounds are strictly enforced.

> diff --git a/tools/testing/selftests/bpf/prog_tests/signed_loader.c b/tools/testing/selftests/bpf/prog_tests/signed_loader.c
> index 5fc417e31fc61..b3ac3d9b0e316 100644
> --- a/tools/testing/selftests/bpf/prog_tests/signed_loader.c
> +++ b/tools/testing/selftests/bpf/prog_tests/signed_loader.c
[ ... ]
> +static void signed_btf_fd_array_rejected(void)
> +{
> +	char dir_tmpl[] = "/tmp/signed_loader_btfXXXXXX", *dir = NULL;
> +	__u32 sig_sz = 8192;
> +	int map_fd = -1, prog_fd = -1;
> +	unsigned char *buf = NULL;
> +	struct gen_loader_fixture f;
> +	bool have_fixture = false;
> +	struct btf *btf = NULL;
> +	union bpf_attr attr;
> +	int fds[2];
> +	__u8 sig[8192];
> +
> +	syscall(__NR_request_key, "keyring", "_uid.0", NULL,
> +		KEY_SPEC_SESSION_KEYRING);
> +	dir = mkdtemp(dir_tmpl);
> +	if (!ASSERT_OK_PTR(dir, "mkdtemp"))
> +		return;
> +	if (!ASSERT_OK(run_setup("setup", dir), "verify_sig_setup")) {
> +		rmdir(dir);
> +		return;
> +	}
> +	if (gen_loader_fixture_init(&f) != 0)
> +		goto out;
> +	have_fixture = true;

[Severity: Low]
Does this leak partially allocated fixture resources on failure?

If gen_loader_fixture_init(&f) fails partway through its setup, it returns
non-zero and the code jumps to the out label. 

Because have_fixture is set to true only after the function call succeeds,
the cleanup block at the out label:

    if (have_fixture)
        gen_loader_fixture_fini(&f);

is bypassed. This skips the cleanup of resources like f.skel which may have
been allocated prior to the failure.

Should have_fixture be set to true before calling gen_loader_fixture_init()
so that any partially allocated resources are safely torn down?

[ ... ]
> +out:
> +	if (btf)
> +		btf__free(btf);
> +	if (map_fd >= 0)
> +		close(map_fd);
> +	if (have_fixture)
> +		gen_loader_fixture_fini(&f);
> +	if (dir)
> +		run_setup("cleanup", dir);
> +	free(buf);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702143605.252797-1-daniel@iogearbox.net?part=5