Skip to content
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

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
82d3f1e
Remove bignum_internal.h, moving contents to bignum_core.h
tom-cosgrove-arm Aug 23, 2022
71f4b0d
Add bignum_new.c starting with MPI_CORE(montmul) for Montgomery multi…
Aug 23, 2022
90c426b
Tidy up, removing MPI_CORE(), and using the new mbedtls_mpi_core_mla()
tom-cosgrove-arm Aug 23, 2022
7e655f7
Use new mbedtls_mpi_core_sub() instead of old static mpi_sub_hlp()
tom-cosgrove-arm Jul 20, 2022
268f96b
Fix Windows builds, which were getting "possible loss of data"
tom-cosgrove-arm Jul 31, 2022
0cc7865
Add unit tests for the new function mbedtls_mpi_core_add_if() in bign…
tom-cosgrove-arm Aug 23, 2022
2a65b85
Add unit tests for the new function mbedtls_mpi_core_sub() in bignum_…
tom-cosgrove-arm Aug 17, 2022
659c84a
Add unit tests for the new function mbedtls_mpi_core_mla() in bignum_…
tom-cosgrove-arm Aug 17, 2022
79b70f6
Make a public version of mpi_montg_init() in bignum_new.c and add uni…
tom-cosgrove-arm Aug 17, 2022
f334d96
Add unit tests for bignum_new.c:mbedtls_mpi_core_montmul()
tom-cosgrove-arm Aug 17, 2022
9384284
Use mbedtls_mpi_core_montmul() in mpi_montmul()
tom-cosgrove-arm Aug 5, 2022
40d2294
Tidy up doc comments on existing function mpi_montmul()
tom-cosgrove-arm Aug 17, 2022
4641ec6
Fix style following review comments
tom-cosgrove-arm Aug 17, 2022
f88b47e
Remove 'const' qualifier from temporary for mpi_montmul()
tom-cosgrove-arm Aug 17, 2022
2523791
Better constant-time properties for mbedtls_mpi_core_montmul()
tom-cosgrove-arm Aug 19, 2022
958fd3d
Remove bignum_new.c, moving contents to bignum_core.c
tom-cosgrove-arm Aug 24, 2022
f0ffb15
Have mbedtls_mpi_montg_init() take the modulus, rather than just its …
tom-cosgrove-arm Aug 24, 2022
7259463
Apply the function parameter naming convention
tom-cosgrove-arm Aug 24, 2022
b2c06f4
Remove stale comment, and fix whitespace issue
tom-cosgrove-arm Aug 24, 2022
ecbb124
Fix incorrect parameter name in mbedtls_mpi_core_add_if() doc comment
tom-cosgrove-arm Aug 25, 2022
d932de8
Remove incorrect constant-time claim from doc for mbedtls_mpi_core_ad…
tom-cosgrove-arm Aug 25, 2022
b496486
Reorder functions in bignum_core.[ch]
tom-cosgrove-arm Aug 30, 2022
5dd97e6
Update comments following code review
tom-cosgrove-arm Aug 30, 2022
f0c8a8c
One statement per line
tom-cosgrove-arm Aug 30, 2022
9354990
Don't use multiplication by condition in even a semi-constant time fu…
tom-cosgrove-arm Aug 30, 2022
ed43c6c
In add_if(), B MAY be aliased to A. Also update another comment for c…
tom-cosgrove-arm Aug 31, 2022
630110a
Fix documentation where ciL should be biL
tom-cosgrove-arm Aug 31, 2022
5eefc3d
Move macros to come before function declarations
tom-cosgrove-arm Aug 31, 2022
f0b2231
Update comments at the end of montmul following Gilles' feedback
tom-cosgrove-arm Aug 31, 2022
9339f05
Swap arguments of TEST_EQUAL() where it improves readability
tom-cosgrove-arm Sep 1, 2022
b0fb17a
Use ASSERT_COMPARE() instead of memcmp() in new tests
tom-cosgrove-arm Sep 1, 2022
1b2947a
Remove mbedtls_ prefix from bignum test cases
tom-cosgrove-arm Sep 2, 2022
eceb4cc
Rename variables and update comments in mpi_core_add_if test
tom-cosgrove-arm Sep 2, 2022
a043aeb
Rename variables and update comments in mpi_core_sub test
tom-cosgrove-arm Sep 2, 2022
42dfac6
Rename variables and update comments in mpi_core_mla test
tom-cosgrove-arm Sep 2, 2022
1135b20
Add mbedtls_mpi_core_add_if() tests for when inputs are aliased
tom-cosgrove-arm Sep 2, 2022
67c9247
Move the T++ in mbedtls_mpi_core_montmul() to within the loop body
tom-cosgrove-arm Sep 2, 2022
2b17792
Use ASSERT_ALLOC() in tests
tom-cosgrove-arm Sep 15, 2022
1feb5ac
Switch to using TEST_LE_S() and TEST_LE_U() in tests
tom-cosgrove-arm Sep 15, 2022
50c477b
Use S and sum (rather than X/expected) in mpi_core_add_if()
tom-cosgrove-arm Sep 15, 2022
be7209d
Remove unnecessary casts
tom-cosgrove-arm Sep 15, 2022
e2159f2
Use the MAX() macro
tom-cosgrove-arm Sep 15, 2022
359feb0
Better wording for the reason why we use an input MPI for a scalar value
tom-cosgrove-arm Sep 15, 2022
818d992
Note that T must not overlap other parameters of mbedtls_mpi_core_mon…
tom-cosgrove-arm Sep 15, 2022
2701dea
Use mbedtls_ct_mpi_uint_mask() rather than rolling our own
tom-cosgrove-arm Sep 15, 2022
b7438d1
Update name of mbedtls_mpi_montg_init()
tom-cosgrove-arm Sep 15, 2022
17f1fdc
Update comments in mpi_core_add_if() test
tom-cosgrove-arm Sep 15, 2022
dbc1561
Don't bother to test b + a after testing a + b if a == b
tom-cosgrove-arm Sep 15, 2022
c71ca0c
Remove some unnecessary whitespace (two spaces after commas)
tom-cosgrove-arm Sep 15, 2022
5c0e810
Prefer 'fixed-size' to 'known-size' in doc comments
tom-cosgrove-arm Sep 15, 2022
3bd7bc3
Use X rather than A for accumulator-style input (and output!) params,…
tom-cosgrove-arm Sep 15, 2022
f2b3818
Test when all three inputs to mbedtls_mpi_core_sub() are aliased
tom-cosgrove-arm Sep 20, 2022
ea45c1d
Document and test aliasing of output for mbedtls_mpi_core_montmul()
tom-cosgrove-arm Sep 20, 2022
b0b77e1
Document and test aliasing of the bignums given to mbedtls_mpi_core_m…
tom-cosgrove-arm Sep 20, 2022
4782823
Ensure we explicitly document the modulus for fixed-width arithmetic
tom-cosgrove-arm Sep 20, 2022
c573882
Merge remote-tracking branch 'upstream/development' into issue-6015-m…
tom-cosgrove-arm Sep 21, 2022
119eae2
Update names of test cases in generate_bignum_tests.py
tom-cosgrove-arm Sep 21, 2022
4386ead
Correct the aliasing requirements in doc for mbedtls_mpi_core_montmul…
tom-cosgrove-arm Sep 29, 2022
6da3a3b
Fix doc regarding aliasing of modulus input to mbedtls_mpi_core_montm…
tom-cosgrove-arm Sep 29, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions library/bignum_core.h
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,14 @@ mbedtls_mpi_uint mbedtls_mpi_core_montmul_init( const mbedtls_mpi_uint *N );
/**
* \brief Montgomery multiplication: X = A * B * R^-1 mod N (HAC 14.36)
*
* \p X may be aliased to \p A or \p N, or even \p B (if \p AN_limbs ==
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably

Suggested change
* \p X may be aliased to \p A or \p N, or even \p B (if \p AN_limbs ==
* \p X may be aliased to \p A, or even \p B (if \p AN_limbs ==

The documentation of N states that it may not alias X. Our general rule is that outputs may not alias moduli.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the function does actually work if X aliases N - there are already test cases for that. So in this case the documentation for N is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the documentation should state what we are promising. The implementation might actually do more than that, but that shouldn't be a problem.

In this case, I think it makes sense not to promise that X can alias N as it doesn't make much sense. (eg. in the modulus structure, the value of the modulus is const. Also, the post condition on X is that it is less than N, which in the case of aliasing is impossible in a tight interpretation and still confusing when we try to interpret it well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For an output parameter to alias to an input parameter means the storage of the input parameter can be overwritten to be used for the output - the input value doesn't matter (whereas for two input parameters to alias, their values must be identical).

As long as core is just used internally, I agree with you, that it's unlikely that we will want to alias X and N. However, there's nothing to stop a library user calling these functions, and they may have their own reasons to minimise memory use and overwrite N with the output value, so I see no harm in documenting that this is safe (and, of course, testing it!).

Given that a temporary working area is supplied to this function, this kind of "over promise" (beyond what we need and currently expect) is unlikely to constrain any future implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's nothing to stop a library user calling these functions

Yes, there is: it's an internal function. Only the legacy bignum API is public and must obey its promises until the next major version change.

But because it's an internal function, we can make high-reaching promises if we want: we can rescind them at any time, we just need to check that it doesn't break our own code.

* \p B_limbs) but may not overlap any parameters otherwise.
*
* \p A, \p B and \p N must not alias or overlap each other in any way, even
* if \p AN_limbs == \p B_limbs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a strange requirement: why would it be a problem that an input overlaps an input? (Unless we plan to add restrict annotations in the future, but we currently have no such plans.)

Here I'd expect A == B to make sense. A == N or B == N wouldn't make sense since A or B would be out of range.

Note: if we allow A == B, we need to test the case where R == A && R == B. (Yes, I've had bugs, at least in my working copy, where A=f(A,B) and B=f(A,B) worked but not A=f(A,A).)

We could state that X == N is forbidden, because in practice a value that's used as the modulus tends to stick around during the whole high-level computation. Even if it doesn't help the implementation, it would help to reduce the testing burden (especially if we allow inputs to overlap, because then it reduces the overlap possibilities from 8 to 4).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, you are right - I must have misread the code when determining the restrictions.

I've updated the text, and added a test for A aliased to B and X (which covers A and B only - it doesn't seem worth having yet another test for that)

*
* \p A and \p B must be in canonical form: that is, <= \p N.
*
* \param[out] X The destination MPI, as a little-endian array of
gilles-peskine-arm marked this conversation as resolved.
Show resolved Hide resolved
* length \p AN_limbs.
* On successful completion, X contains the result of
Expand Down
27 changes: 27 additions & 0 deletions tests/suites/test_suite_mpi.function
Original file line number Diff line number Diff line change
Expand Up @@ -2114,6 +2114,33 @@ void mpi_core_montmul( int limbs_AN4, int limbs_B4,
size_t bytes = N.n * sizeof(mbedtls_mpi_uint);
ASSERT_COMPARE( R.p, bytes, X->p, bytes );

gilles-peskine-arm marked this conversation as resolved.
Show resolved Hide resolved
/* The output (R, above) may be aliased to A - use R to save the value of A */

memcpy( R.p, A.p, bytes );

mbedtls_mpi_core_montmul( A.p, A.p, B.p, B.n, N.p, N.n, mm, T.p );
ASSERT_COMPARE( A.p, bytes, X->p, bytes );

memcpy( A.p, R.p, bytes ); /* restore A */

/* The output may be aliased to N - use R to save the value of N */

memcpy( R.p, N.p, bytes );

mbedtls_mpi_core_montmul( N.p, A.p, B.p, B.n, N.p, N.n, mm, T.p );
ASSERT_COMPARE( N.p, bytes, X->p, bytes );

memcpy( N.p, R.p, bytes );

/* The output may even be aliased to B, if AN_limbs == B_limbs */

if (limbs_AN == limbs_B)
{
/* Note: last test, so we don't save B */
mbedtls_mpi_core_montmul( B.p, A.p, B.p, B.n, N.p, N.n, mm, T.p );
ASSERT_COMPARE( B.p, bytes, X->p, bytes );
}

exit:
mbedtls_mpi_free( &A );
mbedtls_mpi_free( &B );
Expand Down