-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bignum: Montgomery multiplication from bignum prototype #6083
Changes from 20 commits
82d3f1e
71f4b0d
90c426b
7e655f7
268f96b
0cc7865
2a65b85
659c84a
79b70f6
f334d96
9384284
40d2294
4641ec6
f88b47e
2523791
958fd3d
f0ffb15
7259463
b2c06f4
ecbb124
d932de8
b496486
5dd97e6
f0c8a8c
9354990
ed43c6c
630110a
5eefc3d
f0b2231
9339f05
b0fb17a
1b2947a
eceb4cc
a043aeb
42dfac6
1135b20
67c9247
2b17792
1feb5ac
50c477b
be7209d
e2159f2
359feb0
818d992
2701dea
b7438d1
17f1fdc
dbc1561
c71ca0c
5c0e810
3bd7bc3
f2b3818
ea45c1d
b0b77e1
4782823
c573882
119eae2
4386ead
6da3a3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,8 @@ | |
#endif | ||
|
||
#include "bignum_core.h" | ||
#include "bn_mul.h" | ||
#include "constant_time_internal.h" | ||
|
||
size_t mbedtls_mpi_core_clz( mbedtls_mpi_uint a ) | ||
{ | ||
|
@@ -291,4 +293,139 @@ int mbedtls_mpi_core_write_be( const mbedtls_mpi_uint *X, | |
return( 0 ); | ||
} | ||
|
||
void mbedtls_mpi_core_montmul( mbedtls_mpi_uint *X, | ||
gilles-peskine-arm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const mbedtls_mpi_uint *A, | ||
const mbedtls_mpi_uint *B, | ||
size_t B_limbs, | ||
const mbedtls_mpi_uint *N, | ||
size_t AN_limbs, | ||
mbedtls_mpi_uint mm, | ||
mbedtls_mpi_uint *T ) | ||
{ | ||
memset( T, 0, ( 2 * AN_limbs + 1 ) * ciL ); | ||
minosgalanakis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
for( size_t i = 0; i < AN_limbs; i++, T++ ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Subjective: I would find the code easier to understand if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me That said, we could make this clearer by doing it at the end of the loop block and not in the loop header. And possibly we could add a comment to remind to the arithmetic meaning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with Janos. I find the division each iteration more intuitive. I've left this as-is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how I don't consider this a blocker. But please at least move |
||
{ | ||
mbedtls_mpi_uint u0, u1; | ||
/* T = (T + u0*B + u1*N) / 2^biL */ | ||
u0 = A[i]; | ||
u1 = ( T[0] + u0 * B[0] ) * mm; | ||
|
||
(void) mbedtls_mpi_core_mla( T, AN_limbs + 2, B, B_limbs, u0 ); | ||
(void) mbedtls_mpi_core_mla( T, AN_limbs + 2, N, AN_limbs, u1 ); | ||
} | ||
|
||
/* It's possible that the result in T is > N, and so we might need to subtract N */ | ||
|
||
mbedtls_mpi_uint carry = T[AN_limbs]; | ||
gilles-peskine-arm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
mbedtls_mpi_uint borrow = mbedtls_mpi_core_sub( X, T, N, AN_limbs ); | ||
|
||
/* | ||
* Both carry and borrow can only be 0 or 1. | ||
gilles-peskine-arm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* If carry = 1, the result in T must be > N by definition, and the subtraction | ||
* using only AN_limbs limbs will create borrow, but that will have the correct | ||
* final result. | ||
* | ||
* i.e. (carry, borrow) of (1, 1) => return X | ||
* | ||
* If carry = 0, then we want to use the result of the subtraction iff | ||
minosgalanakis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* borrow = 0. | ||
* | ||
* i.e. (carry, borrow) of (0, 0) => return X | ||
* (0, 1) => return T | ||
* | ||
* We've confirmed that the unit tests exercise this function with all 3 of | ||
gilles-peskine-arm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* the valid (carry, borrow) combinations (listed above), and that we don't | ||
* see (carry, borrow) = (1, 0). | ||
* | ||
* So the correct return value is already in X if (carry ^ borrow) = 0, | ||
* but is in (the lower AN_limbs limbs of) T if (carry ^ borrow) = 1. | ||
*/ | ||
mbedtls_ct_mpi_uint_cond_assign( AN_limbs, X, T, (unsigned char) ( carry ^ borrow ) ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this was initially an This function is the most performance critical routine throughout ECC -- any change has to be considered very carefully, not only from a security, but also from a performance perspective. Has it been analyzed what using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original version used With There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, your call! |
||
} | ||
|
||
/* | ||
* Fast Montgomery initialization (thanks to Tom St Denis). | ||
*/ | ||
mbedtls_mpi_uint mbedtls_mpi_montg_init( const mbedtls_mpi_uint *N ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why take a pointer as an argument and not the limb value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because this function calculates N^-1 mod 2^biL and technically we only need the last limb of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As Janos says, this is a function calculating a value based on the MPI N. The fact it only needs the least significant limb (LSL) of N is incidental. It is premature optimisation for the caller to extract the LSL - it is likely that this function will be inlined eventually. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It certainly doesn't. When I see this function I wonder why we're passing a pointer to a read-only scalar. It looks like it's a a pointer to an array, and seeing that it's a pointer to bignum limbs it sure like it's part of an array, but then I'd expect a size. It's not a blocker, but I do feel the code is gratuitously hard to read here. |
||
{ | ||
mbedtls_mpi_uint x = N[0]; | ||
|
||
x += ( ( N[0] + 2 ) & 4 ) << 1; | ||
|
||
for( unsigned int i = biL; i >= 8; i /= 2 ) | ||
x *= ( 2 - ( N[0] * x ) ); | ||
|
||
return( ~x + 1 ); | ||
} | ||
|
||
mbedtls_mpi_uint mbedtls_mpi_core_mla( mbedtls_mpi_uint *d, size_t d_len, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't updated the parameter names here to match those in the header because they are directly used by the |
||
const mbedtls_mpi_uint *s, size_t s_len, | ||
mbedtls_mpi_uint b ) | ||
{ | ||
mbedtls_mpi_uint c = 0; /* carry */ | ||
if( d_len < s_len ) | ||
gilles-peskine-arm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
s_len = d_len; | ||
gilles-peskine-arm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
size_t excess_len = d_len - s_len; | ||
size_t steps_x8 = s_len / 8; | ||
size_t steps_x1 = s_len & 7; | ||
|
||
while( steps_x8-- ) | ||
{ | ||
MULADDC_X8_INIT | ||
MULADDC_X8_CORE | ||
MULADDC_X8_STOP | ||
} | ||
|
||
while( steps_x1-- ) | ||
{ | ||
MULADDC_X1_INIT | ||
MULADDC_X1_CORE | ||
MULADDC_X1_STOP | ||
} | ||
|
||
while( excess_len-- ) | ||
{ | ||
*d += c; c = ( *d < c ); d++; | ||
} | ||
|
||
return( c ); | ||
} | ||
|
||
mbedtls_mpi_uint mbedtls_mpi_core_sub( mbedtls_mpi_uint *X, | ||
const mbedtls_mpi_uint *A, | ||
const mbedtls_mpi_uint *B, | ||
size_t limbs ) | ||
{ | ||
mbedtls_mpi_uint c = 0; | ||
|
||
for( size_t i = 0; i < limbs; i++ ) | ||
{ | ||
mbedtls_mpi_uint z = ( A[i] < c ); | ||
mbedtls_mpi_uint t = A[i] - c; | ||
c = ( t < B[i] ) + z; | ||
X[i] = t - B[i]; | ||
} | ||
|
||
return( c ); | ||
} | ||
|
||
mbedtls_mpi_uint mbedtls_mpi_core_add_if( mbedtls_mpi_uint *A, | ||
const mbedtls_mpi_uint *B, | ||
size_t limbs, | ||
unsigned cond ) | ||
{ | ||
mbedtls_mpi_uint c = 0, t; | ||
for( size_t i = 0; i < limbs; i++ ) | ||
minosgalanakis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
mbedtls_mpi_uint add = cond * B[i]; | ||
gilles-peskine-arm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
t = c; | ||
t += A[i]; c = ( t < A[i] ); | ||
gilles-peskine-arm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
t += add; c += ( t < add ); | ||
A[i] = t; | ||
} | ||
return( c ); | ||
} | ||
|
||
#endif /* MBEDTLS_BIGNUM_C */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not go one step further and elimitate this trivial auxiliary function, as was done for
mpi_sub_hlp
? Likewise formpi_montmul
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my understanding RR and mm will be pre-calcuated during mbedtls_mod_modulus_setup() phase.
There is also the consideration of adjusting them to a more efficient computionally logic. So it makes it simpler to keep them as small single purpose service functions, which can be absorbed later on when the new interface is the standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very specifically, for this PR to have minimal, obvious, easily-reviewable changes. The legacy bignum interface will be retired at some point, and then these will just vanish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is so trivial, mathematically it is quite involved. I would prefer to have it in a separate function and document it better in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean to eliminate
mbedtls_mpi_montg_init
, which is where the mathematics happen. I meant to eliminate this function,mpi_montg_init
, and callmbedtls_mpi_montg_init
directly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said above, my goal was for this PR to have minimal, obvious, easily-reviewable changes. Making the existing function marshal its arguments and call the new function is (to me) the minimal, obvious, way to do this.
Since
mpi_montg_init()
is static, the compiler inlines it (at least on-O2
), and there is no code-size impact.And in the future, all of this is going away.
So I don't see the benefit of changing the caller of
mbedtls_mpi_montg_init()