TI C2000 C28x: 16-bit-byte fixes, octet helpers, and an optional precomputed ML-DSA matrix A - #11344
TI C2000 C28x: 16-bit-byte fixes, octet helpers, and an optional precomputed ML-DSA matrix A#11344dgarske wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Fixes wolfCrypt behavior on 16-bit-byte targets (e.g., TI C28x, CHAR_BIT==16) by restoring SP backend wide-byte correctness, adding supported octet pack/unpack helpers for boundary conversions, and hardening SHA-3 byte loads to avoid lane contamination.
Changes:
- Restore
CHAR_BIT != 8correctness in generatedsp_c32.c(octet masks, constant-time mask shift). - Add
wc_UnpackOctets()/wc_PackOctets()plus sizing macros and a newoctets_test(). - Mask SHA-3 unaligned loads under
WOLFSSL_WIDE_BYTEto avoid high-bit bleed from wide byte cells.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
wolfssl/wolfcrypt/types.h |
Adds octet layout macros and wide-byte-only pack/unpack API declarations. |
wolfcrypt/src/wc_port.c |
Implements wide-byte pack/unpack helpers. |
wolfcrypt/test/test.c |
Adds and runs octets_test() to validate sizing + pack/unpack behavior. |
wolfcrypt/test/test.h |
Declares octets_test() for the test harness. |
wolfcrypt/src/wc_mldsa.c |
Ensures ML-DSA 6-bit packing truncates to octets on wide-byte targets. |
wolfcrypt/src/sp_c32.c |
Restores wide-byte safety fixes in generated SP serializers and masks. |
wolfcrypt/src/sha3.c |
Masks byte loads to octets under WOLFSSL_WIDE_BYTE. |
doc/dox_comments/header_files/types.h |
Documents the new pack/unpack APIs. |
IDE/C2000/compile.sh |
Updates guard comments for the C28x compile-only surface. |
IDE/C2000/README.md |
Documents octet representation and the new boundary conversion helpers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
13aa8ce to
d585e4b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
wc_PackOctets() currently performs a left shift that can be undefined on 16-bit-int targets, and the new precomputed-A API documentation doesn’t fully match its implemented error behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Lite
d585e4b to
852c88d
Compare
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: review
Overall recommendation: REQUEST_CHANGES
Findings: 5 total — 5 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] Precomputed matrix survives key replacement —
wolfcrypt/src/wc_mldsa.c:11850-11862 - [High] Default verifier ignores the attached matrix —
wolfcrypt/src/wc_mldsa.c:10952-10958 - [Medium] Precomputed-A API has no regression tests —
wolfcrypt/src/wc_mldsa.c:11833-11864 - [Medium] SP mask guard passes partial regressions —
.github/workflows/ti-c2000-compile.yml:170-182 - [Medium] New ML-DSA API is absent from canonical documentation —
wolfssl/wolfcrypt/wc_mldsa.h:768-783
Review generated by Skoll
852c88d to
9aa87a3
Compare
|
Jenkins retest this please |
|
Jenkins retest this please |
9aa87a3 to
2b98259
Compare
Support work for 16-bit-byte targets (TI C2000 C28x,
CHAR_BIT == 16). All of it is a no-op whereCHAR_BIT == 8, and the ML-DSA addition is off unless explicitly enabled.SP regression.
sp_c32.cis generated, and a regeneration reverted twoCHAR_BIT != 8fixes it carried: the& 0xFFoctet masks insp_*_to_bin_*, andsizeof(mp_digit) * CHAR_BIT(not* 8) in thesp_*_from_mpconstant-time mask. Without them ECDSA sign/verify, ECDH, RSA-2048 verify and DH FFDHE-2048 all fail on the C28x. Replaced with the output of the corrected generator, so the next regeneration is a no-op; the template fix is a companion PR in wolfSSL/scripts.Octet boundary helpers. A wolfCrypt
byte*buffer holds one octet perbytecell. Data arriving from flash or a byte-oriented link is packed two octets per cell instead, and callers had no supported way to convert. Addswc_UnpackOctets()/wc_PackOctets()-- both bounds-check source and destination -- plusWC_OCTETS_PER_BYTE/WC_PACKED_CELLS(). The functions are declared only underWOLFSSL_WIDE_BYTE: on an 8-bit-byte target the two layouts are the same buffer, so there is nothing to convert and no new API surface. Also masksmldsa_encode_w1_88_c(), whose 6-bit inputs reach 2795.SHA-3 absorb.
misc.c'sreadUnalignedWord32/64()mask every cell underWOLFSSL_WIDE_BYTE;sha3.c'sLoad64Unaligned()/Load64BitLittleEndian()did not, so SHA-3/SHAKE silently trusted callers to hand it clean octets. Now masked to match.Optional precomputed ML-DSA matrix A. ML-DSA verify expands the public matrix A from
rhoby SHAKE128 rejection sampling on every call, which dominates verify on a part with no SHA-3 acceleration. A depends only on the public key, so where the verification key is fixed at build time it can be expanded once off target and stored in flash. AddsWOLFSSL_MLDSA_VERIFY_PRECOMP_Aandwc_MlDsaKey_SetPrecompA(), which attaches a caller-owned, read-only matrix that is read in place and costs no RAM. It is consulted in the shared verify core, so it applies to every verify mode rather than one entry point. The setter binds the matrix to the key by comparingrhoand requires the public key to have been imported first, so a stale or mismatched matrix is rejected rather than silently verified against. With nothing attached the behavior is bit-identical to before, including when the gate is enabled.Security note: A is derived from the public key and must be integrity-protected exactly as the public key is, since substituting it can influence verification. This is stated at the API and in the header.
Also adds a CI job that fails if
sp_c32.closes its octet masks or reverts tosizeof(mp_digit) * 8, since that regression passes every 8-bit-byte build and all hardware-free CI. And setsWC_16BIT_CPUinIDE/C2000/user_settings.h-- the C28x has a 16-bitintas well as a 16-bitchar, and without it the 32-bit OID sums inwc_OidGetHash()collide once truncated. That letshash.c, which HashML-DSA depends on, join the compile guard.Test status
Hardware-validated on a TI LAUNCHXL-F28P55X (cl2000): the example's full 15-config matrix passes, including ML-DSA-44/65/87 verify,
wc_MlDsaKey_VerifyCtxHash()over SHA-256/SHA-512, a verify from a packed key and signature, and a pure-mode verify of a packed firmware image with a corrupted-image rejection. The five SP KATs fail before this change and pass after. The precomputed matrix was regenerated independently fromrhoand matched the stored one exactly. Hostmake checkandtestwolfcryptpass with the ML-DSA gate both on and off, andIDE/C2000/compile.shis clean.Notes
The
mldsa_encode_w1_88_c()mask is defensive rather than a live fix. Its overflow bits are, by construction, the same bits the next cell contributes at the same position in the 64-bit Keccak lane, and the absorb ORs octets into that lane -- so the stray bits are either idempotent or shifted out of the word entirely. Verified on hardware (the level-44 commitment matches with the mask reverted) and exhaustively in simulation (192,000 lanes, zero differences). The invariance depends on the absorb staying OR-based and on nothing else consuming the buffer, so the mask removes the dependency rather than fixing a defect.Generating the matrix is left to the application. A build-time generator and a complete worked example are in the companion wolfssl-examples PR wolfSSL/wolfssl-examples#617