Re: [PATCH bpf-next 04/11] bpftool: Support ML-DSA program signing

> bpftool: Support ML-DSA program signing
>
> Add bpftool support for ML-DSA program signing and drop the flag for
> ML-DSA keys on affected OpenSSL versions, the same way as commit
> 0ad9a71933e7 ("modsign: Enable ML-DSA module signing").

This isn't a bug, but the changelog describes the change as dropping
CMS_NOATTR "the same way as commit 0ad9a71933e7", though the diff does two
further things a reviewer cannot infer from it.

First, MAX_SIG_SIZE grows 4x (4096 to 16384):

> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -57,7 +57,7 @@ #define REQ_ARGS(cnt)							\
>  	})
>
>  #define ERR_MAX_LEN	1024
> -#define MAX_SIG_SIZE	4096
> +#define MAX_SIG_SIZE	16384

which resizes the on-stack sig_buf[] in both gen_trace() and try_loader().
The number is also unrelated to the BPF_PROG_MAX_SIGNATURE_SIZE of 64 KiB
introduced two patches earlier in the same series, so the choice of 16384 is
unexplained.

Second, the ML-DSA branch also pins the CMS digest to SHA-512:

> diff --git a/tools/bpf/bpftool/sign.c b/tools/bpf/bpftool/sign.c
> --- a/tools/bpf/bpftool/sign.c
> +++ b/tools/bpf/bpftool/sign.c
> @@ -130,6 +130,9 @@ __u32 register_session_key(const char *key_der_path)
>
>  int bpftool_prog_sign(struct bpf_load_and_run_opts *opts)
>  {
> +	unsigned int signer_flags = CMS_NOCERTS | CMS_BINARY | CMS_NOSMIMECAP |
> +				    CMS_USE_KEYID | CMS_NOATTR;
> +	const EVP_MD *cms_digest = EVP_sha256();

[ ... ]

> @@ -167,6 +170,21 @@ int bpftool_prog_sign(struct bpf_load_and_run_opts *opts)
>  		goto cleanup;
>  	}
>
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L && OPENSSL_VERSION_NUMBER < 0x40000000L
> +	if (EVP_PKEY_is_a(private_key, "ML-DSA-44") ||
> +	    EVP_PKEY_is_a(private_key, "ML-DSA-65") ||
> +	    EVP_PKEY_is_a(private_key, "ML-DSA-87")) {
> +		/*
> +		 * ML-DSA + CMS_NOATTR is not supported in openssl-3.5 and
> +		 * before, so fall back to signedAttrs. ML-DSA does its own
> +		 * hashing and ignores the CMS digest, except when signedAttrs
> +		 * are used, where only SHA-512 is permitted.
> +		 */
> +		signer_flags &= ~CMS_NOATTR;
> +		cms_digest = EVP_sha512();
> +	}
> +#endif

which the referenced modsign commit does not do. The referenced
scripts/sign-file.c keeps taking its digest from the -d argument and only
clears CMS_NOATTR. The reasoning for the SHA-512 pin is in the code comment
but not in the changelog.

Would it be worth a sentence on why MAX_SIG_SIZE lands on 16384 (rather
than matching the 64 KiB kernel bound added earlier in the series), and
noting that unlike sign-file.c this also pins the CMS digest to SHA-512?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32530917987