Re: [PATCH bpf-next 08/11] selftests/bpf: Add an end-to-end ML-DSA signed loader test

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Temporary directory and generated key files are leaked if `verify_sig_setup.sh setup-mldsa` fails.
--

--- Patch [8]: [PATCH bpf-next 08/11] selftests/bpf: Add an end-to-end ML-DSA signed loader test ---
commit 3cc419c08f9d1d4d5a77de9eb5cccbc21b94232e
Author: Daniel Borkmann <[email protected]>

selftests/bpf: Add an end-to-end ML-DSA signed loader test

This commit adds an end-to-end ML-DSA signed loader test to the BPF CI. It
introduces a new test case that verifies an ML-DSA signed program can be
loaded successfully using the existing pkcs7 signature verification.

> diff --git a/tools/testing/selftests/bpf/prog_tests/signed_loader.c b/tools/testing/selftests/bpf/prog_tests/signed_loader.c
> index 4b2416903d90d..a1fa1c37815bf 100644
> --- a/tools/testing/selftests/bpf/prog_tests/signed_loader.c
> +++ b/tools/testing/selftests/bpf/prog_tests/signed_loader.c
[ ... ]
> @@ -1550,6 +1563,87 @@ static void loadtime_with_map(void)
>  	test_signed_loader_map__destroy(skel);
>  }
>  
> +/*
> + * End-to-end signed load with a post-quantum key. ML-DSA (FIPS-204) is wired
> + * through the X.509 and PKCS#7 parsers, and BPF reaches them via
> + * verify_pkcs7_signature() without knowing the algorithm, so an ML-DSA key in
> + * the keyring should verify an ML-DSA signed program with no BPF-side work.
> + */
> +static void mldsa_signed_load(void)
> +{
> +	char dir_tmpl[] = "/tmp/bpfmldsaXXXXXX";
> +	int map_fd = -1, prog_fd = -1, err;
> +	__u8 *sig = NULL, *buf = NULL;
> +	struct gen_loader_fixture f;
> +	bool have_fixture = false;
> +	__u32 sig_sz = 16384;
> +	char *dir;
> +
> +	dir = mkdtemp(dir_tmpl);
> +	if (!ASSERT_OK_PTR(dir, "mkdtemp"))
> +		return;
> +
> +	err = run_setup("setup-mldsa", dir);
> +	if (err == SETUP_SKIP) {
> +		printf("%s:SKIP:openssl has no ML-DSA support (needs 3.5+)\n",
> +		       __func__);
> +		test__skip();
> +		rmdir(dir);
> +		return;
> +	}
> +	if (!ASSERT_OK(err, "verify_sig_setup setup-mldsa")) {
> +		rmdir(dir);

[Severity: Low]
If run_setup("setup-mldsa", dir) fails after creating files in the temporary
directory (for example, if keyctl padd fails in the bash script), does this
code leak the temporary directory and its contents?

Since rmdir() does not recursively remove files, it will fail with ENOTEMPTY
if there are still generated files like signing_key.der inside the directory.

Would it be safer to call run_setup("cleanup", dir) here instead of rmdir()
to ensure all temporary files are properly cleaned up?

> +		return;
> +	}

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