[3.6] Reduce performance regression in RSA public operations

[mpg]

Description

Fix #9232 - partially, the low-hanging fruits (also those that don't make the code size go up by too much).

Status: work in progress.

TODO:

• measure code size
• measure perf on other platforms
• currently the window size is based on the limbsize of E, should it use the actual bitsize instead when E is public?
• decide on API
• propagate to RSA functions

PR checklist

• changelog provided
• development TODO
• 3.6 backport this is it
• 2.28 backport not required (we are fixing a regression in 3.6)
• tests partly provided, will be continued in [3.6] Rsapub additional tests #9493

[mpg]

Initial performance measurements on my laptop (64-bit Intel):

3.6.0

1024/1024-bit exp_mod, cold r: 0.961081 ms
1024/1024-bit exp_mod,  hot r: 1.050500 ms
  17/1024-bit exp_mod, cold r: 0.100309 ms
  17/1024-bit exp_mod,  hot r: 0.177438 ms

This PR

1024/1024-bit exp_mod, cold r: 1.070629 ms
1024/1024-bit exp_mod,  hot r: 1.062271 ms
  17/1024-bit exp_mod, cold r: 0.034083 ms
  17/1024-bit exp_mod,  hot r: 0.027739 ms

3.5.1

1024/1024-bit exp_mod, cold r: 1.071714 ms
1024/1024-bit exp_mod,  hot r: 1.083624 ms
  17/1024-bit exp_mod, cold r: 0.022744 ms
  17/1024-bit exp_mod,  hot r: 0.016366 ms

Edit: code used for benchmarking

[yanesca]

The approach looks good to me, this should eliminate all the overhead. One opinion about the API: maybe it would be worth to keep the _optionally_safe() function static and have an _unsafe() version in the header instead.

[gilles-peskine-arm]

Ok (with a few minor remarks) if we really have to have mixed-leakiness functions, but do we really have to do that? I find it harder to follow the code and reason about the function, and that's with a single function, I fear it won't scale to more complex algorithms.

[gilles-peskine-arm]

Some constants can be loaded with fewer instructions than others. We should pick a constant that's short to load on Thumb. I think 0x2a2a can be improved on. This page has some guidance.

[yanesca]

0x2a2a is a 16 bit value and can be loaded with a movw. That supposed to be a single cycle, how can we improve on that?

[gilles-peskine-arm]

@yanesca That's new in v7. And on second thoughts that blog post only covers 32-bit Arm instructions, but when thinking of code size, we care about Thumb instructions. So this is more relevant:

In Thumb instructions, constant can be:

• any constant that can be produced by shifting an 8-bit value left by any number of bits within a 32-bit word
• any constant of the form 0x00XY00XY
• any constant of the form 0xXY00XY00
• any constant of the form 0xXYXYXYXY.

That covers 0x002a002a and 0x2a2a2a2a and 0x0000002a and 0x00002a00 and more but not 0x00002a2a.

[tom-cosgrove-arm]

Just to confirm that for Thumb2 the comparisons are encoded as single instructions, e.g.

  16:   f1b1 3f2a       cmp.w   r1, #707406378  ; 0x2a2a2a2a

[gilles-peskine-arm]

If we're going to have a single function for both cases, I would prefer a more specific naming convention than optionally_safe. I don't like the use of “safe”, which could refer to many different things. I'd prefer leaky or “public”.

[gilles-peskine-arm]

I don't like the complexity of having functions whose security properties depend on a runtime argument. Neither P256-m nor BearSSL do that. I would prefer to have separate code paths for leaky and non-leaky, converging only on functions that have a single implementation either way. The only argument I can think of to merge the two is code size. Is the gain really worth it?

[yanesca]

I am not happy about it either, but I think now that we are turning on USE_PSA_CRYPTO by default, we will be pretty desperate for code size savings for a while. I think the gain can worth it and we should try.

[mpg]

Ok (with a few minor remarks) if we really have to have mixed-leakiness functions, but do we really have to do that?

No, I don't think we absolutely have to do that, and that's one of the main points about which I was hoping to get feedback. I just has to make a temporary choice for prototyping.

One opinion about the API: maybe it would be worth to keep the _optionally_safe() function static and have an _unsafe() version in the header instead.

Yes, I think that would already be better.

I don't like the complexity of having functions whose security properties depend on a runtime argument.

Agreed. Do you think it's acceptable for static functions though or would you rather avoid it entirely?

The only argument I can think of to merge the two is code size. Is the gain really worth it?

That, and also there can be a maintenance cost if we end up duplicating source code. I can give it a try to do some size measurements and check how easy or awkward it is to avoid duplication.

[gilles-peskine-arm]

I don't like the complexity of having functions whose security properties depend on a runtime argument.

Agreed. Do you think it's acceptable for static functions though or would you rather avoid it entirely?

I'd prefer to avoid it, but for smaller functions with a limited scope, it's not so bad.

[jforissier]

With this PR (and the necessary changes to make use of the E_public parameter), the OP-TEE test case runs faster although still much slower than with v3.5.2. This is because the test doesn't only run RSA signature verification and decryption, it also includes RSA signing and encryption. Assuming OP-TEE switches to the more secure algorithm for secret stuff, we may need to disable this test in the CI and run it only during the release tests.
Here are the numbers (running time xtest 4011 on the arm32 QEMU target):

• v3.5.2 (optee_os master @ b339ffbd9): 14.5s
• v3.6.0: 8m 26.6s
• This PR (my branch: test-mbedtls-expmod-regression-fix-9281 @ 9dca1658f): 5m 58s

[mpg]

Just checking: @gilles-peskine-arm @yanesca will you be volunteering to review this once I've updated it, or should I look for another reviewer? (I'd appreciate if at least one of you could give at least a design review.)

[gilles-peskine-arm]

I intend to review if I'm around (I'll take about a week off around 15 August).

[yanesca]

I am happy to review it as well, but I will be taking some days off too (this Friday and next Wednesday).

[yanesca]

There is still work needed, just pushing what I have so far to get an early CI overnight.

[yanesca]

Reverted to 878af12.

[gilles-peskine-arm]

I approve the library changes for the release freeze today, after fixing the CI (plus the changelog entry while you're at it). I haven't reviewed the tests, but I'm satisfied that they're non-harmful.

This is conditional on doing the following as soon as possible, in any case before the end of the quarter:

• Have the tests reviewed and approved (by me or someone else).
• Improving the test hooks
• Make initialization robust

[gilles-peskine-arm]

I approve the library changes at 5f31697 for the release freeze today. I haven't reviewed the tests, but I'm satisfied that they're non-harmful.

This is conditional on doing the following as soon as possible, in any case before the end of the quarter:

• Have the tests reviewed and approved (by me or someone else).
• Improving the test hooks
• Make initialization robust

[tom-cosgrove-arm]

LGTM

[gilles-peskine-arm]

LGTM with the reservations in #9281 (review)

[gilles-peskine-arm]

I've pressed the merge button so that we can get on with the release. We need a forward-port to development, but that doesn't go through the release process so it can wait a few days.