From 4e1f8792172cc3514450101e480e200f5d2b59a2 Mon Sep 17 00:00:00 2001 From: Dorn Hetzel Date: Sat, 19 Sep 2026 09:31:36 -0600 Subject: [PATCH 1/3] safegcd: compute the iteration count in u64 `iterations(bits)` evaluated `(45907 * bits + 30179) / 19929` in u32. The sum exceeds u32::MAX from bits = 93558, and a release build wraps it: at 98304 bits the reduction loop ran 10934 divsteps instead of 226447, so `invert_mod`/`invert_odd_mod` reported no inverse for invertible values and `gcd`/`gcd_odd` returned partially reduced results. The result itself (about 2.3 * bits) outgrows u32 above ~1.86e9 bits, so the count and the remaining-steps accumulators are now u64 throughout; `next_batch` narrows only the per-batch count (at most 62) back to u32. Tests cover the exact counts around the overflow point and inversion and gcd at 93568 bits, the smallest limb-aligned precision affected, checked against num-bigint. They fail on the previous arithmetic. --- CHANGELOG.md | 5 +++ src/modular/safegcd.rs | 58 +++++++++++++++++++++++++----- src/modular/safegcd/boxed.rs | 68 ++++++++++++++++++++++++++++++++---- src/uint/boxed/invert_mod.rs | 32 ++++++++++++++++- 4 files changed, 147 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 817e41fb7..4fbaabb00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Implement `Zeroize` for `EncodedUint` ([#1342]) +### Fixed +- `u32` overflow of the safegcd iteration count at precisions of 93558 bits and above, which made + `invert_mod`/`invert_odd_mod` spuriously report no inverse and `gcd` return wrong results ([#TBD]) + [#1342]: https://github.com/RustCrypto/crypto-bigint/pull/1342 +[#TBD]: https://github.com/RustCrypto/crypto-bigint/pull/TBD ## 0.7.5 (2026-06-22) ### Added diff --git a/src/modular/safegcd.rs b/src/modular/safegcd.rs index f7b23e175..ae32c2279 100644 --- a/src/modular/safegcd.rs +++ b/src/modular/safegcd.rs @@ -12,7 +12,7 @@ #[cfg(feature = "alloc")] pub(crate) mod boxed; -use crate::{Choice, CtOption, I64, Int, Limb, Odd, U64, Uint, bitlen, primitives::u32_min}; +use crate::{Choice, CtOption, I64, Int, Limb, Odd, U64, Uint, bitlen}; use core::fmt; const GCD_BATCH_SIZE: u32 = 62; @@ -122,11 +122,11 @@ const fn invert_odd_mod_precomp( if VARTIME && g.is_zero_vartime() { break; } - let batch = u32_min(steps, GCD_BATCH_SIZE); + let (batch, remaining) = next_batch(steps); (delta, t) = jump::(f.lowest(), g.lowest(), delta, batch); (f, g) = update_fg(&f, &g, t, batch); (d, e) = update_de(&d, &e, m.as_ref(), mi, t, batch); - steps -= batch; + steps = remaining; } let d = d.norm(f.is_negative(), m.as_ref()); @@ -147,10 +147,10 @@ pub const fn gcd_odd( if VARTIME && g.is_zero_vartime() { break; } - let batch = u32_min(steps, GCD_BATCH_SIZE); + let (batch, remaining) = next_batch(steps); (delta, t) = jump::(f.lowest(), g.lowest(), delta, batch); (f, g) = update_fg(&f, &g, t, batch); - steps -= batch; + steps = remaining; } f.magnitude().to_odd().expect_copied("odd by construction") @@ -343,12 +343,32 @@ const fn shr_in_place_wide( /// Calculate the maximum number of iterations required according to /// safegcd-bounds: +/// +/// Computed in `u64`: `45907 * bits + 30179` exceeds `u32::MAX` once `bits >= 93558`, and the +/// result itself (about `2.3 * bits`) exceeds `u32::MAX` for `bits` above about `1.86e9`. // NOTE: the division is non-constant-time, but this is used to compute the number of iterations we // perform which is leaked in timing information #[inline] #[allow(clippy::integer_division_remainder_used, reason = "public parameter")] -const fn iterations(bits: u32) -> u32 { - (45907 * bits + 30179) / 19929 +#[allow(clippy::cast_lossless, reason = "`const fn`")] +const fn iterations(bits: u32) -> u64 { + (45907 * bits as u64 + 30179) / 19929 +} + +/// Split `steps` into the next batch of at most `GCD_BATCH_SIZE` reduction steps and the steps +/// remaining after it. +#[inline] +#[allow(clippy::cast_lossless, reason = "`const fn`")] +#[allow( + clippy::cast_possible_truncation, + reason = "`steps < GCD_BATCH_SIZE` where narrowed" +)] +const fn next_batch(steps: u64) -> (u32, u64) { + if steps < GCD_BATCH_SIZE as u64 { + (steps as u32, 0) + } else { + (GCD_BATCH_SIZE, steps - GCD_BATCH_SIZE as u64) + } } /// A `Uint` which carries a separate sign in order to maintain the same range. @@ -530,7 +550,7 @@ impl PartialEq for SignedInt { #[cfg(test)] mod tests { - use super::SafeGcdInverter; + use super::{GCD_BATCH_SIZE, SafeGcdInverter, iterations, next_batch}; use crate::{U128, U256, modular::safegcd::shr_in_place_wide}; #[test] @@ -568,4 +588,26 @@ mod tests { ); assert_eq!(b_hi, U128::from_u128(0x111111112222222)); } + + #[test] + fn iterations_do_not_overflow() { + // Values from the `safegcd-bounds` formula evaluated in arbitrary precision. + assert_eq!(iterations(256), 591); + assert_eq!(iterations(93_504), 215_390); + // `45907 * bits + 30179` first exceeds `u32::MAX` here. + assert_eq!(iterations(93_558), 215_514); + assert_eq!(iterations(93_568), 215_537); + assert_eq!(iterations(98_304), 226_447); + // The result itself exceeds `u32::MAX` for the largest precisions. + assert_eq!(iterations(u32::MAX), 9_893_575_374); + } + + #[test] + fn next_batch_is_bounded() { + assert_eq!(next_batch(0), (0, 0)); + assert_eq!(next_batch(61), (61, 0)); + assert_eq!(next_batch(62), (GCD_BATCH_SIZE, 0)); + assert_eq!(next_batch(63), (GCD_BATCH_SIZE, 1)); + assert_eq!(next_batch(u64::MAX), (GCD_BATCH_SIZE, u64::MAX - 62)); + } } diff --git a/src/modular/safegcd/boxed.rs b/src/modular/safegcd/boxed.rs index 09f4695a5..fa4f61641 100644 --- a/src/modular/safegcd/boxed.rs +++ b/src/modular/safegcd/boxed.rs @@ -3,7 +3,7 @@ //! //! See parent module for more information. -use super::{GCD_BATCH_SIZE, Matrix, iterations, jump}; +use super::{Matrix, iterations, jump, next_batch}; use crate::{ BoxedUint, Choice, ConcatenatingMul, CtAssign, CtOption, CtSelect, I64, Int, Limb, NonZero, Odd, Resize, U64, Uint, @@ -112,11 +112,11 @@ fn invert_odd_mod_precomp( if VARTIME && g.is_zero_vartime() { break; } - let batch = u32_min(steps, GCD_BATCH_SIZE); + let (batch, remaining) = next_batch(steps); (delta, t) = jump::(f.lowest(), g.lowest(), delta, batch); (f, g) = update_fg(&f, &g, t, batch); (d, e) = update_de(&d, &e, &m, mi, t, batch); - steps -= batch; + steps = remaining; } let d = d @@ -188,10 +188,10 @@ pub fn gcd_odd(f: &Odd, g: &BoxedUint) -> Odd(f.lowest(), g.lowest(), delta, batch); (f, g) = update_fg(&f, &g, t, batch); - steps -= batch; + steps = remaining; } f.magnitude() @@ -441,8 +441,62 @@ impl fmt::Debug for SignedBoxedInt { #[cfg(test)] mod tests { - use super::BoxedSafeGcdInverter; - use crate::BoxedUint; + use super::{BoxedSafeGcdInverter, gcd_odd, invert_odd_mod}; + use crate::{BoxedUint, Odd, Resize}; + use num_bigint::BigUint; + use num_integer::Integer; + + /// The smallest precision at which `45907 * bits + 30179` no longer fits in `u32`. + const LARGE_BITS: u32 = 93_568; + + /// A deterministic, odd, full-width `BoxedUint` of `bits` bits; different seeds give + /// unrelated values. + fn large_odd(bits: u32, seed: u32) -> BoxedUint { + let mut bytes: alloc::vec::Vec = (0..bits >> 3) + .map(|i| { + let h = (i ^ seed.wrapping_mul(0x9E37_79B9)).wrapping_mul(0x85EB_CA6B); + let h = (h ^ (h >> 13)).wrapping_mul(0xC2B2_AE35); + (h ^ (h >> 16)).to_le_bytes()[0] + }) + .collect(); + bytes[0] |= 0x80; + *bytes.last_mut().unwrap() |= 1; + BoxedUint::from_be_slice(&bytes, bits).unwrap() + } + + fn to_big(x: &BoxedUint) -> BigUint { + BigUint::from_bytes_be(&x.to_be_bytes()) + } + + #[test] + fn invert_odd_mod_large_precision() { + let m = large_odd(LARGE_BITS, 1); + let a = BoxedUint::from(65537u32).resize(LARGE_BITS); + assert_eq!(to_big(&a).gcd(&to_big(&m)), BigUint::from(1u32)); + let m = Odd::new(m).unwrap(); + + for inv in [ + invert_odd_mod::(&a, &m).unwrap(), + invert_odd_mod::(&a, &m).unwrap(), + ] { + assert_eq!( + (to_big(&a) * to_big(&inv)) % to_big(m.as_ref()), + BigUint::from(1u32) + ); + } + } + + #[test] + fn gcd_odd_large_precision() { + // Two unrelated full-width values: the reduction needs the whole iteration budget, + // unlike a pair with a large common factor and small cofactors. + let f = Odd::new(large_odd(LARGE_BITS, 2)).unwrap(); + let g = large_odd(LARGE_BITS, 4); + let expected = to_big(f.as_ref()).gcd(&to_big(&g)); + + assert_eq!(to_big(gcd_odd::(&f, &g).as_ref()), expected); + assert_eq!(to_big(gcd_odd::(&f, &g).as_ref()), expected); + } #[test] fn invert() { diff --git a/src/uint/boxed/invert_mod.rs b/src/uint/boxed/invert_mod.rs index db56af268..537c5eafd 100644 --- a/src/uint/boxed/invert_mod.rs +++ b/src/uint/boxed/invert_mod.rs @@ -184,10 +184,40 @@ impl InvertMod for BoxedUint { #[cfg(test)] mod tests { - use crate::{Limb, Odd, Resize, U256}; + use crate::{Limb, NonZero, Odd, Resize, U256}; use super::BoxedUint; use hex_literal::hex; + use num_bigint::BigUint; + use num_integer::Integer; + + /// `invert_mod` at a precision where the safegcd iteration count overflowed `u32` + /// (`45907 * bits + 30179 > u32::MAX` from 93558 bits), with an even modulus so that + /// both the odd-modulus and the `2^k` paths are exercised. + #[test] + fn invert_mod_large_precision_even_modulus() { + const BITS: u32 = 93_568; + let mut bytes: alloc::vec::Vec = (0..BITS >> 3) + .map(|i| { + let h = (i ^ 0xDA94_2042).wrapping_mul(0x85EB_CA6B); + let h = (h ^ (h >> 13)).wrapping_mul(0xC2B2_AE35); + (h ^ (h >> 16)).to_le_bytes()[0] + }) + .collect(); + bytes[0] |= 0x80; + *bytes.last_mut().unwrap() &= !1; + let m = BoxedUint::from_be_slice(&bytes, BITS).unwrap(); + let a = BoxedUint::from(65537u32).resize(BITS); + + let to_big = |x: &BoxedUint| BigUint::from_bytes_be(&x.to_be_bytes()); + assert_eq!(to_big(&a).gcd(&to_big(&m)), BigUint::from(1u32)); + + let inv = a.invert_mod(&NonZero::new(m.clone()).unwrap()).unwrap(); + assert_eq!( + (to_big(&a) * to_big(&inv)) % to_big(&m), + BigUint::from(1u32) + ); + } #[test] fn invert_mod2k() { From ebf5b9f967f1a8d83ff2557e08603816662cd5e4 Mon Sep 17 00:00:00 2001 From: Dorn Hetzel Date: Sat, 19 Sep 2026 09:54:03 -0600 Subject: [PATCH 2/3] changelog: reference #1346 --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fbaabb00..c3e9c2196 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - `u32` overflow of the safegcd iteration count at precisions of 93558 bits and above, which made - `invert_mod`/`invert_odd_mod` spuriously report no inverse and `gcd` return wrong results ([#TBD]) + `invert_mod`/`invert_odd_mod` spuriously report no inverse and `gcd` return wrong results ([#1346]) [#1342]: https://github.com/RustCrypto/crypto-bigint/pull/1342 -[#TBD]: https://github.com/RustCrypto/crypto-bigint/pull/TBD +[#1346]: https://github.com/RustCrypto/crypto-bigint/pull/1346 ## 0.7.5 (2026-06-22) ### Added From 6c615d186367faefc522be5cbefb8fed7c7cdfb1 Mon Sep 17 00:00:00 2001 From: Dorn Hetzel Date: Sat, 19 Sep 2026 10:33:12 -0600 Subject: [PATCH 3/3] safegcd: keep the iteration count u32, checked instead of wrapped Review feedback: compute in u64 and narrow the result back to u32, so the loops keep their u32 step counters. The narrowing is checked with a const assertion, which fires only for precisions above 1864517463 bits (a 233 MB operand), where the old arithmetic would have wrapped. --- src/modular/safegcd.rs | 56 ++++++++++++++---------------------- src/modular/safegcd/boxed.rs | 10 +++---- 2 files changed, 27 insertions(+), 39 deletions(-) diff --git a/src/modular/safegcd.rs b/src/modular/safegcd.rs index ae32c2279..b279a40f1 100644 --- a/src/modular/safegcd.rs +++ b/src/modular/safegcd.rs @@ -12,7 +12,7 @@ #[cfg(feature = "alloc")] pub(crate) mod boxed; -use crate::{Choice, CtOption, I64, Int, Limb, Odd, U64, Uint, bitlen}; +use crate::{Choice, CtOption, I64, Int, Limb, Odd, U64, Uint, bitlen, primitives::u32_min}; use core::fmt; const GCD_BATCH_SIZE: u32 = 62; @@ -122,11 +122,11 @@ const fn invert_odd_mod_precomp( if VARTIME && g.is_zero_vartime() { break; } - let (batch, remaining) = next_batch(steps); + let batch = u32_min(steps, GCD_BATCH_SIZE); (delta, t) = jump::(f.lowest(), g.lowest(), delta, batch); (f, g) = update_fg(&f, &g, t, batch); (d, e) = update_de(&d, &e, m.as_ref(), mi, t, batch); - steps = remaining; + steps -= batch; } let d = d.norm(f.is_negative(), m.as_ref()); @@ -147,10 +147,10 @@ pub const fn gcd_odd( if VARTIME && g.is_zero_vartime() { break; } - let (batch, remaining) = next_batch(steps); + let batch = u32_min(steps, GCD_BATCH_SIZE); (delta, t) = jump::(f.lowest(), g.lowest(), delta, batch); (f, g) = update_fg(&f, &g, t, batch); - steps = remaining; + steps -= batch; } f.magnitude().to_odd().expect_copied("odd by construction") @@ -344,31 +344,22 @@ const fn shr_in_place_wide( /// Calculate the maximum number of iterations required according to /// safegcd-bounds: /// -/// Computed in `u64`: `45907 * bits + 30179` exceeds `u32::MAX` once `bits >= 93558`, and the -/// result itself (about `2.3 * bits`) exceeds `u32::MAX` for `bits` above about `1.86e9`. +/// Computed in `u64` because `45907 * bits + 30179` exceeds `u32::MAX` once `bits >= 93558`. +/// The count itself (about `2.3 * bits`) only exceeds `u32::MAX` for `bits` above `1864517463`, +/// which panics rather than wrapping. // NOTE: the division is non-constant-time, but this is used to compute the number of iterations we // perform which is leaked in timing information #[inline] #[allow(clippy::integer_division_remainder_used, reason = "public parameter")] #[allow(clippy::cast_lossless, reason = "`const fn`")] -const fn iterations(bits: u32) -> u64 { - (45907 * bits as u64 + 30179) / 19929 -} - -/// Split `steps` into the next batch of at most `GCD_BATCH_SIZE` reduction steps and the steps -/// remaining after it. -#[inline] -#[allow(clippy::cast_lossless, reason = "`const fn`")] -#[allow( - clippy::cast_possible_truncation, - reason = "`steps < GCD_BATCH_SIZE` where narrowed" -)] -const fn next_batch(steps: u64) -> (u32, u64) { - if steps < GCD_BATCH_SIZE as u64 { - (steps as u32, 0) - } else { - (GCD_BATCH_SIZE, steps - GCD_BATCH_SIZE as u64) - } +#[allow(clippy::cast_possible_truncation, reason = "checked by the assertion")] +const fn iterations(bits: u32) -> u32 { + let iterations = (45907 * bits as u64 + 30179) / 19929; + assert!( + iterations <= u32::MAX as u64, + "precision too large for safegcd" + ); + iterations as u32 } /// A `Uint` which carries a separate sign in order to maintain the same range. @@ -550,7 +541,7 @@ impl PartialEq for SignedInt { #[cfg(test)] mod tests { - use super::{GCD_BATCH_SIZE, SafeGcdInverter, iterations, next_batch}; + use super::{SafeGcdInverter, iterations}; use crate::{U128, U256, modular::safegcd::shr_in_place_wide}; #[test] @@ -598,16 +589,13 @@ mod tests { assert_eq!(iterations(93_558), 215_514); assert_eq!(iterations(93_568), 215_537); assert_eq!(iterations(98_304), 226_447); - // The result itself exceeds `u32::MAX` for the largest precisions. - assert_eq!(iterations(u32::MAX), 9_893_575_374); + // The largest precision whose count still fits in `u32`. + assert_eq!(iterations(1_864_517_463), 4_294_967_294); } #[test] - fn next_batch_is_bounded() { - assert_eq!(next_batch(0), (0, 0)); - assert_eq!(next_batch(61), (61, 0)); - assert_eq!(next_batch(62), (GCD_BATCH_SIZE, 0)); - assert_eq!(next_batch(63), (GCD_BATCH_SIZE, 1)); - assert_eq!(next_batch(u64::MAX), (GCD_BATCH_SIZE, u64::MAX - 62)); + #[should_panic(expected = "precision too large for safegcd")] + fn iterations_panics_beyond_u32() { + iterations(1_864_517_464); } } diff --git a/src/modular/safegcd/boxed.rs b/src/modular/safegcd/boxed.rs index fa4f61641..35d730abe 100644 --- a/src/modular/safegcd/boxed.rs +++ b/src/modular/safegcd/boxed.rs @@ -3,7 +3,7 @@ //! //! See parent module for more information. -use super::{Matrix, iterations, jump, next_batch}; +use super::{GCD_BATCH_SIZE, Matrix, iterations, jump}; use crate::{ BoxedUint, Choice, ConcatenatingMul, CtAssign, CtOption, CtSelect, I64, Int, Limb, NonZero, Odd, Resize, U64, Uint, @@ -112,11 +112,11 @@ fn invert_odd_mod_precomp( if VARTIME && g.is_zero_vartime() { break; } - let (batch, remaining) = next_batch(steps); + let batch = u32_min(steps, GCD_BATCH_SIZE); (delta, t) = jump::(f.lowest(), g.lowest(), delta, batch); (f, g) = update_fg(&f, &g, t, batch); (d, e) = update_de(&d, &e, &m, mi, t, batch); - steps = remaining; + steps -= batch; } let d = d @@ -188,10 +188,10 @@ pub fn gcd_odd(f: &Odd, g: &BoxedUint) -> Odd(f.lowest(), g.lowest(), delta, batch); (f, g) = update_fg(&f, &g, t, batch); - steps = remaining; + steps -= batch; } f.magnitude()