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

Fix wrong modulo call in ecp_double_add_mxz #3209

Merged
merged 1 commit into from Apr 24, 2020
Merged

Fix wrong modulo call in ecp_double_add_mxz #3209

merged 1 commit into from Apr 24, 2020

Conversation

aurel32
Copy link
Contributor

@aurel32 aurel32 commented Apr 20, 2020

Description

ecp_double_add_mxz wrongly does an MPI addition followed by a call to
MOD_MUL instead of MOD_ADD. This is more visible since the
mbedtls_mpi_xxx_mod functions have been added in commit 3b3b34f
("Replace some macros by functions").

Fix that by using mbedtls_mpi_add_mod instead. The testsuite still
passes after that change.

Status

READY

Requires Backporting

I am not sure it requires backporting, I don't think it as any real impact.

Migrations

NO

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

@mpg mpg self-requested a review April 23, 2020 09:22
mpg
mpg previously approved these changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for catching and fixing this. Looks good to me.

@gilles-peskine-arm gilles-peskine-arm added this to To do in Mbed TLS PRs via automation Apr 23, 2020
@gilles-peskine-arm gilles-peskine-arm added component-crypto Crypto primitives and low-level interfaces needs: changelog needs-review Every commit must be reviewed by at least two team members, labels Apr 23, 2020
@gilles-peskine-arm
Copy link
Contributor

The CI failures are due to a temporary issue on development, now fixed. Despite this the test coverage is satisfactory so the CI results are acceptable for merging.

@mpg
Copy link
Contributor

mpg commented Apr 23, 2020

I checked the CI results and the only failures are due to a known issue that was fixed in development in the meantime (which shows as the pr-merge job passes), so that's as good as a pass.

@mpg mpg self-assigned this Apr 23, 2020
@mpg
Copy link
Contributor

mpg commented Apr 23, 2020 •
edited by gilles-peskine-arm

@aurel32 Our current policy is to acknowledge all external contributions in the ChangeLog. Could you add a new file under ChangeLog.d with an entry for this change? Suggested text:

Changes
   * Fix minor performance issue in operations on Curve25519 caused by using a
     suboptimal modular reduction in one place. Found and fix contributed by
     <your name and/or affiliation in your preferred format> in #3209.

Thanks!

Mbed TLS PRs automation moved this from To do to Approved Apr 23, 2020
@gilles-peskine-arm gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Apr 23, 2020
@aurel32
Fix wrong modulo call in ecp_double_add_mxz
66deb38 
ecp_double_add_mxz wrongly does an MPI addition followed by a call to
MOD_MUL instead of MOD_ADD. This is more visible since the
mbedtls_mpi_xxx_mod functions have been added in commit 3b3b34f
("Replace some macros by functions").

Fix that by using mbedtls_mpi_add_mod instead. The testsuite still
passes after that change.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
@aurel32 aurel32 dismissed stale reviews from gilles-peskine-arm and mpg via 66deb38 April 23, 2020 21:16
Mbed TLS PRs automation moved this from Approved to Review in progress Apr 23, 2020
@aurel32
Copy link
Contributor Author

aurel32 commented Apr 23, 2020

I have just done that and force pushed the result.

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs: changelog labels Apr 23, 2020
Mbed TLS PRs automation moved this from Review in progress to Approved Apr 24, 2020
mpg
mpg approved these changes Apr 24, 2020
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for adding the entry. LGTM.

@mpg mpg removed the needs-review Every commit must be reviewed by at least two team members, label Apr 24, 2020
@mpg
Copy link
Contributor

mpg commented Apr 24, 2020

I checked the CI results, and again the only failures are due to issues in development (at the point this PR was based on it) and unrelated to this PR - and the pr-merge job passes. So nothing preventing this PR from being merged.

@mpg mpg added the approved Design and code approved - may be waiting for CI or backports label Apr 24, 2020
@mpg mpg merged commit b1c8e41 into Mbed-TLS:development Apr 24, 2020
Mbed TLS PRs automation moved this from Approved to Done Apr 24, 2020
@daverodgman daverodgman removed this from Done in Mbed TLS PRs Mar 30, 2022
@aurel32 aurel32 deleted the fix-ecp_double_add_mxz branch May 16, 2022 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg mpg approved these changes

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

Assignees

@mpg mpg

Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@aurel32 @gilles-peskine-arm @mpg