TF-M PSA template: Lock Approtect in network core #17093

MarkusLassila · 2024-08-30T09:04:29Z

Add support for locking the Approtect in nRF53 network core, when TF-M transforms from provisioning life-cycle-state (LCS) to secure LCS.

Needs following changes:
https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/30836
nrfconnect/sdk-mcuboot#330

Identified weak points that I am (especially) looking comments for:

PCD code partitioning
Usage of CONFIG_MCUBOOT_USE_ALL_AVAILABLE_RAM, which stores parts of the bootloader memory to non-secure.

NordicBuilder · 2024-08-30T09:16:19Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 9

Inputs:

Sources:

sdk-nrf: PR head: ff7c0cedb968a4693b81470d4148ede22709eae4
mcuboot: PR head: 1e05867f40c38cdc26c23e1518b88a3471aa25aa

more details

sdk-nrf:

PR head: ff7c0cedb968a4693b81470d4148ede22709eae4
merge base: c31c744bf6cf12bb01f0f401398f3dd12589d6ae
target head (main): 85307a9ca0fb6ba8c61f4bd05b7f866cde42612f
Diff

mcuboot:

PR head: 1e05867f40c38cdc26c23e1518b88a3471aa25aa
merge base: daf2946a0f07a14b57bd69d29ac4cdde6f810fb7
target head (main): 5db198194c10e7a99bad2742e87e8c503b6d2c60
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (19)

bootloader
│  ├── mcuboot
│  │  ├── boot
│  │  │  ├── bootutil
│  │  │  │  ├── src
│  │  │  │  │  │ loader.c
│  │  │  ├── zephyr
│  │  │  │  │ main.c
include
│  ├── dfu
│  │  ├── pcd.h
│  │  │ pcd_common.h
modules
│  ├── trusted-firmware-m
│  │  ├── Kconfig.tfm.pm
│  │  ├── tfm_boards
│  │  │  ├── common
│  │  │  │  │ nrf_provisioning.c
│  │  │  ├── partition
│  │  │  │  │ region_defs.h
samples
│  ├── nrf5340
│  │  ├── netboot
│  │  │  ├── src
│  │  │  │  │ main.c
│  ├── tfm
│  │  ├── tfm_psa_template
│  │  │  ├── Kconfig.sysbuild
│  │  │  ├── README.rst
│  │  │  ├── boards
│  │  │  │  ├── nrf5340dk_nrf5340_cpuapp_ns.conf
│  │  │  │  │ nrf5340dk_nrf5340_cpuapp_ns.overlay
│  │  │  ├── sysbuild.conf
│  │  │  ├── sysbuild
│  │  │  │  ├── b0n
│  │  │  │  │  │ prj.conf
│  │  │  │  ├── mcuboot
│  │  │  │  │  ├── boards
│  │  │  │  │  │  ├── nrf5340dk_nrf5340_cpuapp.conf
│  │  │  │  │  │  │ nrf5340dk_nrf5340_cpuapp.overlay
subsys
│  ├── pcd
│  │  ├── Kconfig
│  │  ├── src
│  │  │  │ pcd.c
west.yml

Outputs:

Toolchain

Version: 2aae60c2f9
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:2aae60c2f9_81ed5a52d6

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
❌ Build twister
- sdk-nrf test count: 973
◻️ Integration tests
- ◻️ test-fw-nrfconnect-boot
- ◻️ test-fw-nrfconnect-chip
- ◻️ test-fw-nrfconnect-nrf_crypto
- ◻️ test-fw-nrfconnect-tfm
- ◻️ test-fw-nrfconnect-zigbee
- ◻️ test-sdk-find-my
- ◻️ test-sdk-sidewalk
- ◻️ test-sdk-mcuboot
- ⚠️ test-fw-nrfconnect-fw-update

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_cloud
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_mosh
- test-fw-nrfconnect-nrf-iot_nrf_provisioning
- test-fw-nrfconnect-nrf-iot_positioning
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_serial_lte_modem
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-proprietary_esb
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-thread
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-pmic-samples
- test-sdk-wifi

Note: This message is automatically posted and updated by the CI

NordicBuilder · 2024-08-30T09:45:44Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

NordicBuilder · 2024-08-30T10:34:41Z

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

Name	Old Revision	New Revision	Diff
mcuboot	nrfconnect/sdk-mcuboot@`e66169a`	nrfconnect/sdk-mcuboot#330	nrfconnect/sdk-mcuboot#330/files

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

modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c

Vge0rge · 2024-09-09T10:24:36Z

Overall this looks good to me, the only concern that I have is with the CONFIG_MCUBOOT_USE_ALL_AVAILABLE_RAM option. As long as we don't encrypt the firmware I don't see it as an issue but if we choose to encrypt the firmware is there any chance that this will leave the encryption key in the non secure memory in the end. Can someone from MCUBOOT comment on this?

MarkusLassila · 2024-09-12T06:34:48Z

Overall this looks good to me, the only concern that I have is with the CONFIG_MCUBOOT_USE_ALL_AVAILABLE_RAM option. As long as we don't encrypt the firmware I don't see it as an issue but if we choose to encrypt the firmware is there any chance that this will leave the encryption key in the non secure memory in the end. Can someone from MCUBOOT comment on this?

@hakonfam: I think that you had some thoughts about this. Could you comment on this?

hakonfam · 2024-09-17T10:12:33Z

@hakonfam: I think that you had some thoughts about this. Could you comment on this?

No comments from me on this point.

include/dfu/pcd.h

include/dfu/pcd_common.h

samples/nrf5340/netboot/src/main.c

modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c

modules/trusted-firmware-m/tfm_boards/partition/region_defs.h

MarkusLassila · 2024-09-18T06:30:18Z

Overall this looks good to me, the only concern that I have is with the CONFIG_MCUBOOT_USE_ALL_AVAILABLE_RAM option. As long as we don't encrypt the firmware I don't see it as an issue but if we choose to encrypt the firmware is there any chance that this will leave the encryption key in the non secure memory in the end.

@de-nordic: Could you comment on this?

Add a PCD command for locking the Approtect in B0n. Approtect will be locked from TF-M, which is separate from Zephyr OS. For this purpose, the relevant parts of PCD library are moved to a separate header, which can be used in both TF-M and Zephyr. If TF-M is used, delay locking of the PCD commands memory area to be done in TF-M. NCSDK-17920 Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>

With nRF53, allow the network core Approtect to be locked from TF-M. This is done when we are transitioning from provisioning LCS to secure LCS. NCSDK-17920 Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>

Add support for updating network core with nRF5340. External flash will be used for update images. NCSDK-17920 Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>

hakonfam

Approval only pertains pcd parts of the code, nothing related to TF-M.

SeppoTakalo · 2024-09-24T09:22:12Z

modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c

+ /* Wait 1 second for the network core to start up. */
+ NRFX_DELAY_US(USEC_IN_SEC);
+
+ /* Wait for the debug lock to complete. */
+ for (int i = 0; i < DEBUG_LOCK_TIMEOUT_MS; i++) {
+ if (!pcd_read_cmd_lock_debug()) {
+ break;
+ }
+ NRFX_DELAY_US(USEC_IN_MSEC);


One second seems a long time to wait for core to boot.
If we already have a delay in the for-loop, can we combine these to delays and just poll the core to see it booted, instead of waiting blindly one second before start to poll?

Suggested change

/* Wait 1 second for the network core to start up. */

NRFX_DELAY_US(USEC_IN_SEC);

/* Wait for the debug lock to complete. */

for (int i = 0; i < DEBUG_LOCK_TIMEOUT_MS; i++) {

if (!pcd_read_cmd_lock_debug()) {

break;

}

NRFX_DELAY_US(USEC_IN_MSEC);

/* Wait for the core to boot and debug lock to complete. */

for (int i = 0; i < DEBUG_LOCK_TIMEOUT_MS; i++) {

NRFX_DELAY_US(USEC_IN_MSEC);

if (!pcd_read_cmd_lock_debug()) {

break;

}

This is just an optimization... I don't really think there is anything wrong, other than it might be unnecessary slow to boot.

de-nordic · 2024-09-27T11:57:46Z

samples/tfm/tfm_psa_template/Kconfig.sysbuild

+config PM_EXTERNAL_FLASH_MCUBOOT_SECONDARY
+ default y
+
+config MCUBOOT_USE_ALL_AVAILABLE_RAM


Shouldn't the MCUBOOT_NRF_CLEANUP_NONSECURE_RAM be also y here?

This is supposed to call this:

before passing ctrl to application.

MarkusLassila requested review from SeppoTakalo and juhaylinen August 30, 2024 09:04

MarkusLassila requested review from hakonfam, frkv, Vge0rge, vili-nordic, anangl and rlubos as code owners August 30, 2024 09:04

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Aug 30, 2024

MarkusLassila changed the title ~~Tfm psa template add network core update~~ TF-M PSA template: Lock Approtect in network core Aug 30, 2024

MarkusLassila added the DNM label Aug 30, 2024

MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from 358ddbc to ee9cdc2 Compare August 30, 2024 10:34

MarkusLassila requested review from carlescufi and tejlmand as code owners August 30, 2024 10:34

github-actions bot added the manifest label Aug 30, 2024

NordicBuilder added the manifest-mcuboot label Aug 30, 2024

SeppoTakalo reviewed Aug 30, 2024

View reviewed changes

modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c Outdated Show resolved Hide resolved

SeppoTakalo reviewed Aug 30, 2024

View reviewed changes

modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c Outdated Show resolved Hide resolved

SeppoTakalo reviewed Aug 30, 2024

View reviewed changes

modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c Outdated Show resolved Hide resolved

MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch 2 times, most recently from dd2e330 to d79b499 Compare September 3, 2024 07:56

github-actions bot added the doc-required PR must not be merged without tech writer approval. label Sep 3, 2024

MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from d79b499 to 9827f06 Compare September 3, 2024 08:03

Vge0rge reviewed Sep 6, 2024

View reviewed changes

modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c Outdated Show resolved Hide resolved

MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from 9827f06 to cdb44e8 Compare September 9, 2024 06:23

hakonfam requested changes Sep 17, 2024

View reviewed changes

MarkusLassila requested review from de-nordic and removed request for vili-nordic September 18, 2024 06:29

MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from cdb44e8 to a3d3187 Compare September 18, 2024 06:56

MarkusLassila requested a review from tomi-font as a code owner September 18, 2024 06:56

tomi-font removed their request for review September 18, 2024 07:15

MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from a3d3187 to f9c96b2 Compare September 18, 2024 08:20

MarkusLassila added 3 commits September 18, 2024 13:06

tfm: Lock Approtect in network core

c99e0ad

With nRF53, allow the network core Approtect to be locked from TF-M. This is done when we are transitioning from provisioning LCS to secure LCS. NCSDK-17920 Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>

samples: tfm: psa_template: Add support for netcore update

ff7c0ce

Add support for updating network core with nRF5340. External flash will be used for update images. NCSDK-17920 Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>

MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from f9c96b2 to ff7c0ce Compare September 18, 2024 10:07

MarkusLassila requested review from hakonfam and SeppoTakalo September 18, 2024 10:10

hakonfam approved these changes Sep 24, 2024

View reviewed changes

SeppoTakalo reviewed Sep 24, 2024

View reviewed changes

SeppoTakalo approved these changes Sep 24, 2024

View reviewed changes

de-nordic reviewed Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TF-M PSA template: Lock Approtect in network core #17093

TF-M PSA template: Lock Approtect in network core #17093

MarkusLassila commented Aug 30, 2024 •

edited

Loading

NordicBuilder commented Aug 30, 2024 •

edited

Loading

NordicBuilder commented Aug 30, 2024

NordicBuilder commented Aug 30, 2024 •

edited

Loading

Vge0rge commented Sep 9, 2024

MarkusLassila commented Sep 12, 2024

hakonfam commented Sep 17, 2024

MarkusLassila commented Sep 18, 2024

hakonfam left a comment

SeppoTakalo Sep 24, 2024

de-nordic Sep 27, 2024

de-nordic Sep 27, 2024

TF-M PSA template: Lock Approtect in network core #17093

Are you sure you want to change the base?

TF-M PSA template: Lock Approtect in network core #17093

Conversation

MarkusLassila commented Aug 30, 2024 • edited Loading

NordicBuilder commented Aug 30, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

NordicBuilder commented Aug 30, 2024

NordicBuilder commented Aug 30, 2024 • edited Loading

Vge0rge commented Sep 9, 2024

MarkusLassila commented Sep 12, 2024

hakonfam commented Sep 17, 2024

MarkusLassila commented Sep 18, 2024

hakonfam left a comment

Choose a reason for hiding this comment

SeppoTakalo Sep 24, 2024

Choose a reason for hiding this comment

de-nordic Sep 27, 2024

Choose a reason for hiding this comment

de-nordic Sep 27, 2024

Choose a reason for hiding this comment

MarkusLassila commented Aug 30, 2024 •

edited

Loading

NordicBuilder commented Aug 30, 2024 •

edited

Loading

NordicBuilder commented Aug 30, 2024 •

edited

Loading