Bignum: Extract secp384r1 fast reduction from the prototype #7222

minosgalanakis · 2023-03-07T15:43:40Z

Description

Extract Secp384r1 fast reduction from the prototype (replace the pre-existing implementation)

Implements the requirements set by #6851:

Extract the method from the prototype.
Implement it usings the MBEDTLS_STATIC_TESTABLE so we can test it in isolation.
Implementes tests in cannonical representation using Add function to fix quasi-reduction #6375.

Gatekeeper checklist

changelog Not required( New Interface )
backport Not required( New Interface )
tests Provided

Resolves #6851

library/ecp_curves.c

library/ecp_invasive.h

scripts/mbedtls_dev/ecp.py

tests/suites/test_suite_ecp.function

library/ecp_curves.c

scripts/mbedtls_dev/ecp.py

library/ecp_curves.c

gabor-mezei-arm

Only some minor issues found otherwise looks good to me.

library/ecp_invasive.h

library/ecp_curves.c

scripts/mbedtls_dev/ecp.py

gabor-mezei-arm

LGTM

yanesca · 2023-03-17T13:46:43Z

Ideally we would be testing the second round of carry propagation in the end, but triggering it is not trivial and we shouldn't spend more time on it. I have raised an issue for tracking this and added to the tech debt epic: #7309

yanesca

Needs rebase, but other than that, it looks good to me.

gabor-mezei-arm · 2023-03-17T15:51:07Z

I have checked and found a number which needs the second round of carry reduction for P384:

000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00000000000000000000000000000000FFFFFFFF00000000000000000000000100000000000000000000000000000000FFFFFFFF00000001

Please add it to the testing.

minosgalanakis · 2023-03-21T14:55:03Z

Adding needs review because recent merges caused conflicts.

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

This patch adjusts formatting, documentation and testing. Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

This patch re-introduces `mbedtls_ecp_fix_negative` and appropriately adjusts its' define guards. Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

minosgalanakis · 2023-03-22T11:48:40Z

During rebase it has been discovered that even there are a couple of redudant legacy macros which were guarded by

#if defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \
    defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED)

Whilst the majority of those macros are deprecated, and no longer needed since the curves have been moved to the protype implementation, mbedtls_ecp_fix_negative is needed by other curves test data.

A commit message has been added to keep this method, but adjust its define guards to

#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_ECP_C)

Which is the same that was happening indirectly by the previous guards, making it more clear.

gabor-mezei-arm

LGTM

tom-cosgrove-arm · 2023-03-22T17:23:05Z

library/ecp_curves.c

+/*
+ * Fast quasi-reduction modulo p384 (FIPS 186-3 D.2.4)
+ */
+static int ecp_mod_p384(mbedtls_mpi *N)


It seems very weird having this function above the one it calls, given the long discussion we had last year about "I should be able to read the file in order, seeing functions defined before they are used" :)

True, but there are cases for file consistency > newly tidy code. Since the rest of the file is structured as such, we should keep the public method (first) -> static method (second notation.

tom-cosgrove-arm · 2023-03-22T17:26:51Z

library/ecp_invasive.h

+/** Fast quasi-reduction modulo p384 (FIPS 186-3 D.2.4)
+ *
+ * \param[in,out]   X       The address of the MPI to be converted.
+ *                          Must have exact limb size of `768 / biL`.


Does this mean Must have exactly '768 / biL' limbs?

(Saying "limb size" doesn't say what units are being used for "size")

tom-cosgrove-arm · 2023-03-22T17:30:52Z

library/ecp_curves.c

+static int ecp_mod_p384(mbedtls_mpi *N)
+{
+    int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
+    size_t expected_width = 2 * ((384 + biL - 1) / biL);


This is required_width not expected_width

The doc for mbedtls_ecp_mod_p384_raw() says required size is 768 / biL, but I don't see 768 in this calculation (note: 384 is 3 * 128, so even with biL == 128 (which it is never) the + biL - 1) / biL thing wouldn't be needed. But if it is needed, please keep 768 in there, i.e. (768 + biL - 1) / biL. But 768 / biL with a comment that 768 = 3 * 256, so no need to worry about partial limbs even if limb size was 256 bits would be fine

Again that is ported as is from the prototype, and has been used in all the similar curves. I would rather we keep it consistent as is.

This wording a request coming from the previous round of review. But I agree that is is confusing.

I will reword it to something like

Must have exact limb size that stores a 768-bit MPI * (double the bitlength of the modulus).

The size check is not performed in the X parameter but the X_limbs which is enforced to be less than 2* 386bits .

In the public method, a grow operation ensures, that the static method will be called with appropriate sizes:

MBEDTLS_MPI_CHK(mbedtls_mpi_grow(N, expected_width));

tom-cosgrove-arm · 2023-03-22T17:32:17Z

library/ecp_curves.c

-/*
- * For these primes, we need to handle data in chunks of 32 bits.
- * This makes it more complicated if we use 64 bits limbs in MPI,
- * which prevents us from using a uniform access method as for p192.


So just to be clear: this isn't an issue with the new code, right?

Yes and no. We are doing something similar in principle, but also different in some corner cases. Carrying over design comments from removed, curve specific macros is adding confusion.

tom-cosgrove-arm · 2023-03-22T17:32:28Z

library/ecp_curves.c

- * So, we define a mini abstraction layer to access 32 bit chunks,
- * load them in 'cur' for work, and store them back from 'cur' when done.
- *
- * While at it, also define the size of N in terms of 32-bit chunks.


And we're not doing anything like this any more?

tom-cosgrove-arm · 2023-03-22T17:45:17Z

library/ecp_curves.c

+    /* Set N := 2^bits - 1 - N. We know that 0 <= N < 2^bits, so
+     * set the absolute value to 0xfff...fff - N. There is no carry
+     * since we're subtracting from all-bits-one.  */
+    for (i = 0; i <= bits / 8 / sizeof(mbedtls_mpi_uint); i++) {


Repeating bits / 8 / sizeof(mbedtls_mpi_uint) - can we have a variable for this please?

This function is not new, it was just moved further down the code. I would rather not modify it since it falls out of the scope of this PR

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

tom-cosgrove-arm

LGTM

gabor-mezei-arm

LGTM

yanesca · 2023-03-27T15:56:23Z

Internal CI passed successfully.

minosgalanakis force-pushed the bignum/6851_extract_Secp384r1_fast_reduction branch from 2c25858 to 92513db Compare March 7, 2023 16:48

minosgalanakis removed the needs-ci Needs to pass CI tests label Mar 8, 2023

gabor-mezei-arm self-requested a review March 8, 2023 15:00

gabor-mezei-arm requested changes Mar 8, 2023

View reviewed changes

minosgalanakis force-pushed the bignum/6851_extract_Secp384r1_fast_reduction branch from 5e85a48 to dd0dc74 Compare March 9, 2023 14:51

gabor-mezei-arm self-requested a review March 14, 2023 11:35

gabor-mezei-arm requested changes Mar 14, 2023

View reviewed changes

library/ecp_invasive.h Outdated Show resolved Hide resolved

library/ecp_curves.c Outdated Show resolved Hide resolved

scripts/mbedtls_dev/ecp.py Show resolved Hide resolved

scripts/mbedtls_dev/ecp.py Outdated Show resolved Hide resolved

minosgalanakis force-pushed the bignum/6851_extract_Secp384r1_fast_reduction branch 3 times, most recently from 99efac9 to 187a80e Compare March 15, 2023 13:11

minosgalanakis requested a review from gabor-mezei-arm March 16, 2023 09:05

gabor-mezei-arm previously approved these changes Mar 16, 2023

View reviewed changes

yanesca previously approved these changes Mar 17, 2023

View reviewed changes

yanesca mentioned this pull request Mar 17, 2023

Add test triggering the second carry in secp384r1 #7309

Closed

minosgalanakis dismissed stale reviews from yanesca and gabor-mezei-arm via 5f8f200 March 20, 2023 12:21

minosgalanakis requested a review from gabor-mezei-arm March 21, 2023 14:47

minosgalanakis added the needs-work label Mar 21, 2023

minosgalanakis added 3 commits March 21, 2023 15:41

ecp_curves: Ported prototypes

6fb105f

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

test_suite_ecp: Added ecp_mod_p384_raw() test case.

619385d

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

ecp test generator: Added EcpPp384R1Raw().

f359c91

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

minosgalanakis added 2 commits March 21, 2023 15:46

ecp_curves: Minor rework for p384

37f4cb6

This patch adjusts formatting, documentation and testing. Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

EcpP384R1Raw: Added test case for 2nd round of carry reduction.

4af90bb

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

minosgalanakis force-pushed the bignum/6851_extract_Secp384r1_fast_reduction branch from 5f8f200 to 4af90bb Compare March 21, 2023 15:50

ecp_curves: Re-introduced mbedtls_ecp_fix_negative()

68d64a1

This patch re-introduces `mbedtls_ecp_fix_negative` and appropriately adjusts its' define guards. Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

minosgalanakis added needs-ci Needs to pass CI tests and removed needs-work needs-ci Needs to pass CI tests labels Mar 22, 2023

minosgalanakis mentioned this pull request Mar 22, 2023

ecp: Unify test cases #7329

Merged

3 tasks

gabor-mezei-arm previously approved these changes Mar 22, 2023

View reviewed changes

tom-cosgrove-arm reviewed Mar 22, 2023

View reviewed changes

ecp_curves: Updated ecp_mod_p384_raw documentation

f9fca53

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>

minosgalanakis dismissed gabor-mezei-arm’s stale review via f9fca53 March 23, 2023 12:17

tom-cosgrove-arm approved these changes Mar 25, 2023

View reviewed changes

minosgalanakis requested a review from gabor-mezei-arm March 27, 2023 08:35

gabor-mezei-arm approved these changes Mar 27, 2023

View reviewed changes

gabor-mezei-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Mar 27, 2023

yanesca merged commit 445c3bf into Mbed-TLS:development Mar 27, 2023

minosgalanakis mentioned this pull request Mar 27, 2023

ecp: Updated ecp384 depedency macro #7343

Merged

3 tasks

gabor-mezei-arm mentioned this pull request Mar 28, 2023

Remove obsolete ecp_fix_negative function #7352

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bignum: Extract secp384r1 fast reduction from the prototype #7222

Bignum: Extract secp384r1 fast reduction from the prototype #7222

minosgalanakis commented Mar 7, 2023 •

edited by yanesca

Loading

gabor-mezei-arm left a comment

gabor-mezei-arm left a comment

yanesca commented Mar 17, 2023

yanesca left a comment

gabor-mezei-arm commented Mar 17, 2023

minosgalanakis commented Mar 21, 2023

minosgalanakis commented Mar 22, 2023 •

edited

Loading

gabor-mezei-arm left a comment

tom-cosgrove-arm Mar 22, 2023

minosgalanakis Mar 23, 2023

tom-cosgrove-arm Mar 22, 2023

tom-cosgrove-arm Mar 22, 2023

minosgalanakis Mar 23, 2023

tom-cosgrove-arm Mar 22, 2023

minosgalanakis Mar 23, 2023

tom-cosgrove-arm Mar 22, 2023

tom-cosgrove-arm Mar 22, 2023

minosgalanakis Mar 23, 2023 •

edited

Loading

tom-cosgrove-arm Mar 25, 2023

tom-cosgrove-arm left a comment

gabor-mezei-arm left a comment

yanesca commented Mar 27, 2023

Bignum: Extract secp384r1 fast reduction from the prototype #7222

Bignum: Extract secp384r1 fast reduction from the prototype #7222

Conversation

minosgalanakis commented Mar 7, 2023 • edited by yanesca Loading

Description

Gatekeeper checklist

gabor-mezei-arm left a comment

Choose a reason for hiding this comment

gabor-mezei-arm left a comment

Choose a reason for hiding this comment

yanesca commented Mar 17, 2023

yanesca left a comment

Choose a reason for hiding this comment

gabor-mezei-arm commented Mar 17, 2023

minosgalanakis commented Mar 21, 2023

minosgalanakis commented Mar 22, 2023 • edited Loading

gabor-mezei-arm 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

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

minosgalanakis Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

gabor-mezei-arm left a comment

Choose a reason for hiding this comment

yanesca commented Mar 27, 2023

minosgalanakis commented Mar 7, 2023 •

edited by yanesca

Loading

minosgalanakis commented Mar 22, 2023 •

edited

Loading

minosgalanakis Mar 23, 2023 •

edited

Loading