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

[EC] Unify point doubling for P-256/384/521 #1567

Merged
merged 13 commits into from
May 20, 2024
Merged

Conversation

dkostic
Copy link
Contributor

@dkostic dkostic commented May 1, 2024

Issues:

CryptoAlg-2408

Description of changes:

Implement and use a single version of point doubling
for implementations of NIST curves P-384, P-521, and
Fiat-crypto based implementation of P-256. The change
does not affect performance.

Point addition will be unified in a subsequent change.

Call-outs:

I verified the performance was not affected on Graviton 3, Intel, and M1 CPUs. Example for M1:

Before
Did 2882750 EC POINT P-384 dbl operations in 1000082us (2882513.6 ops/sec)
Did 1600000 EC POINT P-384 add operations in 1000497us (1599205.2 ops/sec)
Did 7051 EC POINT P-384 mul operations in 1078289us (6539.1 ops/sec)
Did 28000 EC POINT P-384 mul base operations in 1000115us (27996.8 ops/sec)
Did 5632 EC POINT P-384 mul public operations in 1062456us (5300.9 ops/sec)
Did 2685500 EC POINT P-521 dbl operations in 1000037us (2685400.6 ops/sec)
Did 1435000 EC POINT P-521 add operations in 1000129us (1434814.9 ops/sec)
Did 4928 EC POINT P-521 mul operations in 1055318us (4669.7 ops/sec)
Did 19000 EC POINT P-521 mul base operations in 1022199us (18587.4 ops/sec)
Did 3850 EC POINT P-521 mul public operations in 1036809us (3713.3 ops/sec)

After:
Did 2888250 EC POINT P-384 dbl operations in 1000028us (2888169.1 ops/sec)
Did 1593000 EC POINT P-384 add operations in 1000405us (1592355.1 ops/sec)
Did 6875 EC POINT P-384 mul operations in 1054301us (6520.9 ops/sec)
Did 28000 EC POINT P-384 mul base operations in 1000818us (27977.1 ops/sec)
Did 5555 EC POINT P-384 mul public operations in 1056370us (5258.6 ops/sec)
Did 2775250 EC POINT P-521 dbl operations in 1000021us (2775191.7 ops/sec)
Did 1435000 EC POINT P-521 add operations in 1000085us (1434878.0 ops/sec)
Did 4840 EC POINT P-521 mul operations in 1044164us (4635.3 ops/sec)
Did 19000 EC POINT P-521 mul base operations in 1027887us (18484.5 ops/sec)
Did 3883 EC POINT P-521 mul public operations in 1051447us (3693.0 ops/sec)

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@dkostic dkostic requested a review from a team as a code owner May 1, 2024 14:37
@dkostic dkostic force-pushed the ec-nistp-refactor branch from 6da596e to ef5e5ff Compare May 1, 2024 16:49
@dkostic dkostic force-pushed the ec-nistp-refactor branch from a5bb59b to 2d75906 Compare May 10, 2024 22:20
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.92%. Comparing base (7ef93cb) to head (4b55dad).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1567   +/-   ##
=======================================
  Coverage   77.91%   77.92%           
=======================================
  Files         560      561    +1     
  Lines       94629    94620    -9     
  Branches    13620    13599   -21     
=======================================
  Hits        73729    73729           
+ Misses      20305    20299    -6     
+ Partials      595      592    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crypto/fipsmodule/ec/ec_nistp.c Outdated Show resolved Hide resolved
crypto/fipsmodule/ec/ec_nistp.h Outdated Show resolved Hide resolved
Co-authored-by: Nevine Ebeid <66388554+nebeid@users.noreply.github.com>
nebeid
nebeid previously approved these changes May 15, 2024
crypto/fipsmodule/ec/ec_nistp.c Outdated Show resolved Hide resolved
crypto/fipsmodule/ec/ec_nistp.h Outdated Show resolved Hide resolved
crypto/fipsmodule/ec/ec_nistp.c Outdated Show resolved Hide resolved
crypto/fipsmodule/ec/ec_nistp.c Show resolved Hide resolved
@dkostic dkostic merged commit fc06ecb into aws:main May 20, 2024
78 of 81 checks passed
@dkostic dkostic deleted the ec-nistp-refactor branch May 20, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants