Repository navigation
Conversation
embhorn
left a comment
There was a problem hiding this comment.
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
c5c1c2a to
bba6689
Compare
There was a problem hiding this comment.
🟡 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
bba6689 to
3754ec9
Compare
There was a problem hiding this comment.
🟡 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
embhorn
left a comment
There was a problem hiding this comment.
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
a12a715 to
c9e769b
Compare
embhorn
left a comment
There was a problem hiding this comment.
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
c9e769b to
aa02dc5
Compare
embhorn
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: review
Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] Keep the RSA unique field within the selected modulus size —
examples/bench/bench.c:725 - [High] Do not classify negative wolfCrypt errors as unsupported TPM parameters —
examples/bench/bench.c:206-209 - [High] Treat buffer errors during key creation as failures —
examples/bench/bench.c:222
Review generated by Skoll
aa02dc5 to
2c5d8b6
Compare
embhorn
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: review
Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] Substituted curves produce duplicate benchmark rows —
examples/bench/bench.c:546-550 - [Medium] Unsupported RSA size and ECC curve paths lack automated coverage —
.github/workflows/fwtpm-test.yml:405-432
Review generated by Skoll
2c5d8b6 to
0d172b6
Compare


Description
examples/bench/benchhardcodedTPM_MLDSA_65andTPM_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_mldsaandbench_pqc_mlkemnow 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 intests/unit_tests.care. The RSA section walks the sizesMAX_RSA_KEY_BITSallows, keeping the configuredWOLFTPM2_WRAP_RSA_KEY_BITSas a candidate when it is smaller;wolfTPM2_GetKeyTemplate_RSAtakes no size argument, so the loop overridesparameters.rsaDetail.keyBitsafter 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_PARMSfor an excluded ML-DSA or ML-KEM parameter set,TPM_RC_KEY_SIZEorTPM_RC_VALUEfor a modulus the TPM does not offer,TPM_RC_CURVEfor a curve it lacks.bench_keygen_unsupportedclassifies those andbench_keygen_donerecords 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. A build without P-256 substitutes its default curve for the first entry, so a curve that has already run is skipped rather than reported twice.Testing
Against the fwTPM simulator with
--enable-fwtpm --enable-v185, every ML-DSA and ML-KEM row runs, and withHAVE_ALL_CURVESthe P-256, P-384 and P-521 rows all run.A new
fwtpm-v185-mldsa-trimCI row builds the server with-DWOLFSSL_NO_ML_DSA_44, so one parameter set answersTPM_RC_PARMS. The benchmark has to record it as unsupported and carry on to the rest;run_examples.shfails on a non-zero bench exit, which is what catches a regression.The
fwtpm-rsa4096CI row also rebuilds only the server with-DMAX_RSA_KEY_BITS=3072 -DMAX_ECC_KEY_BITS=256, and checks that RSA 4096 and P-384 are recorded as unsupported while the rows between them still run.Builds clean with
--enable-fwtpm,--enable-swtpm --disable-fwtpm,--enable-fwtpm --enable-v185and--disable-wrapper, which covers the case where neitherWOLFTPM_MLDSAnorWOLFTPM_MLKEMis defined and the new loop variables compile out.A library sized for 4096-bit RSA was run against ibmswtpm2: the 2048 and 3072 rows run, and 4096 is recorded as unsupported.