[dgarske]

Adds ECC384=1 (SECP384R1), ECCFAST=1 (drops WOLFSSL_SP_SMALL for the windowed SP path) and ECCBENCH=1, which swaps the test and benchmark harness for ECDSA sign/verify, ECDH and SHA-256/SHA-384 image-hash timings over a flash-resident 128 KB array. The stock benchmark reports nothing for ECC here, because the example builds with NO_ASN and bench_ecc() compiles its ECDSA section out behind that macro. The README gains the resulting table. ECCFAST=1 needs the companion SP recode fix in wolfSSL/wolfssl#11889. Validated on a LAUNCHXL-F28P55X.

[aidangarske]

Skoll Code Review

Scan type: review

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

Posted findings

• [Medium] P-384 lacks an independent correctness test — embedded/ti-c2000-f28p55x/Source/wolf_main.c:879-881
• [Medium] Failed iterations still produce successful-operation timing figures — embedded/ti-c2000-f28p55x/Source/wolf_main.c:693-701,746-770,802-810,849-859
• [Medium] The flash-image benchmark repeatedly hashes only a 4 KiB allocation — embedded/ti-c2000-f28p55x/Source/wolf_main.c:672-677
• [Medium] The documented ECCFAST option omits its required wolfSSL dependency — embedded/ti-c2000-f28p55x/README.md:94
• [Low] ECC_USER_CURVES comment still says P-256 only — embedded/ti-c2000-f28p55x/Header/user_settings.h:238

Review generated by Skoll

[aidangarske]

P-384 lacks an independent correctness test

ECC384=1 enables a new curve on a target whose 16-bit-byte behavior requires explicit validation, but P-384 is only exercised by self-generated sign/verify operations and a one-sided ECDH call when ECCBENCH=1 is also set. Building with ECC384=1 alone does not execute P-384 at all. Unlike the existing P-256 path, there is no deterministic verification KAT or comparison of secrets derived in both ECDH directions, so matching defects or serialization errors can escape detection.

Fix: Add a P-384 deterministic ECDSA verification KAT and bidirectional ECDH agreement check, mirroring wolf_ecc_test(), and run them whenever WOLF_ECC384 is enabled.

[aidangarske]

Failed iterations still produce successful-operation timing figures

The ECC loops break when an operation fails but still print the configured reps and divide by that value. A failed verification with ret == 0 and res == 0 likewise reports all repetitions as completed. The hash loops accumulate errors with ret |= ..., continue using the failed context, call Final, and publish a timing. Runtime failures, including an entropy-source failure in an ENTROPY=1 build, therefore produce misleading operation counts and latency figures.

Fix: Stop each benchmark on the first error, track the number of completed operations, and only calculate latency from successful iterations. Do not finalize a hash context after an update failure.

[aidangarske]

The flash-image benchmark repeatedly hashes only a 4 KiB allocation

The code processes 128 KiB in total, but it does so by hashing the same 4 KiB flash allocation 32 times. It therefore does not exercise the address range or working-set behavior of the claimed 128 KiB flash-resident image. This matters because the benchmark specifically attributes the measurement to flash wait-state behavior. On this target, a real secure-boot image may also use the packed flash representation documented elsewhere in the README, while this array stores one octet per 16-bit C byte.

Fix: Either stream across an actual image-sized flash allocation, preferably using the production packed/unpack path, or label the result as hashing 128 KiB from a repeated 4 KiB unpacked flash block and justify why that is representative.

[aidangarske]

The documented ECCFAST option omits its required wolfSSL dependency

The PR context states that ECCFAST=1 requires the companion SP recode fix in wolfSSL PR 11889, but the README presents the option without a minimum wolfSSL revision or prerequisite. Since WOLFROOT may point to another checkout, users can select the known-dependent windowed path and receive incorrect or unreproducible behavior.

Fix: Land the companion fix before this change, or document and, if possible, compile-time gate the required wolfSSL revision.

[aidangarske]

ECC_USER_CURVES comment still says P-256 only

The comment now contradicts the immediately following conditional P-384 configuration and can mislead future configuration changes.

Fix: Update the comment to describe both the mandatory P-256 curve and optional P-384 curve.

Suggestion:

Suggested change

	#define ECC_USER_CURVES
	#undef ECC_USER_CURVES /* build only the curves named below */