mgmt: mcumgr: replace Tinycrypt by PSA #71947

tomi-font · 2024-04-25T13:19:40Z

As part of ongoing work to move away from TinyCrypt and towards PSA (#43712), make fs_mgmt use either PSA (when available) or MbedTLS (as a fallback) for SHA-256.

The use of PSA is guarded by CONFIG_MBEDTLS_PSA_CRYPTO_CLIENT which requires a locally-built PSA core for devices without TF-M.

nordicjm

I cannot accept this change. I understand that having multiple security backends is not great, but when I enable the existing tinycrypt mode for this feature, I get:

Memory region         Used Size  Region Size  %age Used
           FLASH:      208648 B       472 KB     43.17%
             RAM:       61280 B       256 KB     23.38%
        IDT_LIST:          0 GB        32 KB      0.00%

When I switch over to mbedtls I get:

Memory region         Used Size  Region Size  %age Used
           FLASH:      219436 B       472 KB     45.40%
             RAM:       61280 B       256 KB     23.38%
        IDT_LIST:          0 GB        32 KB      0.00%

This is an increase of 10.6KiB, for a simple sha256 hash. I don't know what the overhead of this is in tfm but the overhead without is not acceptable

carlescufi · 2024-04-26T07:49:23Z

More context to this discussion:
#8081
#43712

carlescufi · 2024-04-26T07:58:43Z

Would it be an option to compile this conditionally? i.e. by default mcumgr would use PSA APIs, since that is the direction the project has chosen, but with Kconfig one could still enable Tinycrypt.

tomi-font · 2024-04-26T08:04:23Z

In my opinion it could definitely be.

nandojve · 2024-04-26T14:06:09Z

Would it be an option to compile this conditionally? i.e. by default mcumgr would use PSA APIs, since that is the direction the project has chosen, but with Kconfig one could still enable Tinycrypt.

+1

ceolin · 2024-04-26T21:27:53Z

This is an increase of 10.6KiB, for a simple sha256 hash. I don't know what the overhead of this is in tfm but the overhead without is not acceptable

@nordicjm what is the target / configuration you tested it ? 10k is really too much, I have seen this building

west build -p -b nrf52840dk/nrf52840 tests/subsys/mgmt/mcumgr/fs_mgmt_hash_supported/ -- -DEXTRA_CONF_FILE=configuration/sha256.conf

But lets not jump into early conclusions, I have just disabled some additional features that were enabled by default when building with mbedTLS and the difference was way smaller.

Before any change (building with mbedTLS):

-- Zephyr version: 3.6.99 (/home/faceolin/p/zephyrproject/zephyr), build: v3.6.0-2908-gf366bd3fb3f0
[271/271] Linking C executable zephyr/zephyr.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       75496 B         1 MB      7.20%
             RAM:       14392 B       256 KB      5.49%
        IDT_LIST:          0 GB        32 KB      0.00%
Generating files from /home/faceolin/p/zephyrproject/zephyr/build/zephyr/zephyr.elf for board: nrf52840dk

After disable unnecessary options for this test:

 [265/265] Linking C executable zephyr/zephyr.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       65192 B         1 MB      6.22%
             RAM:       14392 B       256 KB      5.49%
        IDT_LIST:          0 GB        32 KB      0.00%
Generating files from /home/faceolin/p/zephyrproject/zephyr/build/zephyr/zephyr.elf for board: nrf52840dk

Without these changes (current upstream)

-- Zephyr version: 3.6.99 (/home/faceolin/p/zephyrproject/upstream-main), build: v3.6.0-3023-g7084662cc80b
[181/181] Linking C executable zephyr/zephyr.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       64772 B         1 MB      6.18%
             RAM:       14520 B       256 KB      5.54%
        IDT_LIST:          0 GB        32 KB      0.00%
Generating files from /home/faceolin/p/zephyrproject/upstream-main/build/zephyr/zephyr.elf for board: nrf52840dk

I don't think supporting TinyCrypt indefinitely is a proper solution (obviously, we don't want to be disruptive in a LTS release). We need to have a proper solution.

I understand the concern with additional resources needed, but there are also multiple benefits adopting it.

nordicjm · 2024-04-29T06:12:05Z

Tinycrypt itself I'm not so bothered about, the 10KiB jump is what I'm bothered about. In tinycrypt you enable it then have to enable what features you want, seemingly in mbedtls you enable it then have to go and disable features you don't want, that's not really useful and certainly not what other Kconfigs in zephyr subsystems do, you enable a feature with a set of minimal defaults then enable the additional ones you want, so if the size difference from that can be vastly reduced by having minimal defaults and a way for features to select what they need e.g. SHA256 which enables them then that's a path on the right track

nordicjm · 2024-04-29T06:44:59Z

Build string:

cmake -GNinja -DBOARD=nrf52840dk/nrf52840 -DOVERLAY_CONFIG="overlay-bt.conf" -DCONFIG_TINYCRYPT=y -DCONFIG_TINYCRYPT_SHA256=y -DCONFIG_MCUMGR_GRP_FS_CHECKSUM_HASH=y -DCONFIG_MCUMGR_GRP_FS_HASH_SHA256=y ..

ceolin · 2024-04-29T20:57:09Z

Tinycrypt itself I'm not so bothered about, the 10KiB jump is what I'm bothered about. In tinycrypt you enable it then have to enable what features you want, seemingly in mbedtls you enable it then have to go and disable features you don't want, that's not really useful and certainly not what other Kconfigs in zephyr subsystems do, you enable a feature with a set of minimal defaults then enable the additional ones you want, so if the size difference from that can be vastly reduced by having minimal defaults and a way for features to select what they need e.g. SHA256 which enables them then that's a path on the right track

I agree with you, it is non-sense pulling a bunch of functionalities that were not requested. My comment was to address the size increase issue. There will much likely be an increase with PSA but that must not be in this magnitude.
Regarding TinyCrypt, everyone should care about it since that is big security problem for the project.

zephyrbot · 2024-04-30T12:19:03Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

tomi-font · 2024-05-02T08:57:40Z

With the changes from #72078 that removes the default enabling of hash algorithms for MbedTLS, we get to the following differences when building smp_svr with overlay-bt.conf for nrf52840dk/nrf52840:

TinyCrypt (reference):

Memory region         Used Size  Region Size  %age Used
           FLASH:      208428 B       472 KB     43.12%
             RAM:       61248 B       256 KB     23.36%
        IDT_LIST:          0 GB        32 KB      0.00%

MbedTLS (+480 bytes):

Memory region         Used Size  Region Size  %age Used
           FLASH:      208908 B       472 KB     43.22%
             RAM:       61248 B       256 KB     23.36%
        IDT_LIST:          0 GB        32 KB      0.00%

And when using directly mbedtls_sha256 instead of mbedtls_md (which is now the case with my latest changes):

MbedTLS (+68 bytes):

Memory region         Used Size  Region Size  %age Used
           FLASH:      208496 B       472 KB     43.14%
             RAM:       61248 B       256 KB     23.36%
        IDT_LIST:          0 GB        32 KB      0.00%

Additionally, if compiling with overlay-fs.conf instead of overlay-bt.conf the difference shrinks a bit further. This is because currently the bluetooth subsystem pulls in TinyCrypt unconditionally.

Now, regarding the PSA case. As of now the PSA API will only be used when building with TF-M, in which case the crypto operations happen in TF-M. Because TF-M already has the code, the net difference in code size when going from TinyCrypt to PSA is negative.

Building with overlay-fs.conf for nrf5340dk/nrf5340/cpuapp/ns (to have TF-M):

TinyCrypt (reference):

Memory region         Used Size  Region Size  %age Used
           FLASH:       79540 B       192 KB     40.46%
             RAM:       16944 B       192 KB      8.62%
        IDT_LIST:          0 GB        32 KB      0.00%

PSA (-768 bytes):

Memory region         Used Size  Region Size  %age Used
           FLASH:       78772 B       192 KB     40.07%
             RAM:       16944 B       192 KB      8.62%
        IDT_LIST:          0 GB        32 KB      0.00%

de-nordic

Looks good, thanks!

It is meant specifically for configuration of the PSA crypto library. The underlying PSA configuration items are guarded by the condition that a PSA crypto provider must be present, which is the case when either TF-M is in use or MbedTLS's PSA core is built as part of the application image. Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

Exclude two more platforms from the tests. They provoke devicetree-related build errors which weren't introduced by the changes in this PR (zephyrproject-rtos#71947). Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

tomi-font · 2024-05-10T06:53:59Z

Rebased and resolved conflicts.

Exclude two more platforms from the tests. They provoke devicetree-related build errors which weren't introduced by the changes in this PR (zephyrproject-rtos#71947). Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

As part of ongoing work to move away from TinyCrypt and towards PSA (zephyrproject-rtos#43712), make fs_mgmt use either PSA (when available) or MbedTLS (as a fallback) for SHA-256. The use of PSA is guarded by CONFIG_MBEDTLS_PSA_CRYPTO_CLIENT which requires a locally-built PSA core for devices without TF-M. Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

Exclude two more platforms from the tests. They provoke devicetree-related build errors which weren't introduced by the changes in this PR (zephyrproject-rtos#71947). Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

Exclude some more platforms from the tests. They provoke devicetree-related build errors which weren't introduced by the changes in this PR (zephyrproject-rtos#71947). Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

tomi-font · 2024-05-10T09:21:37Z

All CI failures should be addressed now.

Exclude some more platforms from the tests. They provoke devicetree-related build errors which weren't introduced by the changes in this PR (zephyrproject-rtos#71947). Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

zephyrbot added area: mcumgr area: Crypto / RNG labels Apr 25, 2024

zephyrbot requested review from ceolin, d3zd3z, de-nordic and nordicjm April 25, 2024 13:20

zephyrbot assigned nordicjm Apr 25, 2024

tomi-font requested a review from ithinuel April 25, 2024 13:20

nordicjm requested changes Apr 25, 2024

View reviewed changes

nordicjm mentioned this pull request Apr 26, 2024

PSA Crypto API adoption in Zephyr #43712

Open

tomi-font mentioned this pull request Apr 29, 2024

modules: mbedtls: remove default-enabling of hash algorithms #72078

Merged

tomi-font marked this pull request as draft April 30, 2024 10:38

tomi-font force-pushed the psa_migration_subsys_mgmt_mcumgr branch from f366bd3 to a3c71b1 Compare April 30, 2024 12:17

zephyrbot added manifest manifest-mbedtls manifest-tf-m-tests manifest-trusted-firmware-m manifest-mcuboot DNM This PR should not be merged (Do Not Merge) labels Apr 30, 2024

tomi-font force-pushed the psa_migration_subsys_mgmt_mcumgr branch from a3c71b1 to f64f1aa Compare May 2, 2024 08:12

nordicjm previously approved these changes May 8, 2024

View reviewed changes

de-nordic previously approved these changes May 8, 2024

View reviewed changes

This was referenced May 8, 2024

tls_credentials: use PSA functions instead of MbedTLS for SHA256 computation #71830

Merged

net: tcp: use PSA functions for ISN generation instead of legacy MbedTLS ones #71827

Merged

tomi-font dismissed stale reviews from de-nordic and nordicjm via 500b25c May 10, 2024 06:51

tomi-font force-pushed the psa_migration_subsys_mgmt_mcumgr branch from 9b8fe02 to 500b25c Compare May 10, 2024 06:51

tomi-font requested review from de-nordic and nordicjm May 10, 2024 06:54

nordicjm previously approved these changes May 10, 2024

View reviewed changes

tomi-font dismissed nordicjm’s stale review via 4341b58 May 10, 2024 07:38

tomi-font force-pushed the psa_migration_subsys_mgmt_mcumgr branch from 500b25c to 4341b58 Compare May 10, 2024 07:38

tomi-font force-pushed the psa_migration_subsys_mgmt_mcumgr branch from 4341b58 to bf4a226 Compare May 10, 2024 08:19

de-nordic previously approved these changes May 10, 2024

View reviewed changes

tomi-font dismissed de-nordic’s stale review via fc67ac8 May 10, 2024 09:21

tomi-font force-pushed the psa_migration_subsys_mgmt_mcumgr branch from bf4a226 to fc67ac8 Compare May 10, 2024 09:21

tomi-font requested a review from de-nordic May 10, 2024 09:21

de-nordic approved these changes May 10, 2024

View reviewed changes

nordicjm approved these changes May 10, 2024

View reviewed changes

nashif merged commit 73da457 into zephyrproject-rtos:main May 10, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgmt: mcumgr: replace Tinycrypt by PSA #71947

mgmt: mcumgr: replace Tinycrypt by PSA #71947

tomi-font commented Apr 25, 2024 •

edited

Loading

nordicjm left a comment

carlescufi commented Apr 26, 2024

carlescufi commented Apr 26, 2024

tomi-font commented Apr 26, 2024 •

edited

Loading

nandojve commented Apr 26, 2024

ceolin commented Apr 26, 2024

nordicjm commented Apr 29, 2024

nordicjm commented Apr 29, 2024

ceolin commented Apr 29, 2024

zephyrbot commented Apr 30, 2024 •

edited

Loading

tomi-font commented May 2, 2024

de-nordic left a comment

tomi-font commented May 10, 2024

tomi-font commented May 10, 2024

mgmt: mcumgr: replace Tinycrypt by PSA #71947

mgmt: mcumgr: replace Tinycrypt by PSA #71947

Conversation

tomi-font commented Apr 25, 2024 • edited Loading

nordicjm left a comment

Choose a reason for hiding this comment

carlescufi commented Apr 26, 2024

carlescufi commented Apr 26, 2024

tomi-font commented Apr 26, 2024 • edited Loading

nandojve commented Apr 26, 2024

ceolin commented Apr 26, 2024

nordicjm commented Apr 29, 2024

nordicjm commented Apr 29, 2024

ceolin commented Apr 29, 2024

zephyrbot commented Apr 30, 2024 • edited Loading

tomi-font commented May 2, 2024

de-nordic left a comment

Choose a reason for hiding this comment

tomi-font commented May 10, 2024

tomi-font commented May 10, 2024

tomi-font commented Apr 25, 2024 •

edited

Loading

tomi-font commented Apr 26, 2024 •

edited

Loading

zephyrbot commented Apr 30, 2024 •

edited

Loading