Add ML-DSA 65/87 Sign/Verify/KeyGen benchmarks

[miyazakh]

Add ML-DSA 65/87 Sign/Verify/KeyGen benchmarks

Summary

• Implement the ML-DSA 65 and ML-DSA 87 Sign/Verify/KeyGen benchmark entry points in wh_bench_mod_mldsa.c, which previously returned WH_ERROR_NOTIMPL stubs.
• Extend the shared _benchMlDsaSign/_benchMlDsaVerify/_benchMlDsaKeyGen helpers to select the correct test key/message/signature data based on the requested paramSet (WC_ML_DSA_44 / WC_ML_DSA_65 / WC_ML_DSA_87).
• Add per-level WOLFSSL_NO_ML_DSA_44/65/87 feature gates so any level can be individually disabled and its test data compiled out.
• Add ML-DSA 65/87 test key, message, and signature vectors.
• Update benchmark config (user_settings.h, wolfhsm_cfg.h) to support the new levels.

Changes

File	Description
benchmark/bench_modules/wh_bench_mod_mldsa.c	Fill in wh_Bench_Mod_MlDsa65{Sign,Verify,KeyGen}[Dma] and wh_Bench_Mod_MlDsa87{Sign,Verify,KeyGen}[Dma], which previously just returned WH_ERROR_NOTIMPL; add ML-DSA 65/87 key/message/signature test vectors; guard each level's data and helper cases with WOLFSSL_NO_ML_DSA_44/65/87
benchmark/config/user_settings.h	Add (disabled by default) WOLFSSL_NO_ML_DSA_44/65/87 toggles so individual ML-DSA levels can be excluded from the build
benchmark/config/wolfhsm_cfg.h	Pull in user_settings.h early so the ML-DSA feature gates are visible when sizing buffers; size WOLFHSM_CFG_COMM_DATA_LEN to the largest enabled ML-DSA level (1280*8 for 87, 1280*5 for 65, 1280*4 otherwise)

Details

Benchmark entry points (wh_bench_mod_mldsa.c)

Previously, wh_Bench_Mod_MlDsa65Sign/Verify/KeyGen and the corresponding ML-DSA 87 and DMA variants were stubs that unconditionally returned WH_ERROR_NOTIMPL. They now call the shared _benchMlDsaSign, _benchMlDsaVerify, and _benchMlDsaKeyGen helpers with WC_ML_DSA_65 / WC_ML_DSA_87 as the parameter set, matching the existing ML-DSA 44 pattern.

The shared helpers were extended with a switch (paramSet) to select the correct static test key (and, for verify, message/signature) buffer for the requested level, each guarded by the corresponding WOLFSSL_NO_ML_DSA_xx macro so disabled levels don't pull in their test data.

Config changes

• WOLFHSM_CFG_COMM_DATA_LEN must be large enough to hold the biggest enabled ML-DSA level's key/signature payload for non-DMA client/server messages, so it now scales with which levels are enabled.
• wolfhsm_cfg.h needs to see the WOLFSSL_NO_ML_DSA_xx gates from user_settings.h before computing that buffer size, so user_settings.h is now included early (it's guarded, so the later include from wh_settings.h remains a no-op).

Impact

• ML-DSA 65 and 87 benchmarks are now functional instead of returning "not implemented".
• Any ML-DSA level can be disabled individually via WOLFSSL_NO_ML_DSA_44/65/87, shrinking both the compiled test data and the comm buffer accordingly.

[Copilot]

Pull request overview

Adds functional ML-DSA level 65/87 benchmark entry points (sign/verify/keygen, including DMA variants) and wires configuration so individual ML-DSA levels can be compiled out and benchmark comm buffers scale to the largest enabled level.

Changes:

• Implement ML-DSA 65/87 benchmark entry points by delegating to the shared ML-DSA helper routines with the requested parameter set.
• Extend ML-DSA helpers and vector compilation to support per-level feature gates (WOLFSSL_NO_ML_DSA_44/65/87) and per-level test vectors.
• Update benchmark configuration headers to expose per-level toggles and size WOLFHSM_CFG_COMM_DATA_LEN based on enabled ML-DSA level(s).

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File	Description
benchmark/bench_modules/wh_bench_mod_mldsa.c	Adds ML-DSA 65/87 benchmark entry points and extends helper logic/vectors to be parameter-set aware and feature-gated.
benchmark/config/user_settings.h	Adds per-ML-DSA-level compile-time toggles (disabled by default).
benchmark/config/wolfhsm_cfg.h	Includes user_settings.h early (when enabled) and sizes WOLFHSM_CFG_COMM_DATA_LEN to the largest enabled ML-DSA level.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 7 total — 5 posted, 2 skipped

Posted findings

• [High] ML-DSA-87 KeyGen hangs on the shm transport: the ~7550-byte response exceeds the hard-coded 7000-byte SHM buffers (reproduced) — benchmark/bench_modules/wh_bench_mod_mldsa.c:3120-3134
• [Medium] Benchmark never checks the verified output of wc_MlDsaKey_VerifyCtx, so a bad vector would silently benchmark the reject path — benchmark/bench_modules/wh_bench_mod_mldsa.c:2728-2753
• [Medium] The early user_settings.h include in wolfhsm_cfg.h bypasses wh_settings.h's NO_CRYPTO gate, and is inert in WOLFSSL_LIB=1 builds — benchmark/config/wolfhsm_cfg.h:35-43
• [Low] New ML-DSA-65/87 pubkey and signature arrays lack static, adding four external symbols with generic names — benchmark/bench_modules/wh_bench_mod_mldsa.c:1333,1499,1782,2001
• [Low] Ungrammatical comment introducing the per-level ML-DSA toggles — benchmark/config/user_settings.h:157

Skipped findings

• [Medium] COMM_DATA_LEN ladder: undocumented multipliers, only 344 bytes of headroom on the ML-DSA-65 branch, and no WOLFSSL_HAVE_MLDSA gate
• [Low] New declarations in _benchMlDsaVerify are not clang-format aligned with the surrounding block

Review generated by Skoll via Claude/Codex

[Frauschi]

🟠 [High] ML-DSA-87 KeyGen hangs on the shm transport: the ~7550-byte response exceeds the hard-coded 7000-byte SHM buffers (reproduced)
🚫 BLOCK

This PR enables wh_Bench_Mod_MlDsa87KeyGen (previously WH_ERROR_NOTIMPL) and raises WOLFHSM_CFG_COMM_DATA_LEN to 1280*8 = 10240 so the comm buffer can hold the larger payloads. But the benchmark's own POSIX SHM transport buffers are hard-coded literals that are NOT derived from WOLFHSM_CFG_COMM_DATA_LEN, so they were not raised along with it.

Path: _benchMlDsaKeyGen -> wh_Client_MlDsaMakeExportKey -> server _HandleMlDsaKeyGen takes the WH_NVM_FLAGS_EPHEMERAL branch and serializes both keys via wh_Crypto_MlDsaSerializeKeyDer(). For ML-DSA-87 that is WC_MLDSA_87_BOTH_KEY_DER_SIZE = 7520 bytes; with whCommHeader plus the crypto response headers the frame is ~7550 B. The server bounds it only by WOLFHSM_CFG_COMM_DATA_LEN (10240), so it emits the full response. benchmark/wh_bench.c:997-998 (client) and :1072-1073 (server) then configure the SHM transport with literal .req_size = 7000, .resp_size = 7000, and wh_TransportMem_SendResponse (src/wh_transport_mem.c:219-220) rejects any len above resp_size - sizeof(whTransportMemCsr) (~6992).

I built the PR head and ran it. ML-DSA-87-KEY-GEN on the shm transport does not merely fail with WH_ERROR_BADARGS as the code path suggests - it produces no output at all and never terminates (killed at a 90 s timeout, exit 124). The rejected response is never delivered, so the client waits forever.

Measured on the PR head, all 10 iterations each:

Benchmark	mem	shm	tcp
ML-DSA-87-SIGN	ok	ok	ok
ML-DSA-87-VERIFY	ok	ok	ok
ML-DSA-87-KEY-GEN	ok	HANGS	ok
ML-DSA-65-KEY-GEN	ok	ok	ok

Only ML-DSA-87 KeyGen is affected: ML-DSA-65 KeyGen fits (WC_MLDSA_65_BOTH_KEY_DER_SIZE = 6016), and 87 Sign/Verify stay well under the limit. The mem transport is fine because its BUFFER_SIZE (benchmark/wh_bench.c:59-61) IS derived from WOLFHSM_CFG_COMM_DATA_LEN. The WH_BENCH_TRANSPORT_POSIX_DMA case falls through into the SHM case, so it inherits the same 7000 and the same defect.

The PR correctly identified this class of problem for the comm buffer but missed the transport-layer literals.

Recommendation: Derive the SHM transport sizes from the comm buffer rather than hard-coding them, in both _configureClientTransport and _configureServerTransport:

.req_size  = (uint16_t)(sizeof(whCommHeader) + WOLFHSM_CFG_COMM_DATA_LEN),
.resp_size = (uint16_t)(sizeof(whCommHeader) + WOLFHSM_CFG_COMM_DATA_LEN),

I applied exactly this change on top of the PR head and ML-DSA-87-KEY-GEN --type shm then completes normally (10 iterations, ~13k ops/s), so it is a confirmed fix. Adding a WH_UTILS_STATIC_ASSERT tying the transport sizes to WOLFHSM_CFG_COMM_DATA_LEN would turn any future comm-buffer bump into a build failure instead of a runtime hang.

[Frauschi]

🟡 [Medium] Benchmark never checks the verified output of wc_MlDsaKey_VerifyCtx, so a bad vector would silently benchmark the reject path
💡 SUGGEST bug

_benchMlDsaVerify declares int verified = 0; (line 2628), passes &verified to wc_MlDsaKey_VerifyCtx(), and never reads it. A signature mismatch is not an error return from that API: it returns 0 and sets *res = 0. So if a public key / signature pair did not actually verify against test_msg, every iteration would report success, the benchmark would print throughput numbers, and CI would stay green - with the numbers optimistic, since a rejected ML-DSA verify short-circuits early and is measurably cheaper than a full verification.

The missing check is pre-existing, but this PR is what makes it load-bearing: it triples the number of vector sets riding on the unchecked invariant, and the only provenance for the two new sets is the code comment "freshly generated ... self-verified before embedding", which nothing in-tree confirms.

I checked the vectors empirically rather than leaving it open. Building the PR head with the verified assertion added, all three levels pass on every iteration - ML-DSA-44, 65 and 87 each report 10 successful verifications, 0 failures. The new vectors are correct, so this is not a live bug and does not block the PR. It is worth adding anyway: the assertion costs nothing and makes the embedded data self-validating, which is the only thing that would have let a reviewer confirm the new vectors from the diff alone.

Suggestion:

Suggested change

	if (ret != 0) {
	WH_BENCH_PRINTF("Failed to wc_MlDsaKey_VerifyCtx %d\n", ret);
	break;
	}
	if (verified != 1) {
	WH_BENCH_PRINTF("ML-DSA signature failed to verify\n");
	ret = WH_ERROR_ABORTED;
	break;
	}

[Frauschi]

🟡 [Medium] The early user_settings.h include in wolfhsm_cfg.h bypasses wh_settings.h's NO_CRYPTO gate, and is inert in WOLFSSL_LIB=1 builds
💡 SUGGEST convention

The new #ifdef WOLFSSL_USER_SETTINGS / #include "user_settings.h" block is needed for the WOLFHSM_CFG_COMM_DATA_LEN ladder to see the ML-DSA gates, but it inverts the config layering (wh_settings.h:193 includes wolfhsm_cfg.h, then :204-216 includes user_settings.h) and has three consequences worth addressing together, since one guard change fixes all of them.

1. It defeats the NO_CRYPTO suppression. wh_settings.h:204 deliberately wraps its own include in #if !defined(WOLFHSM_CFG_NO_CRYPTO) && !defined(WH_PADDING_CHECK), to "suppress external dependencies (wolfSSL headers, etc.)". The new include is gated only on WOLFSSL_USER_SETTINGS, and benchmark/Makefile:36-38 defines that independently of -DWOLFHSM_CFG_NO_CRYPTO (make NOCRYPTO=1, Makefile:108-111). So in a NOCRYPTO build user_settings.h is now visible where it previously was not: WOLFSSL_HAVE_MLDSA/LMS/XMSS become defined, flipping WOLFHSM_CFG_SERVER_KEYCACHE_BIG_BUFSIZE from 1200 to 8192 and WOLFHSM_CFG_COMM_DATA_LEN from 5120 to 10240. That is roughly 21 KB of extra static keycache plus doubled comm buffers in the one build mode whose entire purpose is a minimal, crypto-free footprint, and it drags <strings.h> into every NO_CRYPTO translation unit.

2. It is inert in the WOLFSSL_LIB=1 build. benchmark/Makefile:36-38 omits -DWOLFSSL_USER_SETTINGS when linking an installed wolfSSL. There the guard is false, the include is skipped, and the ladder always lands on the first branch (10240) regardless of what the installed library supports. The direction is safe - oversized, never undersized - but the PR's stated benefit, that "smaller devices that disable the larger levels can shrink this buffer accordingly", silently does not apply. Unlike wh_settings.h, this block also has no #else #include <wolfssl/options.h> arm, so an installed library's configuration cannot be observed here at all.

3. It is order-sensitive with no diagnostic. Because user_settings.h has its own include guard, this include is the one that takes effect and wh_settings.h's later one becomes a no-op, so user_settings.h is now processed before lines 45-69 of wolfhsm_cfg.h. The one conditional in user_settings.h that could care (#ifdef WOLFHSM_CFG_DMA -> WOLFSSL_STATIC_MEMORY, lines 220-224) is safe today because WOLFHSM_CFG_DMA arrives from the Makefile. But any future #ifdef WOLFHSM_CFG_* in user_settings.h that depends on a macro defined later in wolfhsm_cfg.h will silently stop working.

Suggestion:

Suggested change

	/* wh_settings.h includes this file before user_settings.h, so the
	/* NOTE: this include is order-sensitive. It must stay ahead of the
	* WOLFHSM_CFG_COMM_DATA_LEN ladder below, and nothing that user_settings.h
	* conditionally tests may be defined later in this file.
	* In the WOLFSSL_LIB=1 build (-DWOLFSSL_USER_SETTINGS omitted) this include
	* is skipped and COMM_DATA_LEN falls back to the largest (ML-DSA-87) size. */
	#ifdef WOLFSSL_USER_SETTINGS
	#include "user_settings.h"
	#endif

[Frauschi]

🔵 [Low] New ML-DSA-65/87 pubkey and signature arrays lack static, adding four external symbols with generic names
🔧 NIT convention

Sites: benchmark/bench_modules/wh_bench_mod_mldsa.c:1333,1499,1782,2001
The four new verify vectors are declared with external linkage:

• const byte ml_dsa_65_pub_key[] (line 1333)
• const byte ml_dsa_65_sig[] (line 1499)
• const byte ml_dsa_87_pub_key[] (line 1782)
• const byte ml_dsa_87_sig[] (line 2001)

They are used only within this translation unit and have no declaration in any header, so they should be static. Note the inconsistency introduced within this very PR: the private keys added alongside them are correctly static const unsigned char bench_mldsa_65_key[] / bench_mldsa_87_key[] (lines 256, 599), while these four are not. The non-static form follows the pre-existing ml_dsa_44_pub_key / ml_dsa_44_sig, so this is arguably just propagating an existing wart — but it doubles the count of generically-named global symbols (ml_dsa_*) exported from a benchmark object file, which is exactly the kind of name that could collide with wolfSSL test-vector symbols in a combined link.

Also raised by the portability scan (benchmark/bench_modules/wh_bench_mod_mldsa.c:1333,1499,1782,2001):

The PR adds four new file-scope arrays -- ml_dsa_65_pub_key (1952 B), ml_dsa_65_sig (3309 B), ml_dsa_87_pub_key (2592 B), ml_dsa_87_sig (4627 B) -- declared const byte without static, so roughly 12 KB of read-only data gains external linkage under generic, collision-prone names. The bench_mldsa_65_key / bench_mldsa_87_key private keys added in the very same commit are correctly static const unsigned char (line 256 and following), so the inconsistency is introduced by this change.

This is latent rather than currently broken: test/wh_test_crypto.c:13143,13254 and test-refactor/client-server/wh_test_crypto_mldsa.c:720,831 already use the identical names ml_dsa_44_pub_key / ml_dsa_44_sig, but as function-local const byte arrays, and neither file is linked into the benchmark binary. If any of those are ever hoisted to file scope, or if a future test links against the benchmark modules, the duplicate external definitions become a link-time conflict. External linkage also blocks -ffunction-sections -fdata-sections (benchmark/Makefile:49) plus --gc-sections from discarding the vectors when a level is unused.

Suggestion:

Suggested change

	const byte ml_dsa_65_pub_key[] = {
	static const byte ml_dsa_65_pub_key[] = {
	static const byte ml_dsa_65_sig[] = {
	static const byte ml_dsa_87_pub_key[] = {
	static const byte ml_dsa_87_sig[] = {

[Frauschi]

🔵 [Low] Ungrammatical comment introducing the per-level ML-DSA toggles
🔧 NIT style

The comment heading the new toggle block reads "The following options can be individually disabled ML-DSA levels", which is not a grammatical sentence. Compare with the parallel comment 14 lines above, which reads correctly: "The following options can be individually controlled to customize the ML-DSA configuration". The #if 0 / #define / #endif pattern itself matches the surrounding style and is fine.

Suggestion:

Suggested change

	/* The following options can be individually disabled ML-DSA levels */
	/* The following options can be individually enabled to disable specific
	* ML-DSA security levels */