Proof-of-concept: make grp argument of ecp_mul const #7970
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When FIXED_POINT_OPTIM == 1, all Weierstrass curves (those that use comb multiplication) have a static table for multiples of G defined in ecp_curve.c, and there's not reason we'd want one of them not to. So, the code that handles the case where we don't have one, and then fall back to computing it storing it in grp->T, is effectively dead since 3.0. Actually it becomes undead when adding a new curve, before it has a static table: scripts/ecc_comb_table.py relies on this fallback kicking in so that we can then dump the T that was computed on the fly, stick the data in ecp_curves.c, then the fallback code becomes dead again. Having extra complexity in the library just for supporting a development script is rarely a good idea, especially when that complexity involves memory management, and is the only thing that prevents us from making the grp argument of ecp_mul const (which in turns forces us to play tricks to ensure thread safety when an ECC PK context is shared across threads). So, this commit is the first in a series that will clarify how the table of pre-computed points is used and managed. Right now, it breaks scripts/ecc_comb_table.py, but the script will be fixed later (by making it capable of computing the table on its own, rather than relying on undead code in the library for that). Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
This is a remnant of the era where we supported arbitrary curves, and took advantage of that by using some small curves in some tests. Now we only support standard curves, which all have nbits >= 192 so this code was effectively dead. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
This ensures we don't waste code handling T when it's not used. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
It was only useful for freeing all the points in the table, but we never want to do that as the table is static now. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
All the points in the table can be stored with Z freed, because Z is never going to be accessed. I'm not sure why we introduced the Z1 macros, but we should never have needed it. It was probably a miss in reviewing the PR that added static tables. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The old script was no longer working once we removed the legacy code that updated grp->T in ecp_mul_comb() (which was necessary to simplify the code an in the future support making grp const.) Also, I didn't like that it had quite a lot of C embedded in a python script. Since the new program needs access to an MBEDTLS_STATIC_TESTABLE function, and programs can't directly access internal header, take an indirect route via a "test" helper, that programs can access. (This is a bit of hack as non-test code lives in test, but I think it's OK.) Large parts of the "test" helper are adapted from the code previously embedded in the python script. The new program was manually tested by running on all short Weierstrass curves (1-8 and 10-12) and checking the output is byte for byte identical to the current content of ecp_curves.c. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Follow-up: simplify PK accordingly (no need for a copy any more) Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
3 tasks
gilles-peskine-arm
added
the
api-break
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just to check that #7623 indeed reaches its goal. We don't want to add the
constin a minor version, but this ensures we could.See also #7600.