Add CB_ONLY mode for ML-KEM - #11212
Conversation
|
jenkins retest this please |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11212
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 10
10 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
6ee7ab0 to
1e0d76c
Compare
1e0d76c to
30b3da0
Compare
30b3da0 to
24c9732
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11212
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 6
4 finding(s) posted as inline comments (see file-level comments below)
Required changes (2)
CB_ONLY_MLKEM strips ML-KEM assembly that retained helpers still call, breaking the link
File: wolfcrypt/src/wc_mlkem_poly.c:7199
Function: mlkem_to_bytes
Category: Logic errors
WOLF_CRYPTO_CB_ONLY_MLKEM now compiles out all of wc_mlkem_asm.S and the ARM *-mlkem-asm* files, but wc_mlkem_poly.c keeps mlkem_to_bytes/mlkem_from_bytes/mlkem_cmp/mlkem_gen_matrix/mlkem_csubq_c, which reference mlkem_to_bytes_avx2, mlkem_csubq_neon, mlkem_arm32_csubq etc. Any asm-enabled build (--enable-intelasm, --enable-armasm) fails to link; wc_MlKemKey_EncodePublicKey needs mlkem_to_bytes.
Recommendation: Keep the still-referenced assembly helpers compiled under CB_ONLY_MLKEM, or disable the asm paths consistently in wc_mlkem_poly.c for that mode.
Referenced code: wolfcrypt/src/wc_mlkem_poly.c:7199-7201 (3 lines)
CB_ONLY_MLKEM strips ML-KEM assembly while wc_mlkem_poly.c still calls it
File: wolfcrypt/src/wc_mlkem_poly.c:7104
Function: mlkem_from_bytes
Category: Preprocessor-conditional security bypass
WOLF_CRYPTO_CB_ONLY_MLKEM disables the whole of wc_mlkem_asm.S (and the ARM ML-KEM asm), but the new gating in wc_mlkem_poly.c leaves mlkem_from_bytes, mlkem_to_bytes and mlkem_cmp compiled with their asm dispatch intact. With USE_INTEL_SPEEDUP (or aarch64 WOLFSSL_ARMASM) the library fails to link on mlkem_from_bytes_avx2, mlkem_to_bytes_avx2, mlkem_cmp_avx2/_avx512, mlkem_cmp_neon. No added CI config enables asm, so this is uncaught.
Recommendation: Keep the byte-packing/compare assembly routines compiled under WOLF_CRYPTO_CB_ONLY_MLKEM, or force WC_MLKEM_NO_ASM in that mode, and add an asm-enabled CB_ONLY_MLKEM CI entry.
Referenced code: wolfcrypt/src/wc_mlkem_poly.c:7104-7106 (3 lines)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
24c9732 to
3434b9d
Compare
|
Retest this please. (no logs) |
c45c30d to
0d7ec0f
Compare
philljj
left a comment
philljj
left a comment
There was a problem hiding this comment.
merge conflict in wolfcrypt test.c
0d7ec0f to
643f207
Compare
|
Retest this please. (all green except one hang in ready config) |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11212
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 4
3 finding(s) posted as inline comments (see file-level comments below)
Required changes (1)
ARM assembly builds with WOLF_CRYPTO_CB_ONLY_MLKEM lose mlkem_csubq while mlkem_to_bytes_c still calls it
File: wolfcrypt/src/wc_mlkem_poly.c:7337
Function: mlkem_to_bytes_c
Category: Preprocessor-conditional security bypass
mlkem_to_bytes_c is ungated and calls mlkem_csubq_c, which on WOLFSSL_ARMASM builds is a macro for mlkem_csubq_neon/mlkem_thumb2_csubq/mlkem_arm32_csubq (wc_mlkem.h:832/848/864). Those symbols now compile out with the ARM ML-KEM asm, so every ARM asm build with WOLF_CRYPTO_CB_ONLY_MLKEM fails to link wc_MlKemKey_EncodePrivateKey/EncodePublicKey. The Intel path was fixed at line 7365 but the ARM path was not.
Recommendation: Keep the C mlkem_csubq_c (or the ARM csubq asm) available when WOLF_CRYPTO_CB_ONLY_MLKEM is defined.
Referenced code: wolfcrypt/src/wc_mlkem_poly.c:7337-7339 (3 lines)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
643f207 to
1f6eb54
Compare
|
jenkins retest this please |
|
Retest this please. (PRB config A openssl test timeout) |
4 participants
Description
Saves approx 14kB when enabled/offloaded.
Testing
Added new tests
Checklist