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] Use s2n-bignum's modular inversion for P-256/384/521 #2057

Merged
merged 16 commits into from
Dec 17, 2024

Conversation

dkostic
Copy link
Contributor

@dkostic dkostic commented Dec 13, 2024

Issues:

N/A

Description of changes:

Use s2n-bignum's inversion modulo the field characteristic
for curves P-256/384/521. This gives us the following
performance improvements:

_____Apple M1____| before |  after | speedup |
P-256 ECDH       |  22724 |  23419 |   1.03x |
P-256 ECDSA sign |  60677 |  69731 |   1.15x |
P-384 ECDH       |   5863 |   6217 |   1.06x |
P-384 ECDSA sign |  13232 |  15011 |   1.13x |
P-521 ECDH       |   4041 |   4163 |   1.03x |
P-521 ECDSA sign |   7079 |   7584 |   1.07x |

______x86_64_____| before |  after | speedup |
P-256 ECDH       |  19410 |  20408 |   1.05x |
P-256 ECDSA sign |  54477 |  63617 |   1.17x |
P-384 ECDH       |   5309 |   5599 |   1.05x |
P-384 ECDSA sign |  12087 |  13780 |   1.14x |
P-521 ECDH       |   3539 |   3677 |   1.04x |
P-521 ECDSA sign |   6584 |   7068 |   1.07x |

_______GV4_______| before |  after | speedup |
P-256 ECDH       |  16642 |  17491 |   1.05x |
P-256 ECDSA sign |  51527 |  61108 |   1.18x |
P-384 ECDH       |   4208 |   4453 |   1.06x |
P-384 ECDSA sign |   9848 |  11308 |   1.15x |
P-521 ECDH       |   2668 |   2811 |   1.05x |
P-521 ECDSA sign |   5092 |   5626 |   1.10x |

Call-outs:

Point out areas that need special attention or support during the review process. Discuss architecture or design changes.

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 December 13, 2024 18:23
@dkostic dkostic changed the title S2n bignum inv [EC] Use s2n-bignum's modular inversion for P-256/384/521 Dec 13, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.74%. Comparing base (02ea4c4) to head (63633a8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2057      +/-   ##
==========================================
- Coverage   78.76%   78.74%   -0.02%     
==========================================
  Files         598      598              
  Lines      103688   103655      -33     
  Branches    14742    14733       -9     
==========================================
- Hits        81666    81627      -39     
- Misses      21369    21374       +5     
- Partials      653      654       +1     

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

andrewhop
andrewhop previously approved these changes Dec 16, 2024
@nebeid nebeid self-requested a review December 16, 2024 23:05
@@ -63,6 +63,8 @@ static inline void p256_montjscalarmul_selector(uint64_t res[S2N_BIGNUM_STATIC 1
else { p256_montjscalarmul(res, scalar, point); }
}

extern void bignum_montinv_p256(uint64_t z[S2N_BIGNUM_STATIC 4], const uint64_t x[S2N_BIGNUM_STATIC 4]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we document what the function does and that it's constant-time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@dkostic dkostic enabled auto-merge (squash) December 17, 2024 01:26
@dkostic dkostic merged commit 51ae4b1 into aws:main Dec 17, 2024
122 of 126 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants