[EC] Implement generic scalar multiplication #1982

dkostic · 2024-11-11T19:21:43Z

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.

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-commenter · 2024-11-11T19:44:10Z

Codecov Report

Attention: Patch coverage is 59.32203% with 24 lines in your changes missing coverage. Please review.

Project coverage is 78.85%. Comparing base (c9d48a6) to head (1bf868b).
Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
crypto/fipsmodule/ec/ec_nistp.c	52.94%	24 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

crypto/fipsmodule/ec/ec_nistp.c

torben-hansen · 2024-11-12T21:00:40Z

crypto/fipsmodule/ec/ec_nistp.c

@@ -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];


something ensures no overflow?

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?

Hmm, a runtime debug assert on ctx->felem_num_limbs seems reasonable to exercise that during testing.

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?

torben-hansen · 2024-11-12T21:20:51Z

crypto/fipsmodule/ec/ec_nistp.c

+  // 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);


non constant modes?

torben-hansen · 2024-11-13T14:50:47Z

crypto/fipsmodule/ec/ec_nistp.c

@@ -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];


Hmm, a runtime debug assert on ctx->felem_num_limbs seems reasonable to exercise that during testing.

torben-hansen · 2024-11-13T14:55:56Z

crypto/fipsmodule/ec/ec_nistp.c

-  // 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,


quite a big function to inline.

torben-hansen · 2024-11-13T15:02:56Z

crypto/fipsmodule/ec/ec_nistp.c

+                                   const EC_SCALAR *scalar,
+                                   const ec_nistp_felem_limb *table,
+                                   enum scalar_mul_core_mode mode) {
+  // Mode based invariants.


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.

torben-hansen · 2024-11-13T15:06:11Z

crypto/fipsmodule/ec/ec_nistp.c

@@ -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];


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?

nebeid

Closed, but just submitting my pending changes.

nebeid · 2024-11-18T19:21:41Z

crypto/fipsmodule/ec/ec_nistp.c

+// operation doesn't have to be implemented in constant-time.
+enum scalar_mul_core_mode { base, base_nonct, arbitrary, arbitrary_nonct };
+//
+// In arbitrary modes the input table is a single table with odd multiples of P:


Suggested change

// In arbitrary modes the input table is a single table with odd multiples of P:

// When used to multiply an arbitrary point, the input table is a single table with odd multiples of P:

nebeid · 2024-11-18T19:22:05Z

crypto/fipsmodule/ec/ec_nistp.c

+//
+// In arbitrary modes the input table is a single table with odd multiples of P:
+//     [2i + 1]P for i in [0, SCALAR_MUL_TABLE_NUM_POINTS - 1],
+// In base modes the input table is a full table of m single subtables:


Suggested change

// In base modes the input table is a full table of m single subtables:

// When used to multiply a base point, the input table is a full table of m single subtables:

nebeid · 2024-11-18T19:23:26Z

crypto/fipsmodule/ec/ec_nistp.c

 //
 // The required product, [scalar]P, is computed by the following algorithm.
 //     1. Initialize the accumulator with the point from |table|
 //        corresponding to the most significant digit s_{m-1} of the scalar.
 //     2. For digits s_i starting from s_{m-2} down to s_0:
-//     3.   Double the accumulator w times. (note that doubling a point [a]P
-//          w times results in [2^w*a]P).
+//     3.   Double the accumulator w times if in an arbitrary mode.


Suggested change

// 3. Double the accumulator w times if in an arbitrary mode.

// 3. Double the accumulator w times if in an arbitrary-point multiplication mode.

dkostic requested a review from a team as a code owner November 11, 2024 19:21

dkostic mentioned this pull request Nov 11, 2024

[EC] Unify scalar multiplication of the base point for ec_nistp curves #1720

Closed

torben-hansen reviewed Nov 12, 2024

View reviewed changes

addressing code review

1bf868b

torben-hansen approved these changes Nov 13, 2024

View reviewed changes

nebeid self-requested a review November 15, 2024 17:13

dkostic closed this Nov 19, 2024

nebeid reviewed Nov 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EC] Implement generic scalar multiplication #1982

[EC] Implement generic scalar multiplication #1982

dkostic commented Nov 11, 2024

codecov-commenter commented Nov 11, 2024 •

edited

Loading

torben-hansen Nov 12, 2024

dkostic Nov 12, 2024

torben-hansen Nov 13, 2024

torben-hansen Nov 13, 2024

torben-hansen Nov 12, 2024

dkostic Nov 12, 2024

torben-hansen Nov 13, 2024

torben-hansen Nov 13, 2024

torben-hansen Nov 13, 2024

torben-hansen Nov 13, 2024

nebeid left a comment

nebeid Nov 18, 2024

nebeid Nov 18, 2024

nebeid Nov 18, 2024

	// In arbitrary modes the input table is a single table with odd multiples of P:
	// When used to multiply an arbitrary point, the input table is a single table with odd multiples of P:

	// In base modes the input table is a full table of m single subtables:
	// When used to multiply a base point, the input table is a full table of m single subtables:

	// 3. Double the accumulator w times if in an arbitrary mode.
	// 3. Double the accumulator w times if in an arbitrary-point multiplication mode.

[EC] Implement generic scalar multiplication #1982

[EC] Implement generic scalar multiplication #1982

Conversation

dkostic commented Nov 11, 2024

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Nov 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nebeid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 11, 2024 •

edited

Loading