Skip to content

Add benchmark coverage for every parameter set, ECC curve and RSA key size the build supports - #662

Open
dgarske wants to merge 2 commits into
wolfSSL:masterfrom
dgarske:bench_param_sets
Open

dgarske wants to merge 2 commits into
wolfSSL:masterfrom
dgarske:bench_param_sets

Conversation

@dgarske

@dgarske dgarske commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

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.

@dgarske dgarske self-assigned this Oct 30, 2026
@dgarske dgarske changed the title Add benchmark support for every ML-DSA and ML-KEM parameter set Add benchmark coverage for every parameter set, ECC curve and RSA key size the build supports Oct 31, 2026
@dgarske dgarske assigned wolfSSL-Bot and embhorn and unassigned dgarske Nov 2, 2026
@dgarske
dgarske requested a review from embhorn November 2, 2026 18:48

@embhorn embhorn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread examples/bench/bench.c Resolved
Comment thread examples/bench/bench.c Resolved
Comment thread examples/bench/bench.c Resolved
Comment thread examples/bench/bench.c Resolved
Comment thread examples/bench/bench.c Resolved
Comment thread examples/bench/bench.c Resolved
Comment thread examples/bench/bench.c Resolved
Copilot AI balanced review requested due to automatic review settings November 2, 2026 21:30
@dgarske
dgarske requested a review from embhorn November 2, 2026 21:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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
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

Comment thread .github/workflows/fwtpm-test.yml Resolved
Comment thread examples/bench/bench.c Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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
2 resolved since last review

Review effort: Balanced

Comment thread examples/bench/bench.c
if (rc != 0) break;
} while (bench_stats_check(start, &count, maxDuration));
rc = bench_asym_done("ML-DSA", 65, "verify", count, start, rc);
rc = bench_asym_done("ML-DSA", strength, "verify", count, start, rc);
Comment thread examples/bench/bench.c
Comment on lines +541 to +544
#if (defined(HAVE_ECC384) || defined(HAVE_ALL_CURVES)) && ECC_MIN_KEY_SZ <= 384
TPM_ECC_NIST_P384,
#endif
#if (defined(HAVE_ECC521) || defined(HAVE_ALL_CURVES)) && ECC_MIN_KEY_SZ <= 521
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants