[EC] Implement generic scalar multiplication #1982
Conversation
This change implements a generic scalar multiplication function that can work in four modes of operation which covers the three functions we need to implement: multiplication of an arbitrary point, of a base point, and the so-called public multiplication.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1982 +/- ##
==========================================
+ Coverage 78.79% 78.85% +0.06%
==========================================
Files 590 593 +3
Lines 101506 101977 +471
Branches 14401 14454 +53
==========================================
+ Hits 79979 80414 +435
- Misses 20891 20918 +27
- Partials 636 645 +9 ☔ View full report in Codecov by Sentry. |
| @@ -444,6 +453,15 @@ void ec_nistp_scalar_mul(const ec_nistp_meth *ctx, | |||
| ec_nistp_felem_limb *x_tmp = &tmp[0]; | |||
| ec_nistp_felem_limb *y_tmp = &tmp[ctx->felem_num_limbs]; | |||
| ec_nistp_felem_limb *z_tmp = &tmp[ctx->felem_num_limbs * 2]; | |||
There was a problem hiding this comment.
something ensures no overflow?
There was a problem hiding this comment.
few lines above:
ec_nistp_felem_limb tmp[3 * FELEM_MAX_NUM_OF_LIMBS];
where FELEM_MAX_NUM_OF_LIMBS is defined to ensure no overflow. But I can add an assert if you think it's worth it?
There was a problem hiding this comment.
Hmm, a runtime debug assert on ctx->felem_num_limbs seems reasonable to exercise that during testing.
There was a problem hiding this comment.
Actually, you already have these below
assert(SCALAR_MUL_TABLE_MAX_NUM_FELEM_LIMBS >=
SCALAR_MUL_TABLE_NUM_POINTS * ctx->felem_num_limbs * 3);
So, that should be sufficient, right?
crypto/fipsmodule/ec/ec_nistp.c
Outdated
| // Table of multiples of the base point G. | ||
| const ec_nistp_felem_limb *table_g = ctx->scalar_mul_base_table; | ||
|
|
||
| scalar_mul_core(ctx, x_res_g, y_res_g, z_res_g, scalar_g, table_g, base); |
| @@ -444,6 +453,15 @@ void ec_nistp_scalar_mul(const ec_nistp_meth *ctx, | |||
| ec_nistp_felem_limb *x_tmp = &tmp[0]; | |||
| ec_nistp_felem_limb *y_tmp = &tmp[ctx->felem_num_limbs]; | |||
| ec_nistp_felem_limb *z_tmp = &tmp[ctx->felem_num_limbs * 2]; | |||
There was a problem hiding this comment.
Hmm, a runtime debug assert on ctx->felem_num_limbs seems reasonable to exercise that during testing.
| // Table of multiples of P = (x_in, y_in, z_in). | ||
| ec_nistp_felem_limb table[SCALAR_MUL_TABLE_MAX_NUM_FELEM_LIMBS]; | ||
| generate_table(ctx, table, x_in, y_in, z_in); | ||
| static inline void scalar_mul_core(const ec_nistp_meth *ctx, |
There was a problem hiding this comment.
quite a big function to inline.
| const EC_SCALAR *scalar, | ||
| const ec_nistp_felem_limb *table, | ||
| enum scalar_mul_core_mode mode) { | ||
| // Mode based invariants. |
There was a problem hiding this comment.
Just something to ponder later. Is something else using scalar_mul_core apart from the three function further below? If not, while these modes attempts to abstract some semantics, it's almost too much abstraction. If the set of functions that invoke, and will ever invoke scalar_mul_core, one could have this as three arguments.
It also appears there is some implicit relationship between the table argument and the mode argument. Although, any mistakes mis-configuring that e.g. use mode != base for a full table, will likely cause the unit tests to have severe hiccups.
| @@ -444,6 +453,15 @@ void ec_nistp_scalar_mul(const ec_nistp_meth *ctx, | |||
| ec_nistp_felem_limb *x_tmp = &tmp[0]; | |||
| ec_nistp_felem_limb *y_tmp = &tmp[ctx->felem_num_limbs]; | |||
| ec_nistp_felem_limb *z_tmp = &tmp[ctx->felem_num_limbs * 2]; | |||
There was a problem hiding this comment.
Actually, you already have these below
assert(SCALAR_MUL_TABLE_MAX_NUM_FELEM_LIMBS >=
SCALAR_MUL_TABLE_NUM_POINTS * ctx->felem_num_limbs * 3);
So, that should be sufficient, right?
Issues:
Resolves #ISSUE-NUMBER1
Addresses #ISSUE-NUMBER2
Description of changes:
This change implements a generic scalar multiplication function
that can work in four modes of operation which covers the three
functions we need to implement: multiplication of an arbitrary
point, of a base point, and the so-called public multiplication.
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.