[3.6] Rsapub additional tests #9493

yanesca · 2024-08-21T12:53:41Z

Description

Depends on: #9281 merged

Add some more tests for the performance regression fix.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

changelog not required because: just tests
development PR provided [dev] Rsapub performance fix #9536
framework PR not required
3.6 PR here
2.28 PR not required because: 2.28 not affected
tests provided

yanesca · 2024-08-22T07:51:44Z

Closing as accidentally pushed these commits to the prerequisite too and are being reviewed and will be merged there.

yanesca · 2024-08-22T12:08:38Z

Reopening as these commits won't make it for the release after all. All the remaining review comments should be addressed here as well:
#9281 (comment)
#9281 (comment)
#9281 (comment) 2.

See also #9281 (review)

yanesca · 2024-08-22T12:12:24Z

Added to the 3.6.1 epic as per #9281 (comment)

tom-cosgrove-arm · 2024-08-29T12:13:02Z

library/bignum_core.c

@@ -781,7 +781,8 @@ static inline void exp_mod_calc_first_bit_optionally_safe(const mbedtls_mpi_uint
        *E_bit_index = E_bits % biL;

 #if defined(MBEDTLS_TEST_HOOKS) && !defined(MBEDTLS_THREADING_C)
-        mbedtls_mpi_optionally_safe_codepath = MBEDTLS_MPI_IS_PUBLIC;
+        if(mbedtls_unsafe_codepath_hook != NULL)


surely this isn't going to pass code style (missing space after if)?

Not adding _unsafe version to the tests targeting behaviour related to RR as it is independent from the secret involved in the safe/unsafe distinction. Signed-off-by: Janos Follath <janos.follath@arm.com>

Only add the test hooks where it is meaningful. That is, not adding where the operation is essentially the same or the target is not the function that is being tested. Signed-off-by: Janos Follath <janos.follath@arm.com>

A + B + 1 is not a good way to get a number that's neither A nor B. This can be a problem for example if values later are changed to A = 0 and B = -1. Signed-off-by: Janos Follath <janos.follath@arm.com>

Unfortunately compilers aren't good at analyzing whether variables are analyzed on all code paths, and it is better to initialize to the safe-path values. Signed-off-by: Janos Follath <janos.follath@arm.com>

Without this, it's not at all obvious that turning on MBEDTLS_TEST_HOOKS doesn't change the functional behavior of the code. Signed-off-by: Janos Follath <janos.follath@arm.com>

Signed-off-by: Janos Follath <janos.follath@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

The rest of the file uses mbedtls_mpi_uint_t unconditionally, so its definition should also be #include'd unconditionally. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2024-09-02T09:15:41Z

Pushing an update that:

rebases over current 3.6
fixes code style
fixes one of the build issues found by the CI - let's see if other failures had the same cause of if there are others left.

I'd recommend holding your review until the CI is green.

tom-cosgrove-arm · 2024-09-02T10:11:45Z

tests/include/test/bignum_codepath_check.h

+ */
+#define ASSERT_BIGNUM_CODEPATH(path, ret, E)                            \
+    do {                                                                \
+        if((ret)!=0 || (E).n == 0)                                      \


Shouldn't we have a space after if? And shouldn't we also have braces { } on the if and else?

Aw, silly me, I ran scripts/code_style.py only on one file (where you pointed out a style issue) rather than all modified files.

(I'm not used to having to run it manually, because it's part of my pre-commit hook.)

tom-cosgrove-arm · 2024-09-02T10:18:11Z

tests/include/test/bignum_codepath_check.h

+ *
+ * When a function returns with an error, it can do so before reaching any interesting codepath. The
+ * same can happen if a parameter to the function is zero. In these cases we need to allow
+ * uninitialised value for the codepath tracking variable.


(minor) "we need to allow uninitialised value" sounds clunky - how about "we need to allow the codepath tracking variable to still have it's initial ("not set") value" (when I read "uninitialised" I interpreted it as "indeterminate", e.g. the value of n after int n;)

For some reason I didn't think about other files in the previous commit. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2024-09-02T10:48:35Z

CI came back green except for code style issues. I'm pushing an update fixing them (hopefully for real this time). Once CI fully green I'll go over existing feedback on this PR and on the original.

tom-cosgrove-arm · 2024-09-02T11:34:14Z

library/bignum_core.h

@@ -107,9 +105,16 @@
 *      // safe path
 *  }
 * not the other way round, in order to prevent misuse. (This is, if a value


(pre-existing) Should it be "That is" rather than "This is"?

tom-cosgrove-arm

Minor comments, but looks good otherwise

tom-cosgrove-arm · 2024-09-02T11:36:00Z

tests/include/test/bignum_codepath_check.h

+ *      - MBEDTLS_MPI_IS_SECRET: Only safe paths were teken since the last reset
+ *      - MBEDTLS_MPI_IS_PUBLIC: At least one unsafe path has been taken since the last reset
+ *
+ * Using a simple global variable to track execution path. Making it work with multithreading


(minor) Rather than "Using" should it be "Use" (if you prefer imperative) or "Uses"?

tom-cosgrove-arm · 2024-09-02T11:36:06Z

tests/include/test/bignum_codepath_check.h

+ *      - MBEDTLS_MPI_IS_PUBLIC: At least one unsafe path has been taken since the last reset
+ *
+ * Using a simple global variable to track execution path. Making it work with multithreading
+ * doesn't worth the effort as multithreaded tests add little to no value here.


(minor) "doesn't seem" or "isn't"?

mpg

I reviewed Janos's commits and I'm happy with them, except for the modification I'll do. He kindly agreed to review my commits when the time comes, so between the two of us that will make a complete review to complement Tom's.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2024-09-03T08:15:51Z

I've pushed a commit addressing Tom's comments, and double-check that remaining comments on #9281 had already been addressed by Janos's commits.

@yanesca I think this is now ready for your review.

tom-cosgrove-arm

LGTM

yanesca

I have reviewed Manuel's commits and they look good to me.

mpg

I have reviewed Janos's commits and he reviewed mine so that makes a complete review.

yanesca force-pushed the rsapub_additional_tests branch from 05ed559 to e5ebc85 Compare August 21, 2024 14:06

yanesca mentioned this pull request Aug 21, 2024

[3.6] Reduce performance regression in RSA public operations #9281

Merged

10 tasks

yanesca changed the title ~~Rsapub additional tests~~ [3.6] Rsapub additional tests Aug 21, 2024

mpg self-requested a review August 22, 2024 07:33

yanesca closed this Aug 22, 2024

yanesca reopened this Aug 22, 2024

yanesca added size-s Estimated task size: small (~2d) and removed size-xs Estimated task size: extra small (a few hours at most) labels Aug 22, 2024

mpg changed the base branch from mbedtls-3.6 to development August 28, 2024 10:55

mpg changed the base branch from development to mbedtls-3.6 August 28, 2024 10:56

mpg removed the needs-preceding-pr Requires another PR to be merged first label Aug 28, 2024

tom-cosgrove-arm reviewed Aug 29, 2024

View reviewed changes

mpg mentioned this pull request Sep 2, 2024

Performance regression in mbedtls_mpi_exp_mod() (v3.6.0) #9232

Closed

mpg self-assigned this Sep 2, 2024

mpg added the needs-work label Sep 2, 2024

yanesca added 4 commits September 2, 2024 10:30

Add tests for optionally safe code paths in bignum

5fc20fc

Not adding _unsafe version to the tests targeting behaviour related to RR as it is independent from the secret involved in the safe/unsafe distinction. Signed-off-by: Janos Follath <janos.follath@arm.com>

Add tests for optionally safe code paths in RSA

55be79b

Only add the test hooks where it is meaningful. That is, not adding where the operation is essentially the same or the target is not the function that is being tested. Signed-off-by: Janos Follath <janos.follath@arm.com>

Introduce MBEDTLS_MPI_IS_TEST

42f72b3

A + B + 1 is not a good way to get a number that's neither A nor B. This can be a problem for example if values later are changed to A = 0 and B = -1. Signed-off-by: Janos Follath <janos.follath@arm.com>

Initial local variables to secure default

e86607c

Unfortunately compilers aren't good at analyzing whether variables are analyzed on all code paths, and it is better to initialize to the safe-path values. Signed-off-by: Janos Follath <janos.follath@arm.com>

yanesca and others added 5 commits September 2, 2024 10:30

Move bignum code path testing out of the library

96cfd7a

Without this, it's not at all obvious that turning on MBEDTLS_TEST_HOOKS doesn't change the functional behavior of the code. Signed-off-by: Janos Follath <janos.follath@arm.com>

Fix incorrect test result

e9cc10d

Signed-off-by: Janos Follath <janos.follath@arm.com>

Prepare codepath tests for early termination

0a75adc

Signed-off-by: Janos Follath <janos.follath@arm.com>

Fix code style

126cfed

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Fix guards on #include

4bc15d8

The rest of the file uses mbedtls_mpi_uint_t unconditionally, so its definition should also be #include'd unconditionally. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg force-pushed the rsapub_additional_tests branch from 38f781e to 4bc15d8 Compare September 2, 2024 09:15

mpg removed the needs-review Every commit must be reviewed by at least two team members, label Sep 2, 2024

tom-cosgrove-arm reviewed Sep 2, 2024

View reviewed changes

Fix code style (for real this time, hopefully)

9ec6d45

For some reason I didn't think about other files in the previous commit. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

tom-cosgrove-arm reviewed Sep 2, 2024

View reviewed changes

mpg removed the needs-ci Needs to pass CI tests label Sep 3, 2024

mpg reviewed Sep 3, 2024

View reviewed changes

mpg removed the needs-reviewer This PR needs someone to pick it up for review label Sep 3, 2024

Misc improvements to comments

15fa9ce

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

tom-cosgrove-arm approved these changes Sep 3, 2024

View reviewed changes

tom-cosgrove-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 3, 2024

yanesca commented Sep 3, 2024

View reviewed changes

mpg mentioned this pull request Sep 4, 2024

[dev] Rsapub performance fix #9536

Merged

5 tasks

mpg approved these changes Sep 6, 2024

View reviewed changes

mpg 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, labels Sep 6, 2024

mpg added this pull request to the merge queue Sep 9, 2024

Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit f59d7b9 Sep 9, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.6] Rsapub additional tests #9493

[3.6] Rsapub additional tests #9493

yanesca commented Aug 21, 2024 •

edited by mpg

Loading

yanesca commented Aug 22, 2024

yanesca commented Aug 22, 2024 •

edited

Loading

yanesca commented Aug 22, 2024

tom-cosgrove-arm Aug 29, 2024

mpg commented Sep 2, 2024 •

edited

Loading

tom-cosgrove-arm Sep 2, 2024

mpg Sep 2, 2024

tom-cosgrove-arm Sep 2, 2024 •

edited

Loading

mpg commented Sep 2, 2024

tom-cosgrove-arm Sep 2, 2024

tom-cosgrove-arm left a comment

tom-cosgrove-arm Sep 2, 2024

tom-cosgrove-arm Sep 2, 2024

mpg left a comment

mpg commented Sep 3, 2024

tom-cosgrove-arm left a comment

yanesca left a comment

mpg left a comment

[3.6] Rsapub additional tests #9493

[3.6] Rsapub additional tests #9493

Conversation

yanesca commented Aug 21, 2024 • edited by mpg Loading

Description

PR checklist

yanesca commented Aug 22, 2024

yanesca commented Aug 22, 2024 • edited Loading

yanesca commented Aug 22, 2024

tom-cosgrove-arm Aug 29, 2024

Choose a reason for hiding this comment

mpg commented Sep 2, 2024 • edited Loading

tom-cosgrove-arm Sep 2, 2024

Choose a reason for hiding this comment

mpg Sep 2, 2024

Choose a reason for hiding this comment

tom-cosgrove-arm Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

mpg commented Sep 2, 2024

tom-cosgrove-arm Sep 2, 2024

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm Sep 2, 2024

Choose a reason for hiding this comment

tom-cosgrove-arm Sep 2, 2024

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg commented Sep 3, 2024

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

yanesca left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

yanesca commented Aug 21, 2024 •

edited by mpg

Loading

yanesca commented Aug 22, 2024 •

edited

Loading

mpg commented Sep 2, 2024 •

edited

Loading

tom-cosgrove-arm Sep 2, 2024 •

edited

Loading