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: Add Montgomery conversion and high level I/O #6017

Closed
6 of 7 tasks
yanesca opened this issue Jul 4, 2022 · 6 comments · Fixed by #6407, #6550 or #6619
Closed
6 of 7 tasks

Bignum: Add Montgomery conversion and high level I/O #6017

yanesca opened this issue Jul 4, 2022 · 6 comments · Fixed by #6407, #6550 or #6619
Assignees
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-s Estimated task size: small (~2d)

Comments

@yanesca
Copy link
Contributor

yanesca commented Jul 4, 2022

Prerequisites: #6015, #6016

Add Montgomery conversion and high level I/O.

Add missing fields to the mbedtls_mpi_mont_struct and update setup and free to initialise and free these fields:
https://github.com/hanno-arm/mbedtls/blob/ecp_prototype/library/bignum_core.h#L86-L87
Extract functions required to do this from the prototype.

Extract low level conversion functions from the prototype:
https://github.com/hanno-arm/mbedtls/blob/ecp_prototype/library/bignum_core.c#L963-L978
Adapt naming conventions and add them to bignum_mod_raw.h.

Add I/O functions for mbedtls_mod_residue that take a modulus as a parameter and automatically converts to internal representation as needed based on int_rep field of the modulus.

The task includes making the legacy Bignum functions call the extracted functions where the functionality is duplicated. This is necessary to minimise cost in code size.

Whenever extracting functions from the prototype, there should be separate commits containing the code from the prototype, with the absolute minimum of modifications that make the library compile. (It is Ok if these functions are not called at all at this point.) These commits should have Hanno as the author (git commit --author="Hanno Becker [email protected]")

The prototype is too macro heavy. Most new macros should be expanded/removed or replaced with static functions if possible. (Pre-existing macros should not be touched.) The macro MPI_CORE should expand to mbedtls_mpi_core_ ## func ## instead of mbedtls_mpi_core_ ## func ## _minimal.

All new function implementations should go into bignum_new.c.

This task is done when the following changes are merged on development:

  • Missing fields are added to to the mbedtls_mpi_mont_struct
  • Setup and free functions are updated to allocate, initialise and free these fields
  • Functions for doing these calculations are extracted from the prototype
  • New I/O functions are added for residue
  • New macros are expanded/removed
  • The legacy functions in bignum.c are calling the new functions in library/bignum_core.h wherever possible
  • Extensive unit tests are added for all new functions
@yanesca yanesca added enhancement component-crypto Crypto primitives and low-level interfaces size-m Estimated task size: medium (~1w) needs-info An issue or PR which needs further info from the reporter / author labels Jul 4, 2022
@yanesca yanesca changed the title Add modular reduction and proper I/O Add Montgomery conversion and high level I/O Jul 8, 2022
@yanesca yanesca added size-s Estimated task size: small (~2d) and removed size-m Estimated task size: medium (~1w) labels Jul 8, 2022
@yanesca yanesca removed the needs-info An issue or PR which needs further info from the reporter / author label Jul 8, 2022
@minosgalanakis minosgalanakis self-assigned this Aug 2, 2022
@minosgalanakis
Copy link
Contributor

PR 6083 and PR 6095 have been succeffully test-merged with conflicts resovles on 6017_add_Montgomery_conversion_high_lv_IO branch.

Currently the code compiles, and all implemented tests are passing

@tom-cosgrove-arm
Copy link
Contributor

That's great! Note that with the merging of #6070 remove radix from test data I will be rebasing my PR on top of development, so you'll need to re-do the rebase (sorry)

@minosgalanakis
Copy link
Contributor

That is as expected (Also the reason this is for now a branch and not a PR).

This step was neccessary to have a common starting point for implemting the Montgomery conversion and to also ensure there are no dragons when the other two tasks are merged.

@minosgalanakis minosgalanakis linked a pull request Aug 18, 2022 that will close this issue
2 tasks
@minosgalanakis
Copy link
Contributor

Testing Design

Following the sync design meeting the following items have been agreed with regards to testing for this pr:

For testing we need to test the following methods:

The following methods may need new tests to warranty the succesfull allocation/deallocation/zeorisation of memory:

For testing we need to test for:

  • 4 and 8 limb sizes, prefferably in different testing routes.
  • RR is matching precalculated values.
  • That methods fail with invalid input.
  • That conversion methods from cannonical to montgomery and montgomery to cannonical are working, by comparing precalculated values with the output.
  • That consecutive calls for conversion to and from one form will give consistent outputs.
  • Max allowed size is size of mod - 1. We need to test edge cases, and negative testing that if size == mod it will fail.
  • Verify that memory is zerod and not accessible during setup - clean cycle.

Because of the high level nature of this PR, most of the math are testing during the Multiplication depedency, and is assumed to be trustworthy.

@yanesca
Copy link
Contributor Author

yanesca commented Oct 31, 2022

Reopening as the PR that automatically closed this is only the first in a series of PRs implementing this.

@yanesca yanesca reopened this Oct 31, 2022
@minosgalanakis minosgalanakis linked a pull request Nov 8, 2022 that will close this issue
3 tasks
@yanesca
Copy link
Contributor Author

yanesca commented Nov 14, 2022

Still not done, one more PR to go.

@yanesca yanesca reopened this Nov 14, 2022
@minosgalanakis minosgalanakis linked a pull request Nov 17, 2022 that will close this issue
3 tasks
@tom-cosgrove-arm tom-cosgrove-arm changed the title Add Montgomery conversion and high level I/O Bignum: Add Montgomery conversion and high level I/O Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment