adjusting size of sliding window array to correct size.

[d-otte]

Probably the W[2 << MBEDTLS_MPI_WINDOW_SIZE] notation is based on a transcription of 2**MBEDTLS_MPI_WINDOW_SIZE.

This PR fixes #3591 [edited by mpg to use github keyword]

Status

READY

Requires Backporting

This change is easy to backport, but is not strictly required.

I would recommend to backport it to all LTS branches.

Migrations

No API changes

Additional comments

This change would improve stack usage significantly.

Todos

• Changelog updated
• Backported

[gilles-peskine-arm]

The change looks sensible to me. Thanks for spotting this!

Please update the comment in bignum.h as well. (And while you're at it, would you mind making a commit to change “Maximum windows size” to “Maximum window size” in bignum.h and config.h?) By the way, this comment confirms the intent, since it claims that 2 << MBEDTLS_MPI_WINDOW_SIZE is 64 when MBEDTLS_MPI_WINDOW_SIZE is 6.

Please add a changelog entry file in ChangeLog.d (section: Changes).

We don't normally backport performance improvements, but this one is very simple and has no drawback (e.g. no code size vs performance trade-off), so I think we should backport it.

[mpg]

Other that the things already pointed out by Gilles, this looks good to me. Thanks for spotting and fixing this!

With the default configuration, the stack saving is 64 * sizeof (mbedtls_mpi) which appears to be 768 bytes on ARMv6-M and 1536 bytes on x86-64. That's quite significant, so I agree that it would be great to have this backported, as there is no drawback.

[d-otte]

Now, back from vacation, I included your recommendations. I also changed the notation in the comment from 2 << MBEDTLS_MPI_WINDOW_SIZE to 2 ** MBEDTLS_MPI_WINDOW_SIZE. I found several occasions in other source files which use ** to denote exponentiation.

Since some time had passed, should I rebase my changes?

[gilles-peskine-arm]

Thanks for updating. You don't need to rebase, but you can if you want. Rebasing after review makes it harder to review large changes, but this is a very small change so here it isn't an issue.

Please note that all commits must have a signed-off-by line to indicate that you warrant that you have the right to submit it. We can't accept the pull request if the DCO check fails.

[mpg]

Thanks for making the suggested changes. Looks good to me except for two things:

1. I think this deserves a ChangeLog entry, saving 768 of stack in the default config in 32-bit platforms is significant. (As Gilles suggested earlier, I think the "Changes" section is appropriate.)
2. As Gilles pointed out, we need the Signed-Off-By line in the last two commits as well.

[d-otte]

I added the Signed-off-by: lines, but had to force the push. I hope this does not confuse the history to much.

Thanks for reminding me to take credit in the ChangeLog 😆.

I used the description significant to designate the improvement, since it might vary from platform to platform. sizeof(mbedtls_mpi) depends on the size of int , size_t, mbedtls_mpi_uint and maybe alignment or padding, but the most common cases will indeed be 768 byte for 32-bit systems and 1536 bytes for 64-bit systems.

[mpg]

Looks all good to me.

[gilles-peskine-arm]

Thanks! Would you mind submitting backports to mbedtls-2.16 and mbedtls-2.7 as well?

[d-otte]

Would you mind submitting backports to mbedtls-2.16 and mbedtls-2.7 as well?
I will try to get this done today. But since I will probably be away the next seven weeks, maybe someone else has to finish this.

Should I base my changes on the mbedtls-2.16 and mbedtls-2.7 branch and create two dedicate PRs?

And I'm a bit unsure about the Mbed OS Testing failures. Do I have to care about them? I can't view the details, so I have no idea what they are about.

[mpg]

I will try to get this done today. But since I will probably be away the next seven weeks, maybe someone else has to finish this.

Ok, if you can create backports before you go, that would be very helpful and we'll be sure to review them quickly, otherwise we'll take care of the backports.

Should I base my changes on the mbedtls-2.16 and mbedtls-2.7 branch and create two dedicate PRs?

Precisely! The description of the new PRs can be just "Backport of #3592 to the xxx branch." (Usually git cherry-pick is helpful creating backports.)

And I'm a bit unsure about the Mbed OS Testing failures. Do I have to care about them? I can't view the details, so I have no idea what they are about.

Thanks for asking, but you don't need to care about them, we're actually going to remove them from our CI, see #3650