From 12f74e11e54ed2d8dacbd6a72ec4a4f3a70d879c Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Sun, 19 Jan 2025 17:08:51 -0800 Subject: [PATCH] arithmetic internals: Restore old signature of bn_mul_mont. In BoringSSL, it returns `int`. Have it do that again, to make the next bit of refactoring easier, where we replace `bn_mul_mont` with a Rust function that dispatches to a bunch of other functions, all of which also are declared as having the same semantics. The good thing is that this restores the assembly code to match BoringSSL. The bad thing is that this some bits of dead code to handle zero return values that we'll never see. --- crypto/fipsmodule/bn/asm/x86-mont.pl | 3 +++ crypto/fipsmodule/bn/asm/x86_64-mont.pl | 2 +- crypto/fipsmodule/bn/internal.h | 17 +++-------------- crypto/fipsmodule/ec/gfp_p256.c | 5 ++++- crypto/fipsmodule/ec/gfp_p384.c | 10 ++++++++-- src/arithmetic/ffi.rs | 18 ++++++++++++------ src/arithmetic/montgomery.rs | 3 ++- src/bssl.rs | 12 ++++++++++++ 8 files changed, 45 insertions(+), 25 deletions(-) diff --git a/crypto/fipsmodule/bn/asm/x86-mont.pl b/crypto/fipsmodule/bn/asm/x86-mont.pl index eccefa051c..6f7e25c7dc 100755 --- a/crypto/fipsmodule/bn/asm/x86-mont.pl +++ b/crypto/fipsmodule/bn/asm/x86-mont.pl @@ -68,6 +68,8 @@ &xor ("eax","eax"); &mov ("edi",&wparam(5)); # int num + &cmp ("edi",4); + &jl (&label("just_leave")); &lea ("esi",&wparam(0)); # put aside pointer to argument block &lea ("edx",&wparam(1)); # load ap @@ -327,6 +329,7 @@ &mov ("esp",$_sp); # pull saved stack pointer &mov ("eax",1); +&set_label("just_leave"); &function_end("bn_mul_mont"); &asciz("Montgomery Multiplication for x86, CRYPTOGAMS by "); diff --git a/crypto/fipsmodule/bn/asm/x86_64-mont.pl b/crypto/fipsmodule/bn/asm/x86_64-mont.pl index ce89b17679..be4c69b55a 100755 --- a/crypto/fipsmodule/bn/asm/x86_64-mont.pl +++ b/crypto/fipsmodule/bn/asm/x86_64-mont.pl @@ -65,7 +65,7 @@ # output, so this isn't useful anyway. $addx = 1; -# void bn_mul_mont( +# int bn_mul_mont( $rp="%rdi"; # BN_ULONG *rp, $ap="%rsi"; # const BN_ULONG *ap, $bp="%rdx"; # const BN_ULONG *bp, diff --git a/crypto/fipsmodule/bn/internal.h b/crypto/fipsmodule/bn/internal.h index 3fbb7d7521..8e46914f82 100644 --- a/crypto/fipsmodule/bn/internal.h +++ b/crypto/fipsmodule/bn/internal.h @@ -165,23 +165,12 @@ typedef crypto_word_t BN_ULONG; #error "Must define either OPENSSL_32_BIT or OPENSSL_64_BIT" #endif - -// |num| must be at least 4, at least on x86. -// -// In other forks, |bn_mul_mont| returns an |int| indicating whether it -// actually did the multiplication. All our implementations always do the -// multiplication, and forcing callers to deal with the possibility of it -// failing just leads to further problems. -// -// In other forks, |bn_mod_mul|'s `num` argument has type |int| but it is -// implicitly treated as a |size_t|; when |int| is smaller than |size_t| -// then the |movq 48(%rsp),%r9| done by x86_64-xlate.pl implicitly does the -// conversion. +// See `bn_mul_mont_ffi` and `_MAX_LIMBS_ADDRESSES_MEMORY_SAFETY_ISSUES`. OPENSSL_STATIC_ASSERT(sizeof(int) == sizeof(size_t) || (sizeof(int) == 4 && sizeof(size_t) == 8), "int and size_t ABI mismatch"); -void bn_mul_mont(BN_ULONG *rp, const BN_ULONG *ap, const BN_ULONG *bp, - const BN_ULONG *np, const BN_ULONG *n0, size_t num); +int bn_mul_mont(BN_ULONG *rp, const BN_ULONG *ap, const BN_ULONG *bp, + const BN_ULONG *np, const BN_ULONG *n0, size_t num); static inline void bn_umult_lohi(BN_ULONG *low_out, BN_ULONG *high_out, BN_ULONG a, BN_ULONG b) { diff --git a/crypto/fipsmodule/ec/gfp_p256.c b/crypto/fipsmodule/ec/gfp_p256.c index d82938ed52..f060e64df4 100644 --- a/crypto/fipsmodule/ec/gfp_p256.c +++ b/crypto/fipsmodule/ec/gfp_p256.c @@ -39,7 +39,10 @@ static const BN_ULONG N_N0[] = { void p256_scalar_mul_mont(ScalarMont r, const ScalarMont a, const ScalarMont b) { /* XXX: Inefficient. TODO: optimize with dedicated multiplication routine. */ - bn_mul_mont(r, a, b, N, N_N0, P256_LIMBS); + debug_assert_nonsecret(P256_LIMBS >= 4); + int res = bn_mul_mont(r, a, b, N, N_N0, P256_LIMBS); + debug_assert_nonsecret(res == 1); + (void)res; } /* XXX: Inefficient. TODO: optimize with dedicated squaring routine. */ diff --git a/crypto/fipsmodule/ec/gfp_p384.c b/crypto/fipsmodule/ec/gfp_p384.c index 90065eaeb0..39d791b686 100644 --- a/crypto/fipsmodule/ec/gfp_p384.c +++ b/crypto/fipsmodule/ec/gfp_p384.c @@ -170,7 +170,10 @@ static void elem_div_by_2(Elem r, const Elem a) { static inline void elem_mul_mont(Elem r, const Elem a, const Elem b) { /* XXX: Not (clearly) constant-time; inefficient.*/ - bn_mul_mont(r, a, b, Q, Q_N0, P384_LIMBS); + debug_assert_nonsecret(P384_LIMBS >= 4); + int res = bn_mul_mont(r, a, b, Q, Q_N0, P384_LIMBS); + debug_assert_nonsecret(res == 1); + (void)res; } static inline void elem_mul_by_2(Elem r, const Elem a) { @@ -215,7 +218,10 @@ void p384_elem_neg(Elem r, const Elem a) { void p384_scalar_mul_mont(ScalarMont r, const ScalarMont a, const ScalarMont b) { /* XXX: Inefficient. TODO: Add dedicated multiplication routine. */ - bn_mul_mont(r, a, b, N, N_N0, P384_LIMBS); + debug_assert_nonsecret(P384_LIMBS >= 4); + int res = bn_mul_mont(r, a, b, N, N_N0, P384_LIMBS); + debug_assert_nonsecret(res == 1); + (void)res; } diff --git a/src/arithmetic/ffi.rs b/src/arithmetic/ffi.rs index b3566546d1..bf9549ab79 100644 --- a/src/arithmetic/ffi.rs +++ b/src/arithmetic/ffi.rs @@ -13,7 +13,7 @@ // CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. use super::{inout::AliasingSlices, n0::N0, LimbSliceError, MAX_LIMBS, MIN_LIMBS}; -use crate::{c, limb::Limb, polyfill::usize_from_u32}; +use crate::{c, error, limb::Limb, polyfill::usize_from_u32}; use core::mem::size_of; const _MAX_LIMBS_ADDRESSES_MEMORY_SAFETY_ISSUES: () = { @@ -41,7 +41,7 @@ macro_rules! bn_mul_mont_ffi { n: *const Limb, n0: &N0, len: c::size_t, - ); + ) -> crate::bssl::Result; } unsafe { crate::arithmetic::ffi::bn_mul_mont_ffi::<$Cpu, { $MIN_LEN }>( @@ -64,7 +64,7 @@ pub(super) unsafe fn bn_mul_mont_ffi( n: *const Limb, n0: &N0, len: c::size_t, - ), + ) -> crate::bssl::Result, ) -> Result<(), LimbSliceError> { /// The x86 implementation of `bn_mul_mont`, at least, requires at least 4 /// limbs. For a long time we have required 4 limbs for all targets, though @@ -81,12 +81,18 @@ pub(super) unsafe fn bn_mul_mont_ffi( return Err(LimbSliceError::too_long(n.len())); } + let len = n.len(); in_out .with_pointers(n.len(), |r, a, b| { - let len = n.len(); let n = n.as_ptr(); let _: Cpu = cpu; - unsafe { f(r, a, b, n, n0, len) }; + let result = unsafe { f(r, a, b, n, n0, len) }; + Result::from(result) }) - .map_err(LimbSliceError::len_mismatch) + .map_err(LimbSliceError::len_mismatch)? + .map_err( + #[cold] + #[inline(never)] + |_: error::Unspecified| LimbSliceError::too_short(len), + ) } diff --git a/src/arithmetic/montgomery.rs b/src/arithmetic/montgomery.rs index 6d9a834835..5f07689fe5 100644 --- a/src/arithmetic/montgomery.rs +++ b/src/arithmetic/montgomery.rs @@ -140,7 +140,7 @@ prefixed_export! { n: *const Limb, n0: &N0, num_limbs: c::size_t, - ) { + ) -> crate::bssl::Result { use super::MAX_LIMBS; // The mutable pointer `r` may alias `a` and/or `b`, so the lifetimes of @@ -159,6 +159,7 @@ prefixed_export! { } let r: &mut [Limb] = unsafe { core::slice::from_raw_parts_mut(r, num_limbs) }; limbs_from_mont_in_place(r, tmp, n, n0); + bssl::Result::ok() } } diff --git a/src/bssl.rs b/src/bssl.rs index 9d958b3b6c..8c8abb897a 100644 --- a/src/bssl.rs +++ b/src/bssl.rs @@ -21,6 +21,18 @@ use crate::{c, error}; #[repr(transparent)] pub struct Result(c::int); +impl Result { + #[cfg(not(any( + all(target_arch = "aarch64", target_endian = "little"), + all(target_arch = "arm", target_endian = "little"), + target_arch = "x86", + target_arch = "x86_64" + )))] + pub fn ok() -> Self { + Self(1) + } +} + impl From for core::result::Result<(), error::Unspecified> { fn from(ret: Result) -> Self { match ret.0 {