-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Bignum: Extract secp384r1 fast reduction from the prototype #7222
Bignum: Extract secp384r1 fast reduction from the prototype #7222
Conversation
2c25858
to
92513db
Compare
5e85a48
to
dd0dc74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only some minor issues found otherwise looks good to me.
99efac9
to
187a80e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs rebase, but other than that, it looks good to me.
I have checked and found a number which needs the second round of carry reduction for P384:
Please add it to the testing. |
5f8f200
Adding needs review because recent merges caused conflicts. |
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
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>
5f8f200
to
4af90bb
Compare
This patch re-introduces `mbedtls_ecp_fix_negative` and appropriately adjusts its' define guards. Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
During rebase it has been discovered that even there are a couple of redudant legacy macros which were guarded by
Whilst the majority of those macros are deprecated, and no longer needed since the curves have been moved to the protype implementation, A commit message has been added to keep this method, but adjust its define guards to
Which is the same that was happening indirectly by the previous guards, making it more clear. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/* | ||
* Fast quasi-reduction modulo p384 (FIPS 186-3 D.2.4) | ||
*/ | ||
static int ecp_mod_p384(mbedtls_mpi *N) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
library/ecp_invasive.h
Outdated
/** 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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean Must have exactly '768 / biL' limbs
?
(Saying "limb size" doesn't say what units are being used for "size")
static int ecp_mod_p384(mbedtls_mpi *N) | ||
{ | ||
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; | ||
size_t expected_width = 2 * ((384 + biL - 1) / biL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This is
required_width
notexpected_width
- The doc for
mbedtls_ecp_mod_p384_raw()
says required size is768 / biL
, but I don't see768
in this calculation (note: 384 is 3 * 128, so even withbiL
== 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
. But768 / 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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));
/* | ||
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to be clear: this isn't an issue with the new code, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we're not doing anything like this any more?
library/ecp_curves.c
Outdated
/* 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++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeating bits / 8 / sizeof(mbedtls_mpi_uint)
- can we have a variable for this please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Internal CI passed successfully. |
Description
Extract Secp384r1 fast reduction from the prototype (replace the pre-existing implementation)
Implements the requirements set by #6851:
MBEDTLS_STATIC_TESTABLE
so we can test it in isolation.Gatekeeper checklist
Resolves #6851