Re: [PATCH bpf-next v6 7/8] 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:
- [Medium] Missing file descriptor cleanup on unexpected test failure.
--

--- Patch [7]: [PATCH bpf-next v6 7/8] selftests/bpf: Verify load-time signed loader metadata ---
commit 64830bb6cea6aee71f09d788c841cd423f6308e6
Author: Daniel Borkmann <daniel@iogearbox.net>

selftests/bpf: Verify load-time signed loader metadata

This commit updates the signed loader tests to verify load-time signed loader
metadata end to end. It extends the tests with more cases, such as map-less
programs and asserting the LSM admission hook, while retiring the older
subtests that asserted the removed in-loader check.

> diff --git a/tools/testing/selftests/bpf/prog_tests/signed_loader.c b/tools/testing/selftests/bpf/prog_tests/signed_loader.c
> index 5fc417e31fc61..0019492cf07a8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/signed_loader.c
> +++ b/tools/testing/selftests/bpf/prog_tests/signed_loader.c

[ ... ]

> -static void metadata_sha_mismatch(void)
> +static void signature_enforced(void)
>  {
> +	static const __u8 junk[64] = { 0x30, 0x42, 0x13, 0x37, };
>  	struct gen_loader_fixture f;
> -	bool ran;
> -	int r;
> +	int fd;
>  
>  	if (gen_loader_fixture_init(&f) == 0) {
>  		/*
> -		 * blob[0] lives in the loader's fd_array scratch (first add_data in
> -		 * bpf_gen__init); a 0-map program never reads it, so flipping it
> -		 * changes only map->sha. The metadata check is the only thing that
> -		 * can notice -> isolates emit_signature_match.
> +		 * A present-but-invalid signature (the cert bytes are not a
> +		 * PKCS#7 signature) must be rejected at load: the signature
> +		 * path is honored, not ignored. (The valid path is covered by
> +		 * the signed lskels.) Pin -EBADMSG, the PKCS#7 parse failure:
> +		 * a looser fd < 0 check could also be satisfied by the sparse
> +		 * fd_array rejection (-EACCES) that the loader's map reference
> +		 * would trip even if the signature were silently ignored.
>  		 */
> -		f.blob[0] ^= 0xff;
> -		r = run_gen_loader(f.gopts.insns, f.gopts.insns_sz, f.blob,
> -				   f.data_sz, f.excl, sizeof(f.excl), NULL, 0,
> -				   true, f.ctx, f.ctx_sz, &ran);
> -		ASSERT_TRUE(ran, "loader ran");
> -		ASSERT_EQ(r, -EINVAL, "tampered blob rejected by emit_signature_match");
> +		fd = load_loader(f.gopts.insns, f.gopts.insns_sz, -1, junk,
> +				 sizeof(junk), KEY_SPEC_SESSION_KEYRING, 0);
> +		ASSERT_EQ(fd, -EBADMSG, "invalid signature rejected at load");
>  	}
>  	gen_loader_fixture_fini(&f);
>  }

[Severity: Medium]
If the kernel unexpectedly accepts the load during signature_enforced,
load_loader will return a valid file descriptor. Does this file descriptor
leak since the ASSERT_EQ macro allows execution to continue? 

Other tests introduced in this patch, such as signature_failure_logs, use
the following pattern to ensure the file descriptor is closed on test failure:

    if (fd >= 0)
        close(fd);

Would it make sense to include this cleanup here to prevent file descriptor
exhaustion during a cascading test suite failure?

[ ... ]

> @@ -495,12 +573,31 @@ static void signature_too_large(void)
>  		 * is rejected before the buffer is read.
>  		 */
>  		fd = load_loader(f.gopts.insns, f.gopts.insns_sz, -1, junk,
> -				 64 << 20, KEY_SPEC_SESSION_KEYRING);
> +				 64 << 20, KEY_SPEC_SESSION_KEYRING, 0);
>  		ASSERT_EQ(fd, -EINVAL, "oversized signature rejected");
>  	}
>  	gen_loader_fixture_fini(&f);
>  }
>  
> +static void signature_zero_size(void)
> +{
> +	static const __u8 junk[64] = {};
> +	struct gen_loader_fixture f;
> +	int fd;
> +
> +	if (gen_loader_fixture_init(&f) == 0) {
> +		/*
> +		 * A present signature with signature_size == 0 is rejected
> +		 * up front, before the keyring is resolved or the signature
> +		 * buffer is read.
> +		 */
> +		fd = load_loader(f.gopts.insns, f.gopts.insns_sz, -1, junk,
> +				 0, KEY_SPEC_SESSION_KEYRING, 0);
> +		ASSERT_EQ(fd, -EINVAL, "zero-size signature rejected");
> +	}
> +	gen_loader_fixture_fini(&f);
> +}
> +
>  static void signature_bad_keyring(void)
>  {
>  	static const __u8 junk[64] = {};
> @@ -515,7 +612,7 @@ static void signature_bad_keyring(void)
>  		 * large positive serial takes the user-keyring path and won't exist.
>  		 */
>  		fd = load_loader(f.gopts.insns, f.gopts.insns_sz, -1, junk,
> -				 sizeof(junk), INT_MAX);
> +				 sizeof(junk), INT_MAX, 0);
>  		ASSERT_EQ(fd, -EINVAL, "signature with bad keyring_id rejected");
>  	}
>  	gen_loader_fixture_fini(&f);

[Severity: Medium]
Similar to signature_enforced, if load_loader unexpectedly succeeds in the
newly added signature_zero_size test, or in the updated signature_too_large
and signature_bad_keyring tests, could this result in leaked file descriptors?

Adding the same close cleanup check here might help prevent test environment
instability if these assertions ever fail.

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