[dgarske]

Description

examples/bench/bench hardcoded TPM_MLDSA_65 and TPM_MLKEM_768, one ECC curve and one RSA key size, so a part implementing the other parameter sets, curves or moduli was never exercised by the benchmark.

bench_pqc_mldsa and bench_pqc_mlkem now take the parameter set, and the caller walks ML-DSA 44, 65 and 87 and ML-KEM 512, 768 and 1024. The ECC section walks the curves the host build can also verify with, guarded as the ECC tests in tests/unit_tests.c are. The RSA section walks the sizes MAX_RSA_KEY_BITS allows, keeping the configured WOLFTPM2_WRAP_RSA_KEY_BITS as a candidate when it is smaller; wolfTPM2_GetKeyTemplate_RSA takes no size argument, so the loop overrides parameters.rsaDetail.keyBits after the template call. The reported strength is the parameter set or curve name rather than the enum value or the field width, and is taken from the curve the template ends up carrying, so the output reads as before for what was already covered.

Walking a dimension means the part may not implement a given entry, and key creation is where that is first reported - with codes a completed operation never produces: TPM_RC_PARMS for an excluded ML-DSA or ML-KEM parameter set, TPM_RC_KEY_SIZE for a modulus the TPM does not offer, TPM_RC_CURVE for a curve it lacks. bench_keygen_unsupported classifies those and bench_keygen_done records the row as skipped, so an absent entry no longer ends the benchmark before the later rows run. ECDH key creation takes the same path, since a part can offer a curve for ECDSA but not for ECDH.

Testing

Against the fwTPM simulator with --enable-fwtpm --enable-v185, every ML-DSA and ML-KEM row runs, and with HAVE_ALL_CURVES the P-256, P-384 and P-521 rows all run.

A new fwtpm-v185-mldsa-trim CI row builds the server with -DWOLFSSL_NO_ML_DSA_44, so one parameter set answers TPM_RC_PARMS. The benchmark has to record it as unsupported and carry on to the rest; run_examples.sh fails on a non-zero bench exit, which is what catches a regression.

Builds clean with --enable-fwtpm, --enable-swtpm --disable-fwtpm, --enable-fwtpm --enable-v185 and --disable-wrapper, which covers the case where neither WOLFTPM_MLDSA nor WOLFTPM_MLKEM is defined and the new loop variables compile out.

A 4096-bit RSA run has not been performed, since that needs a library whose structures are sized for it.

[embhorn]

Skoll Multi-Scan Review

Modes: review + review-security

Overall recommendation: REQUEST_CHANGES
Findings: 7 total — 7 posted, 0 skipped
7 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] [review+review-security] Unsupported ML parameter sets abort the remaining benchmarks — examples/bench/bench.c:309
• [High] [review+review-security] Unsupported RSA sizes stop the size loop — examples/bench/bench.c:675
• [High] [review+review-security] An unsupported optional ECC curve aborts the benchmark — examples/bench/bench.c:774
• [Medium] [review+review-security] The RSA list can exceed a configured maximum and drops 1024-bit builds — examples/bench/bench.c:471
• [Medium] [review] A build without P-256 benchmarks P-384 twice — examples/bench/bench.c:483
• [Medium] [review] HAVE_ALL_CURVES builds omit P-384 — examples/bench/bench.c:484
• [Medium] [review] New skip paths lack regression coverage — examples/bench/bench.c:838

Review generated by Skoll

[Copilot]

🟡 Changes recommended

The CI job runs tests that require the disabled ML-DSA-44 parameter set, causing failure, and the benchmark’s verification-skip path leaks a transient TPM sequence slot.

2 open issues

• Disabled ML-DSA set causes the full CI test suite to fail · New
• Verification failure leaks the transient sequence slot · New

What changed in this PR

Expands wolfTPM’s benchmark to exercise more supported cryptographic configurations rather than fixed parameter sets.

Changes:

• Iterates over all ML-DSA and ML-KEM parameter sets, build-supported RSA sizes, and enabled ECC curves.
• Reports accurate strength labels and skips unsupported key-generation configurations.
• Adds a firmware TPM CI configuration with ML-DSA-44 disabled to exercise skip handling.

File	Description
examples/​bench/​bench.c	Adds parameter-set, key-size, and curve iteration with unsupported-configuration handling.
.github/​workflows/​fwtpm-test.yml	Adds a reduced-ML-DSA configuration intended to test benchmark skipping.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The benchmark can select ECC curves exceeding its coordinate buffers and leak verification sequences on oversized-signature rejection, corrupting public data and exhausting TPM slots.

2 open issues

• Flush the verification sequence when oversized signatures are skipped · New
• Gate P-384 and P-521 on coordinate buffer capacity · New

2 resolved since last review

• Disabled ML-DSA set causes the full CI test suite to fail
• Verification failure leaks the transient sequence slot

_{Review effort: Balanced}

[embhorn]

Skoll Multi-Scan Review

Modes: review + review-security

Overall recommendation: REQUEST_CHANGES
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] [review] Additional ECC curves use a different hash for verification — examples/bench/bench.c:820
• [High] [review+review-security] ECC curve array can be empty in a supported build configuration — examples/bench/bench.c:537
• [Medium] [review-security] A failed later keygen can leave the benchmark using an empty key — examples/bench/bench.c:735
• [Medium] [review] CI covers the PQC skip path but not the new RSA and ECC dimensions — .github/workflows/fwtpm-test.yml:396

Review generated by Skoll

[embhorn]

Additional ECC curves use a different hash for verification · bug

The new P-384 and P-521 rows create ECDSA templates with the wrapper's default SHA-256 scheme. wolfTPM2_SignHash uses that scheme, but the existing wolfTPM2_VerifyHash call selects SHA-384 or SHA-512 from the curve. The rows therefore verify a signature under a different declared hash algorithm, and TPMs that enforce the key's scheme can reject them.

Suggested fix: Use wolfTPM2_VerifyHash_ex with the template's signature hash for these rows, and verify the same digest size used for signing.

[embhorn]

ECC curve array can be empty in a supported build configuration · bug

If NO_ECC256 is defined without HAVE_ECC384, HAVE_ECC521, or HAVE_ALL_CURVES, every initializer in eccCurves[] is removed by the preprocessor. The empty initializer list is invalid in ISO C and fails builds with -Wpedantic -Werror. The benchmark had no conditional array before this PR, so this is a regression. The review mode rated it High/BLOCK and reproduced the error with a local C11 compiler check. The review-security mode rated it Low.

Suggested fix: Guard the array and its loop so they compile only when at least one listed curve is enabled, or provide a nonempty representation with an explicit count.

[embhorn]

A failed later keygen can leave the benchmark using an empty key · Logic

Before the PR, any RSA or ECC key creation failure exited immediately. Now an unsupported error is converted to success, but count stays positive if an earlier iteration succeeded. The earlier key has already been unloaded, and wolfTPM2_CreateAndLoadKey clears its output key on the failed attempt. The count == 0 check then fails to skip RSA encryption or ECC signing with an empty handle. This can abort the benchmark instead of continuing to later entries. The ECC loop has the same problem.

Suggested fix: In both the RSA and ECC loops, record whether the most recent key creation failed. Skip that entry's remaining operations whenever it did, even when earlier iterations succeeded.

[embhorn]

CI covers the PQC skip path but not the new RSA and ECC dimensions · test

The new runtime step checks that ML-DSA-44 is skipped and ML-DSA-65 runs. It does not configure or assert any P-384, P-521, or RSA size above 2048 row, so the new ECC and RSA loops have no automated runtime coverage.

Suggested fix: Add a -iter=1 CI configuration with additional curves and RSA-3072 support, then assert that those rows run.

[embhorn]

Skoll Multi-Scan Review

Modes: review + review-security

Overall recommendation: REQUEST_CHANGES
Findings: 4 total — 1 posted, 3 skipped
1 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] [review+review-security] Bound ECC curve list by the TPM field size (MAX_ECC_KEY_BYTES) — examples/bench/bench.c:540-546

Skipped findings

• [SUGGEST] Keep the RSA unique field sized for each benchmark row
• [Low] P-384 is benchmarked twice when P-256 is disabled
• [SUGGEST] Exercise the other new unsupported-key skip paths in CI

Review generated by Skoll

[embhorn]

Bound ECC curve list by the TPM field size (MAX_ECC_KEY_BYTES) · Logic

The new curve list checks wolfCrypt support (HAVE_ECC384, HAVE_ECC521, HAVE_ALL_CURVES) but not MAX_ECC_KEY_BYTES. For example, WOLFTPM_SLB9672 sets 48-byte TPM ECC fields, yet a host build with HAVE_ALL_CURVES includes P-521. Its template requests 66-byte coordinates, but packet marshaling and response parsing clamp them to 48 bytes. The P-521 row therefore cannot represent the requested key correctly: it may report a false skip or fail, or, if the TPM accepts the curve, truncated public coordinates and signatures make verification fail. The fwTPM implementation already guards P-521 with MAX_ECC_KEY_BYTES >= 66. Severity views: the review mode rated this High/BLOCK, and the security mode rated it Medium. The stricter action (BLOCK) is kept.

Suggested fix: Require MAX_ECC_KEY_BYTES >= 48 for P-384 and >= 66 for P-521 in the new curve list. Cover a build where wolfSSL enables P-521 while MAX_ECC_KEY_BITS is 384.