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.

Sign up for GitHub

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

Jump to bottom

Bignum: Extract mod exp from prototype #6547

Merged
merged 20 commits into from
Nov 29, 2022
Merged

Bignum: Extract mod exp from prototype #6547

merged 20 commits into from
Nov 29, 2022

Conversation

yanesca
Copy link
Contributor

@yanesca yanesca commented Nov 7, 2022 •
edited
Loading

Description

The second PR of a series related to #6293 .

Gatekeeper checklist

  • changelog not required
  • backport not required
  • tests provided

@yanesca yanesca added enhancement needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Nov 7, 2022
@tom-cosgrove-arm tom-cosgrove-arm changed the title Extract mod exp from prototype Bignum: Extract mod exp from prototype Nov 8, 2022
@yanesca yanesca added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Nov 8, 2022
@yanesca
Copy link
Contributor Author

yanesca commented Nov 10, 2022

Internal CI has passed and OpenCI fails on an unrelated issue.

@yanesca yanesca removed the needs-ci Needs to pass CI tests label Nov 10, 2022
@yanesca
Copy link
Contributor Author

yanesca commented Nov 10, 2022

I am going to rework the test generation so that it will be easier to adapt when the improvements from #6577 and #6576 are merged.

That said, I would not like to wait for these PRs: these are not blockers and want to merge this PR independently of those two.

@yanesca
Copy link
Contributor Author

yanesca commented Nov 10, 2022

Before that I would like to clean up the history a bit. Force pushing with the cleaner history and a minor fix.

@yanesca yanesca force-pushed the extract_mod_exp_from_prototype branch from 4d1c45a to acde91c Compare November 10, 2022 13:50
@yanesca
Copy link
Contributor Author

yanesca commented Nov 11, 2022

Internal CI has passed, Open CI reported and error, but there is no error on the corresponding CI job.

@yanesca yanesca added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-work labels Nov 11, 2022
@tom-cosgrove-arm tom-cosgrove-arm self-requested a review November 11, 2022 14:38
@yanesca
Copy link
Contributor Author

yanesca commented Nov 14, 2022

@tom-cosgrove-arm I have applied the changes you suggested and the PR is ready for review again.

@tom-cosgrove-arm tom-cosgrove-arm added the needs-ci Needs to pass CI tests label Nov 14, 2022
yanesca and others added 14 commits November 22, 2022 21:22
@yanesca
mpi_core_exp_mod: add unit tests
0f0d1e8 
The test cases aim to mirror the legacy function, but needed the some
cases to be removed because:

- Null representation is not valid in core
- There are no negative numbers in core
- Bignum core doesn't do parameter checking and there are no promises for
  even N

The _size variant of the test has been removed as bignum core doesn't do
parameter checking and there is no promises for inputs that are larger
than MBEDTLS_MPI_MAX_SIZE.

Signed-off-by: Janos Follath <janos.follath@arm.com>
@yanesca
core_exp_mod: improve window selection
a77911e 
We are looking at the exponent at limb granularity and therefore
exponent bits can't go below 32.

The `mpi_` prefix is also removed as it is better not to have prefix at
all than to have just a partial. (Full prefix would be overly long and
would hurt readability.)

Signed-off-by: Janos Follath <janos.follath@arm.com>
@yanesca
mpi_core_mod_exp: improve style and documentation
0ec6e3f 
No intended change in behaviour.

Signed-off-by: Janos Follath <janos.follath@arm.com>
@gilles-peskine-arm @yanesca
More consistent variable names
07f2c69 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm @yanesca
Change E closer to where it's used
7af166b 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm @yanesca
Define variables closer to their use
cf979b0 
Make variables const where possible.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm @yanesca
Move window precomputation into an auxiliary function
0de0a04 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm @yanesca
Local readability improvements
d83b5cb 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm @yanesca
Simplify exponent bit selection
c718a3c 
Use indices instead of mutating data to extract the bits of the exponent.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm @yanesca
Make the main loop's logic clearer
3b63d09 
The loop ends when there are no more bits to process, with one twist: when
that happens, we need to clear the window one last time. Since the window
does not start empty (E_limbs==0 is not supported), the loop always starts
with a non-empty window and some bits to process. So it's correct to move
the window clearing logic to the end of the loop. This lets us exit the loop
when the end of the exponent is reached.

It would be clearer not to do the final window clearing inside the loop, so
we wouldn't need to repeat the loop termination condition (end of exponent
reached) inside the loop. However, this requires duplicating the code to
clear the window. Empirically, this causes a significant code size increase,
even if the window clearing code is placed into a function.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm @yanesca
Simplify cleanup logic
4380d7b 
Take advantage of the fact that there's a single point of failure.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm @yanesca
Explain a little more
0b270a5 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm @yanesca
Zeroize sensitive data
7d89d35 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@yanesca
mpi_exp_mod: improve documentation
3321b58 
Signed-off-by: Janos Follath <janos.follath@arm.com>
@yanesca yanesca force-pushed the extract_mod_exp_from_prototype branch from 756bc83 to 3321b58 Compare November 22, 2022 21:29
@yanesca
Copy link
Contributor Author

yanesca commented Nov 22, 2022

Rebased to current development head.

@yanesca
mpi_core_exp_mod: add generated tests
05867cb 
Signed-off-by: Janos Follath <janos.follath@arm.com>
@yanesca
mpi_core_exp_mod: remove manual tests
cf0e86c 
The previous commit added generated tests, we don't need the manually
added tests anymore.

Signed-off-by: Janos Follath <janos.follath@arm.com>
@yanesca
Make pylint happy
43d3de4 
Signed-off-by: Janos Follath <janos.follath@arm.com>
@yanesca
Copy link
Contributor Author

yanesca commented Nov 22, 2022

Did the rebase in two steps: the first removes automated tests and should be easy to review and the second is a trivial rebase on top of current development head.

The first new commit is 07f2c69.

gilles-peskine-arm
gilles-peskine-arm reviewed Nov 25, 2022
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

I'm happy with the library code as of 43d3de4. I still haven't reviewed the tests.

@yanesca
Copy link
Contributor Author

yanesca commented Nov 29, 2022

The CI run 3 days ago last time and it passed. The recent changes on development didn't impact this part of the code. Re-running the CI just to be sure.

The generated tests are up to date with the latest changes in the framework.

No rebase is necessary, the PR is ready for review as it is.

tom-cosgrove-arm
tom-cosgrove-arm approved these changes Nov 29, 2022
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

library/bignum_core.c
* (limb_index=E_limbs-1, E_bit_index=biL-1) to least significant
* (limb_index=0, E_bit_index=0). */
size_t E_limb_index = E_limbs;
size_t E_bit_index = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor, not to change unless something else needs to) It's a bit weird saying we start from (limb_index = E_limbs-1, E_bit_index = biL-1) to then see

size_t E_limb_index = E_limbs;
size_t E_bit_index = 0;

which clearly does not match the comment and means one is immediately jumping forward to see what is missed.

I wonder about

/* E_bit_index == 0 is trigger to decrement E_limb_index and set E_bit_index = biL-1 */
size_t E_bit_index = 0;

@tom-cosgrove-arm
Copy link
Contributor

(ABI-API check false positive due to this PR being based on development prior to the CID changes going in)

gilles-peskine-arm
gilles-peskine-arm approved these changes Nov 29, 2022
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-work labels Nov 29, 2022
@gilles-peskine-arm gilles-peskine-arm merged commit edaa17b into Mbed-TLS:development Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces enhancement needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yanesca @tom-cosgrove-arm @gilles-peskine-arm