fix: signed-shift UB in ML-DSA slow-multiply reductions#10919
fix: signed-shift UB in ML-DSA slow-multiply reductions#10919MarkAtwood wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses C undefined behavior in the opt-in ML-DSA “slow multiply” reduction paths by eliminating left shifts of routinely-negative signed values in mldsa_mont_red() and mldsa_red(), while preserving bit-identical results.
Changes:
- Updates the
MLDSA_MUL_QINV_SLOWpath inmldsa_mont_red()to perform shift-based arithmetic onword32instead of signed types. - Updates the
MLDSA_MUL_Q_SLOWpaths inmldsa_mont_red()andmldsa_red()to perform shift-based arithmetic using unsigned operands and cast back.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return (sword32)((a - (sword64)((word64)t << 23) + | ||
| (sword64)((word64)t << 13) - t) >> 32); |
There was a problem hiding this comment.
AI to AI: Reworked in ff34b2d: the Q_SLOW expression is now computed entirely in word64 with a single conversion back to sword64 before the arithmetic >> 32. The remaining unsigned-to-signed conversion is value-preserving (|result| < 2^56, well inside sword64 range) on the two's-complement targets wolfSSL supports. Verified: ML-DSA KATs pass under -fsanitize=shift -fno-sanitize-recover=all with both slow-mul defines; pre-fix code aborts on the same harness.
9af907f to
08aaf00
Compare
Under MLDSA_MUL_QINV_SLOW / MLDSA_MUL_Q_SLOW, mldsa_mont_red and mldsa_red left-shift signed operands that are routinely negative (NTT zetas), which is undefined behavior. Perform the shifts on word32/word64 and convert back; results are bit-identical. Only opt-in slow-multiply builds compile these paths. Reproduced with -fsanitize=shift --disable-intelasm; clean after this change.
08aaf00 to
ff34b2d
Compare
Under the opt-in
MLDSA_MUL_QINV_SLOW/MLDSA_MUL_Q_SLOWbuild options (both default OFF),mldsa_mont_red()andmldsa_red()left-shift signed operands that are routinely negative (the NTT zetas), which is undefined behavior (CWE-190, C11 6.5.7p4). The default multiply-based paths are unaffected.Fix: perform the shifts on
word32/word64and convert back. Results are bit-identical: the QINV path only ever keeps the low 32 bits, and the Q_SLOWword64shifts are exact (|t * 2^23| < 2^54). ML-DSA KATs pass.Verified with
-fsanitize=shift -DMLDSA_MUL_QINV_SLOW -DMLDSA_MUL_Q_SLOW --disable-intelasm(and-fwrapvremoved): pre-fix, seven shift-UB reports across the four flagged lines fire during the standard KAT test, includingleft shift of negative value; post-fix the run is clean. Note the C reduction paths only execute with Intel ASM disabled, another reason sanitizer nightlies never saw this.Credit: found by Dominik Blain of Qreative Lab (qreativelab.io) using their Z3-based formal analysis engine, reported with a machine-checked witness.
Related: #10917 (SLH-DSA base_2b) and #10918 (ML-DSA gamma1_19 encoder).