-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
TF-M PSA template: Lock Approtect in network core #17093
base: main
Are you sure you want to change the base?
TF-M PSA template: Lock Approtect in network core #17093
Conversation
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: ddda817191de65c0217413b77ea4462b01f5598c more detailssdk-nrf:
mcuboot:
Github labels
List of changed files detected by CI (22)
Outputs:ToolchainVersion: 3dd8985b56 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
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. |
358ddbc
to
ee9cdc2
Compare
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c
Outdated
Show resolved
Hide resolved
modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c
Outdated
Show resolved
Hide resolved
modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c
Outdated
Show resolved
Hide resolved
dd2e330
to
d79b499
Compare
d79b499
to
9827f06
Compare
modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c
Outdated
Show resolved
Hide resolved
9827f06
to
cdb44e8
Compare
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? |
@de-nordic: Could you comment on this? |
cdb44e8
to
a3d3187
Compare
f9c96b2
to
ff7c0ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval only pertains pcd parts of the code, nothing related to TF-M.
/* 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
/* 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.
config PM_EXTERNAL_FLASH_MCUBOOT_SECONDARY | ||
default y | ||
|
||
config MCUBOOT_USE_ALL_AVAILABLE_RAM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the MCUBOOT_NRF_CLEANUP_NONSECURE_RAM be also y here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing out this configuration option and apologies for slow response.
It seems that the SB_CONFIG_MCUBOOT_USE_ALL_AVAILABLE_RAM that we are setting in here, already sets the
CONFIG_MCUBOOT_NRF_CLEANUP_NONSECURE_RAM for bootloader in:
https://github.com/nrfconnect/sdk-nrf/blob/main/sysbuild/CMakeLists.txt#L164-L167
So it should be unnecessary to add CONFIG_MCUBOOT_NRF_CLEANUP_NONSECURE_RAM for the bootloader.
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>
ff7c0ce
to
56ca9eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also remember the changelog entry
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>
56ca9eb
to
ddda817
Compare
Does this require an update to https://github.com/nrfconnect/sdk-nrf/blob/main/doc/nrf/security/ap_protect.rst , @mia-ko and @MarkusLassila ? |
@greg-fer: I don't think that this PR in particular requires that the ap_protect.rst would be updated. The change is TF-M specific and the ap_protect.rst does not mention TF-M. That said, TF-M does lock the UICR.APPROTECT with some of the Kconfigs that are mentioned in there, but there is a bit of an internal discussion ongoing related to that, so I am hesitant to update that information in there at this point. I'll link you in the discussion. |
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:
-> Comments received.
-> CONFIG_MCUBOOT_NRF_CLEANUP_NONSECURE_RAM, which clears the non-secure ram is set by SB_CONFIG_MCUBOOT_USE_ALL_AVAILABLE_RAM.