Skip to content

Constify mbedtls_cipher_supported #10139

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

Open
wants to merge 1 commit into
base: mbedtls-3.6
Choose a base branch
from

Conversation

XavierChapron
Copy link

Description

Constify cipher_wrap:mbedtls_cipher_supported

This structure can be initialized during the compilation and there is no reason it changes after that.
Making it const allows the compiler to put it in .rodata section instead of .bss one.

This also spares a few bytes in .text section by simplifying mbedtls_cipher_list().

Discussed with @gilles-peskine-arm here Mbed-TLS/TF-PSA-Crypto#261 (comment)

PR checklist

  • changelog not required because: This doesn't fix anything relevant
  • development PR not required because: This is going to be refactored
  • TF-PSA-Crypto PR not required because: It doesn't apply
  • framework PR not required
  • 3.6 PR provided: This is the one.
  • tests not required because: Already included in cipher tests build.

@minosgalanakis minosgalanakis added component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d) needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-medium Medium priority - this can be reviewed as time permits labels Apr 23, 2025
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally, LGTM.

I used this code to validate the list (the left-hand side of the pipe can also be used to update it):

<library/cipher_wrap.c perl -l -ne 'next unless /mbedtls_cipher_definitions\[\] =/../^\}/; print if /^[#{}]/; print "    $1" if /\{ *(MBEDTLS_\w+,)/' | diff - <(perl -l -ne 'print if /mbedtls_cipher_supported\[/../^\}/' library/cipher_wrap.c)

However, now that I look at the result, I'm not so sure this is desirable. This saves some code, but also increases rodata by the same amount that is saved in BSS. So the binary is larger if there are many ciphers. The threshold depends on the code size that is saved. With (one particular version of) arm-none-eabi-gcc -mthumb -mcpu=cortex-m0plus -Os, I see 40 bytes saved in cipher.o. Each entry in the list is 4 bytes, so this is a win (in terms of binary size) only if there are 10 ciphers or less. I would guess that constrained applications are typically under 10 entries, but even then the savings aren't great.

@@ -2423,7 +2423,177 @@ const mbedtls_cipher_definition_t mbedtls_cipher_definitions[] =

#define NUM_CIPHERS (sizeof(mbedtls_cipher_definitions) / \
sizeof(mbedtls_cipher_definitions[0]))
int mbedtls_cipher_supported[NUM_CIPHERS];
const int mbedtls_cipher_supported[NUM_CIPHERS] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this go in cipher.c and be static? (And maybe save a few bytes by not giving it external linkage.)

@XavierChapron
Copy link
Author

However, now that I look at the result, I'm not so sure this is desirable. This saves some code, but also increases rodata by the same amount that is saved in BSS. So the binary is larger if there are many ciphers. The threshold depends on the code size that is saved. With (one particular version of) arm-none-eabi-gcc -mthumb -mcpu=cortex-m0plus -Os, I see 40 bytes saved in cipher.o. Each entry in the list is 4 bytes, so this is a win (in terms of binary size) only if there are 10 ciphers or less. I would guess that constrained applications are typically under 10 entries, but even then the savings aren't great.

That's up to you. As discussed here the saving is indeed not great in binary size (and if you have too many cipher, the binary size even increase) however, the RAM usage always decrease.

As also mentioned in the other PR, this is interesting only if:

  • the application is constraint in RAM size
  • the application either call mbedtls_cipher_list() or don't use -ffunction-sections linker option

And in my opinion, in that case, the real problem is not what this PR fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)
Projects
Status: In Development
Development

Successfully merging this pull request may close these issues.

3 participants