LMS: add to wolfCrypt -Wconversion CI matrix and clean conversion warnings#14
Closed
LMS: add to wolfCrypt -Wconversion CI matrix and clean conversion warnings#14
Conversation
Add --enable-lms to all 11 existing wolfCrypt-Wconversion matrix rows so the LMS sources (wc_lms.c, wc_lms_impl.c) are exercised under the same strict conversion-warning flags already enforced for ML-KEM. Add 4 new matrix rows that cover the LMS sub-options (small, verify-only, sha256-192+shake256, and a combined small+sha256-192+shake256 32-bit build) which use distinct code paths. To make the LMS sources compile cleanly under -Wconversion -Warith-conversion -Wsign-conversion -Wcast-qual: - wc_lms.h: give the small length constants U suffix and add (word32) casts inside the size macros (LMS_PRIV_LEN, LMS_SIG_LEN, HSS_PRIVATE_KEY_LEN, LMS_PRIV_STATE_LEN, LMS_PRIV_Y_TREE_LEN, ...) so the arithmetic stays in unsigned int rather than implicitly converting through signed int. LMS_PRIV_Y_TREE_LEN is now defined unconditionally (still wrapped by the sig-cache guard, but the size formula it returns is also needed by code paths that don't depend on the cache). - wc_lms_impl.c: switch the qe-expand sum to unsigned int with U-suffix literals; widen the sha256 padding remainder to word32; replace hand-rolled (params->p * params->hash_len + ...) expressions with the fixed macros (LMS_SIG_LEN, LMS_PRIV_Y_TREE_LEN, LMS_STACK_CACHE_LEN); cast loop indices to word32 only where they multiply a word32 macro result; rewrite a few `int off = ((int)1 << ...) ...` expressions as `word32 off = (1U << ...) ...`; change wc_lms_idx_zero/inc length parameter to word32 to avoid the int->size_t conversion in XMEMSET. - wc_lms.c: change priv_data_len from int to word32 so it flows cleanly into ForceZero/XMALLOC/the write_private_key callback; cast the public API's int msgSz to (word32) when forwarding to wc_hss_sign / wc_hss_verify. - sha256.c: drop the gratuitous (word32*) cast on the const data input in wc_Sha256HashBlock (LMS-gated function); ByteReverseWords already takes const word32*. Verified locally on every matrix row (rows 1-15) with a clean make -j 4. testwolfcrypt LMS Vfy / LMS tests still pass. The bench_lms benchmark shows no regression > 2% across all parameter sets (most ops are within noise or slightly faster). https://claude.ai/code/session_01EJmy1bKDgHseTwZ5Qqpu1g
- Fix small-variant signing regression (review finding bonus): reverting word16 sum -> unsigned int in wc_lmots_q_expand broke the small-variant checksum loop, which relies on arithmetic wrapping at 2^16. Restore word16 sum with explicit (word16) casts at every assignment so the -Wconversion warning is silenced without changing semantics. Caught by the new runtime test job (also added). - Fix 16-bit shift UB (review #1): the matrix doesn't cover 16-bit, but on word32 = unsigned long targets `1U << params->height` is undefined for height >= 16. Restore (word32)1U on every shift whose exponent can reach height (LMS_Q_AT_LEVEL macro and four open-coded sites). - Add msgSz <= 0 check to wc_LmsKey_Verify (review #2): wc_LmsKey_Sign already rejects non-positive msgSz; the public verify entry point did not. Without it, the (word32)msgSz cast forwarded a huge value to wc_hss_verify and caused buffer over-read. - Add a test_lms_runtime CI job (review #11): the existing matrix only builds; it cannot catch a semantic regression like the q_expand one above. The new job builds --enable-lms and --enable-lms=yes,small and runs testwolfcrypt for both. - Make wc_hss_make_key loop index word32 (review #10) so the (int)cast on HSS_MAX_LEVELS in the second-loop bound is no longer needed. - Add LMOTS_Y_LEN as the canonical name for the LM-OTS y[] length and alias LMS_PRIV_Y_TREE_LEN to it (review #6); switch the non-cache call sites in wc_hss_sign / wc_lms_sig_copy / wc_hss_sign_build_sig to LMOTS_Y_LEN to match intent. Verified: 15 matrix rows still build clean under the conversion flags; testwolfcrypt LMS Vfy / LMS pass for --enable-lms, --enable-lms=yes,small, and --enable-lms=yes,small,sha256-192,shake256; bench_lms shows no regression. https://claude.ai/code/session_01EJmy1bKDgHseTwZ5Qqpu1g
This was referenced May 3, 2026
Frauschi
pushed a commit
that referenced
this pull request
May 4, 2026
Adds --enable-slhdsa to the existing 11 wolfCrypt-Wconversion build rows and adds 4 dedicated rows for the sub-options that the default doesn't cover (small, small-mem, sha2, verify-only on 32-bit). Adds a test_slhdsa_runtime job mirroring test_lms_runtime / test_xmss_runtime that builds without --disable-crypttests and runs testwolfcrypt for five configs (default-SHAKE, small, sha2, verify-only, 32-bit). Cleans the conversion warnings that surface in wc_slhdsa.c and the SLH-DSA code paths in asn.c. Patterns applied (same as LMS PR #14 and XMSS PR #15): - U-suffix on integer literals that flow into word32 size fields (SLHDSA_W, SLHDSA_WM1, 2*n -> 2U*n, len*n -> (word32)len*n, sk-size literals 4 and 3 -> 4U / 3U at ForceZero / XMEMCPY / wc_RNG_GenerateBlock call sites). - (word32)1U << z for shifts whose exponent reaches the SLH-DSA tree height parameters (params->h, params->h_m, params->d) up to 68 for 256s/f variants - avoids 32-bit UB. - word16 csum / mask accumulators with explicit (word16) casts at the assignment boundary for WOTS+ checksum and base-2^b decomposition. - HA_*, SHAKE256_SET_*_X4 and HASH_H/HASH_F macros: parenthesise operands and add (word32) / (size_t) / (byte) casts so the argument types narrow at the macro boundary, not at every call site. - slhdsakey_chain_idx_x4_NN signature: byte i, byte s -> word32 i, word32 s; loop counters int j -> word32 j; call sites cast at invocation. - slhdsakey_shake256_set_seed_ha_x4 / _hash_x4 return type int -> word32; locals to word32. - XMSS / FORS tree generators: introduce const word32 wi = (word32)i / wz = (word32)z and use wi/wz in the inner-loop offset arithmetic so each multiplication / shift is in word32 from the start. - word64* cast-qual fixes: add const where the helper only reads its input (slhdsakey_hash_h_2_x4 inputs, etc.). - asn.c: int slhDsaParam = wc_SlhDsaOidToParam((int)keyOID) and ret = (int)outSz at three call sites where outSz is word32. Critical semantic fix: slhdsakey_base_2b wrote baseb[j] = (byte)((total >> bits) & mask); baseb is word16* and FORS values can be up to 14 bits (parameter a). The byte cast truncated values >255 and broke verification on FORS variants. Changed to (word16) - caught by testwolfcrypt under the new runtime job. Verified clean against all 15 build matrix rows (default, intelasm, smallstack, smallstack+intelasm, NO_INT128, 32-bit, mlkem-small, mlkem-no-large-code, smallstack+NO_INT128, mlkem-small-mem-32bit, mlkem-small-mem+NO_INT128, slhdsa=yes,small / small-mem / sha2 / verify-only-32bit). All five test_slhdsa_runtime configs pass testwolfcrypt's slhdsa_test(). Benchmark with -DBENCH_MIN_RUNTIME_SEC=5.0F across the six SHAKE variants (128s/f, 192s/f, 256s/f) shows sign / verify deltas within +/-5% of master after this change. https://claude.ai/code/session_01EJmy1bKDgHseTwZ5Qqpu1g SLH-DSA Wconversion: review fixes Address review findings from the previous commit: - wc_slhdsa.c:1681: replace the (incorrect) mask comment with a comment on the actual word16 cast. baseb is word16* and FORS values reach 14 bits (parameter a), so the original (byte) cast silently truncated. - wc_slhdsa.c:57-59: revert SLHDSA_W / SLHDSA_WM1 to plain int. The U suffix forced cascaded (int)SLHDSA_WM1, (sword8)SLHDSA_WM1 and (byte)(SLHDSA_WM1 - x) casts at every signed-comparison site without saving anything. The (word16) cast at the WOTS+ csum / mask assignment is sufficient. - wc_slhdsa.c: drop (int)SLHDSA_WM1 at three call sites in slhdsakey_chain_idx_to_max_{16,24,32}, made redundant by the WM1 revert. - wc_slhdsa.c: unify (word32)1U shift form across all eight call sites; the previous commit changed only four of them. - wc_slhdsa.c:8305, 8340: switch ExportPrivate / ExportPublic n back to byte to match every other internal use of params->n. - wc_slhdsa.c:127: add wc_static_assert(SLHDSA_MAX_MSG_SZ <= 255) to document the invariant that the WOTS+ chain helpers' (byte)i and (byte)j casts depend on (max len for current parameter sets is 2*32+3 = 67). - wc_slhdsa.c:4315-4320: deduplicate the csum-word16 explanatory comment; the second copy now defers to the first. - wolfCrypt-Wconversion.yml: bump build_library and test_slhdsa_runtime timeout-minutes from 6 to 10. SLH-DSA adds ~9 kLoC to every row and the 32-bit multilib + smallstack rows need headroom. All five representative build rows still compile clean (default, intelasm, smallstack, sha2, small-mem, verify-only-32bit). All five test_slhdsa_runtime configs still pass slhdsa_test(). https://claude.ai/code/session_01EJmy1bKDgHseTwZ5Qqpu1g
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--enable-lmsto all 11 existing rows ofwolfCrypt-Wconversion.ymland 4 new dedicated rows for the LMS sub-options (small,verify-only,sha256-192,shake256, and a combined small+sha256-192+shake256 32-bit build), so the LMS sources are exercised under the same strict conversion-warning flags as ML-KEM.test_lms_runtimeCI job that builds with--enable-lmsand--enable-lms=yes,smalland runstestwolfcrypt, so a silent semantic regression in a cast cleanup gets caught (the original commit had exactly this — see below).-Wconversion -Warith-conversion -Wsign-conversion -Wcast-qualwarnings inwolfcrypt/src/wc_lms.c,wolfcrypt/src/wc_lms_impl.c,wolfssl/wolfcrypt/wc_lms.h, and one LMS-gated function inwolfcrypt/src/sha256.c.Cleanup approach (kept casts minimal, avoided unnecessary widening)
wc_lms.h: U-suffix on the small length constants and(word32)(hLen)casts inside the size macros (LMS_PRIV_LEN,LMS_SIG_LEN,HSS_PRIVATE_KEY_LEN,LMS_PRIV_STATE_LEN,LMS_PRIV_Y_TREE_LEN, …) so arithmetic stays inunsigned intrather than implicitly converting through signedint.LMOTS_Y_LEN(p, hLen)helper for non-cache call sites that need the LM-OTS y[] length;LMS_PRIV_Y_TREE_LENaliases to it (and is now defined unconditionally because the size formula is also used by code paths that don't depend on the sig cache).wc_lms_impl.c: widened the SHA-256 padding remainder frominttoword32; replaced hand-rolled(params->p * params->hash_len + …)expressions with the fixed macros (LMS_SIG_LEN,LMOTS_Y_LEN,LMS_STACK_CACHE_LEN); cast loop indices toword32only where they multiply aword32macro result; rewrote a fewint off = ((int)1 << …) …expressions asword32 off = ((word32)1U << …) …; changedwc_lms_idx_zero/wc_lms_idx_inclength parameter toword32to avoid theint → size_tconversion inXMEMSET.wc_lms.c: changedpriv_data_lenfrominttoword32so it flows cleanly intoForceZero/XMALLOC/ thewrite_private_keycallback; cast the public API'sint msgSztoword32when forwarding towc_hss_sign/wc_hss_verify; added the missingmsgSz <= 0check inwc_LmsKey_Verify(wc_LmsKey_Signalready had it).sha256.c: dropped the gratuitous(word32*)cast on theconst datainput of the LMS-gatedwc_Sha256HashBlock;ByteReverseWordsalready takesconst word32*.Notable wrinkle
wc_lmots_q_expand's checksum loop relies onsum's arithmetic wrapping at 2^16 (sum <<= wthensum >> (16 - w)reads the top byte of the rolled value). My first cleanup pass widenedsumtounsigned int, which silently broke the small-variant signer — the newtest_lms_runtimejob caught it on the next push. Reverted toword16 sumwith explicit(word16)casts at every assignment.Test plan
make -j 4under the conversion flags (verified locally).testwolfcryptpasses bothLMS VfyandLMSfor--enable-lms,--enable-lms=yes,small, and--enable-lms=yes,small,sha256-192,shake256../wolfcrypt/benchmark/benchmark -lms_hssshows no regression > 2 % vs. master across the parameter sets.build_librarymatrix + newtest_lms_runtimejob.https://claude.ai/code/session_01EJmy1bKDgHseTwZ5Qqpu1g
Generated by Claude Code