Skip to content

Commit

Permalink
arithmetic internals: Further clarify bn_mul_mont ABI.
Browse files Browse the repository at this point in the history
The checks are superceded by the checks in the Rust code, so
replace the checks with documenting the preconditions so we
don't need to worry about the functions failing anymore.

Document some additional caveats discovered.
  • Loading branch information
briansmith committed Jan 20, 2025
1 parent b794f56 commit 888c403
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 26 deletions.
13 changes: 4 additions & 9 deletions crypto/fipsmodule/bn/asm/armv4-mont.pl
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,9 @@
.align 4
.Lialu:
#endif
cmp ip,#2
@ No return value in *ring*. Instead, the caller must ensure num >= 2
mov $num,ip @ load num
#ifdef __thumb2__
ittt lt
#endif
movlt r0,#0
addlt sp,sp,#2*4
blt .Labrt
@ No return value in *ring*

stmdb sp!,{r4-r12,lr} @ save 10 registers

Expand Down Expand Up @@ -285,8 +280,7 @@
add sp,sp,#4 @ skip over tp[num+1]
ldmia sp!,{r4-r12,lr} @ restore registers
add sp,sp,#2*4 @ skip over {r0,r2}
mov r0,#1
.Labrt:
@ No return value in *ring*
#if __ARM_ARCH>=5
ret @ bx lr
#else
Expand Down Expand Up @@ -739,6 +733,7 @@
mov sp,ip
vldmia sp!,{d8-d15}
ldmia sp!,{r4-r11}
@ No return value in *ring*
ret @ bx lr
.size bn_mul8x_mont_neon,.-bn_mul8x_mont_neon
#endif
Expand Down
8 changes: 4 additions & 4 deletions crypto/fipsmodule/bn/asm/armv8-mont.pl
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
$lo1,$hi1,$nj,$m1,$nlo,$nhi,
$ovf, $i,$j,$tp,$tj) = map("x$_",6..17,19..24);

# int bn_mul_mont(
# void bn_mul_mont(
$rp="x0"; # BN_ULONG *rp,
$ap="x1"; # const BN_ULONG *ap,
$bp="x2"; # const BN_ULONG *bp,
Expand Down Expand Up @@ -267,7 +267,7 @@
ldp x19,x20,[x29,#16]
mov sp,x29
ldp x21,x22,[x29,#32]
mov x0,#1
// No return value in *ring*.
ldp x23,x24,[x29,#48]
ldr x29,[sp],#64
AARCH64_VALIDATE_LINK_REGISTER
Expand Down Expand Up @@ -1041,7 +1041,7 @@
ldp x19,x20,[x29,#16]
mov sp,x29
ldp x21,x22,[x29,#32]
mov x0,#1
// No return value in *ring*.
ldp x23,x24,[x29,#48]
ldp x25,x26,[x29,#64]
ldp x27,x28,[x29,#80]
Expand Down Expand Up @@ -1502,7 +1502,7 @@
ldp x19,x20,[x29,#16]
mov sp,x29
ldp x21,x22,[x29,#32]
mov x0,#1
// No return value in *ring*.
ldp x23,x24,[x29,#48]
ldp x25,x26,[x29,#64]
ldp x27,x28,[x29,#80]
Expand Down
4 changes: 3 additions & 1 deletion crypto/fipsmodule/bn/asm/x86-mont.pl
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,9 @@
$_bpend=&DWP(4*7,"esp");
$frame=32; # size of above frame rounded up to 16n

&xor ("eax","eax");
# No return value in *ring*. Instead, the caller must ensure num >= 4
&mov ("edi",&wparam(5)); # int num
# No return value in *ring*.

&lea ("esi",&wparam(0)); # put aside pointer to argument block
&lea ("edx",&wparam(1)); # load ap
Expand Down Expand Up @@ -327,6 +328,7 @@

&mov ("esp",$_sp); # pull saved stack pointer
&mov ("eax",1);
# No return value in *ring*.
&function_end("bn_mul_mont");

&asciz("Montgomery Multiplication for x86, CRYPTOGAMS by <appro\@openssl.org>");
Expand Down
8 changes: 4 additions & 4 deletions crypto/fipsmodule/bn/asm/x86_64-mont.pl
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@

mov 8(%rsp,$num,8),%rsi # restore %rsp
.cfi_def_cfa %rsi,8
mov \$1,%rax
# No return value in *ring*
mov -48(%rsi),%r15
.cfi_restore %r15
mov -40(%rsi),%r14
Expand Down Expand Up @@ -783,7 +783,7 @@
$code.=<<___;
mov 8(%rsp,$num,8),%rsi # restore %rsp
.cfi_def_cfa %rsi, 8
mov \$1,%rax
# No return value in *ring*
mov -48(%rsi),%r15
.cfi_restore %r15
mov -40(%rsi),%r14
Expand Down Expand Up @@ -998,7 +998,7 @@
add \$32,$num
jnz .Lsqr8x_cond_copy

mov \$1,%rax
# No return value in *ring*
mov -48(%rsi),%r15
.cfi_restore %r15
mov -40(%rsi),%r14
Expand Down Expand Up @@ -1366,7 +1366,7 @@

mov %rdx,($tptr)

mov \$1,%rax
# No return value in *ring*.
mov -48(%rsi),%r15
.cfi_restore %r15
mov -40(%rsi),%r14
Expand Down
9 changes: 1 addition & 8 deletions src/arithmetic/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
// CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

use super::{inout::AliasingSlices, n0::N0, LimbSliceError, MAX_LIMBS, MIN_LIMBS};
use super::{inout::AliasingSlices, n0::N0, LimbSliceError, MAX_LIMBS};
use crate::{c, limb::Limb, polyfill::usize_from_u32};
use core::mem::size_of;

Expand All @@ -33,7 +33,6 @@ macro_rules! bn_mul_mont_ffi {
use crate::{c, limb::Limb};
prefixed_extern! {
// `r` and/or 'a' and/or 'b' may alias.
// XXX: BoringSSL declares these functions to return `int`.
fn $f(
r: *mut Limb,
a: *const Limb,
Expand Down Expand Up @@ -66,12 +65,6 @@ pub(super) unsafe fn bn_mul_mont_ffi<Cpu, const MIN_LEN: usize>(
len: c::size_t,
),
) -> 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
/// this may be unnecessary.
const _MIN_LIMBS_AT_LEAST_4: () = assert!(MIN_LIMBS >= 4);
// We haven't tested shorter lengths.
assert!(MIN_LEN >= MIN_LIMBS);
if n.len() < MIN_LEN {
return Err(LimbSliceError::too_short(n.len()));
}
Expand Down
18 changes: 18 additions & 0 deletions src/arithmetic/montgomery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,24 @@ pub(super) fn limbs_mul_mont(
n0: &N0,
cpu: cpu::Features,
) -> Result<(), LimbSliceError> {
// WARNING: In OpenSSL/BoringSSL, `bn_mul_mont` returns `int`, but in this
// codebase they don't return a result.
//
// WARNING: In BoringSSL, at least the 32-bit ARM implementations don't
// actually return 1 or 0, but non-zero or zero, despite how they are
// documented in BoringSSL.
//
// The implementations of `bn_mul_mont` for the implementations in the
// *ring* tree are ABI-compatible with both. However, in some ABIs (for
// other architectures), the ABI for `void` functions and `int`-returning
// functions is significantly different. So if/when importing in, or
// otherwise linking to, other implementations of `bn_mul_mont`.

// Required for 32-bit arm.
const _32_BIT_ARM_REQUIRES_MIN_LIMBS_AT_LEAST_2: () = assert!(MIN_LIMBS >= 2);
// We haven't tested other implementations with less than 4 limbs.
const _32_BIT_X86_REQUIRES_MIN_LIMBS_AT_LEAST_4: () = assert!(MIN_LIMBS >= 4);

bn_mul_mont_ffi!(in_out, n, n0, cpu, unsafe {
(MIN_LIMBS, cpu::Features) => bn_mul_mont
})
Expand Down

0 comments on commit 888c403

Please sign in to comment.