fix: signed-shift UB in ML-DSA gamma1_19 encoder (32-bit path)#10918
Open
MarkAtwood wants to merge 1 commit into
Open
fix: signed-shift UB in ML-DSA gamma1_19 encoder (32-bit path)#10918MarkAtwood wants to merge 1 commit into
MarkAtwood wants to merge 1 commit into
Conversation
The 32-bit little-endian path shifts sword32 operands (z1 << 20, z3 << 28) whose values reach 2^20-1, overflowing INT32_MAX (C11 6.5.7p4). Declare the locals word32 with an explicit cast, the same idiom the gamma1_17/w1 encoders already use. Output bits unchanged; ML-DSA KATs pass on 32-bit and 64-bit paths.
This was referenced Jul 15, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes undefined behavior in the 32-bit little-endian implementation of the ML‑DSA gamma1_19 encoder by ensuring shift operands are unsigned, matching the pattern already used by sibling encoders and keeping the encoded output unchanged.
Changes:
- Change
z0..z3temporaries inmldsa_encode_gamma1_19_bits_c()fromsword32toword32with an explicit cast of the subtraction result. - Eliminate signed-left-shift overflow UB in the 32-bit packing path (
z1 << 20,z3 << 28) while preserving identical bit packing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Jenkins retest this please |
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
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.
The 32-bit little-endian path of
mldsa_encode_gamma1_19_bits_c()(compiled whenWC_64BIT_CPUis not set andWOLFSSL_MLDSA_ALIGNMENT <= 2) shiftssword32operands whose values reach 2^20-1:z1 << 20andz3 << 28both overflowINT32_MAX, which is undefined behavior (CWE-190, C11 6.5.7p4). The sibling encoders (gamma1_17, w1) already declare these locals asword32with an explicit cast; this encoder was missed.Fix: same idiom as the siblings,
word32 zN = (word32)(MLDSA_GAMMA1_19 - z[j + N]);. Output bits are unchanged. ML-DSA KATs pass on both the 32-bit and 64-bit paths.Verified with
-fsanitize=shift -DWC_32BIT_CPU -DWOLFSSL_MLDSA_ALIGNMENT=0(and-fwrapvremoved, since it suppresses the check): pre-fix,left shift of 1034459 by 20andleft shift of 325275 by 28fire at the two lines during the standard KAT test; post-fix the run is clean.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 (same bug class in SLH-DSA base_2b).