Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,11 @@ Trusted Firmware-M (TF-M) samples

* Replaced support for the ``nrf54l15pdk/nrf54l15/cpuapp/ns`` board target with ``nrf54l15dk/nrf54l15/cpuapp/ns``.

* :ref:`tfm_psa_template` sample:

* Added support for updating the network core on the nRF5340 DK.


Thread samples
--------------

Expand Down
27 changes: 5 additions & 22 deletions include/dfu/pcd.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,37 +25,18 @@

#include <zephyr/device.h>
#include <sys/types.h>
#include <dfu/pcd_common.h>

#ifdef __cplusplus
extern "C" {
#endif

#ifdef CONFIG_SOC_SERIES_NRF53X

#ifdef CONFIG_PCD_CMD_ADDRESS

#define PCD_CMD_ADDRESS CONFIG_PCD_CMD_ADDRESS

#else

#include <pm_config.h>

#ifdef PM_PCD_SRAM_ADDRESS
#define PCD_CMD_ADDRESS PM_PCD_SRAM_ADDRESS
#else
/* extra '_' since its in a different domain */
#define PCD_CMD_ADDRESS PM__PCD_SRAM_ADDRESS
#endif /* PM_PCD_SRAM_ADDRESS */

#endif /* CONFIG_PCD_CMD_ADDRESS */

#endif /* CONFIG_SOC_SERIES_NRF53X */

enum pcd_status {
PCD_STATUS_COPY = 0,
PCD_STATUS_DONE = 1,
PCD_STATUS_FAILED = 2,
PCD_STATUS_READ_VERSION = 3,
PCD_STATUS_LOCK_DEBUG = 4,
};

/** @brief Sets up the PCD command structure with the location and size of the
Expand Down Expand Up @@ -87,8 +68,10 @@ int pcd_network_core_update(const void *src_addr, size_t len);
int pcd_network_core_update_initiate(const void *src_addr, size_t len);

/** @brief Lock the RAM section used for IPC with the network core bootloader.
*
* @param lock_conf Lock configuration until next SoC reset.
*/
void pcd_lock_ram(void);
void pcd_lock_ram(bool lock_conf);
MarkusLassila marked this conversation as resolved.
Show resolved Hide resolved

/** @brief Update the PCD CMD to indicate that the operation has completed
* successfully.
Expand Down
83 changes: 83 additions & 0 deletions include/dfu/pcd_common.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
*/

/** @file pcd_common.h
*
* @ingroup pcd
* @{
* @brief Common definitions for the PCD API.
*
* Common definitions are split out from the main PCD API to allow usage
* from non-Zephyr code.
*/

#ifndef PCD_COMMON_H__
#define PCD_COMMON_H__

#ifndef CONFIG_SOC_SERIES_NRF53X
#error "PCD is only supported on nRF53 series"
#endif

#ifdef CONFIG_PCD_CMD_ADDRESS
/* PCD command block location is static. */
#define PCD_CMD_ADDRESS CONFIG_PCD_CMD_ADDRESS

#else
/* PCD command block location is configured with Partition Manager. */
#include <pm_config.h>

#ifdef PM_PCD_SRAM_ADDRESS
/* PCD command block is in this domain, we are compiling for application core. */
#define PCD_CMD_ADDRESS PM_PCD_SRAM_ADDRESS
#else
/* PCD command block is in a different domain, we are compiling for network core.
* Extra '_' since its in a different domain.
*/
#define PCD_CMD_ADDRESS PM__PCD_SRAM_ADDRESS
#endif /* PM_PCD_SRAM_ADDRESS */

#endif /* CONFIG_PCD_CMD_ADDRESS */

/** Magic value written to indicate that a copy should take place. */
#define PCD_CMD_MAGIC_COPY 0xb5b4b3b6
/** Magic value written to indicate that debug should be locked. */
#define PCD_CMD_MAGIC_LOCK_DEBUG 0xb6f249ec
/** Magic value written to indicate that a something failed. */
#define PCD_CMD_MAGIC_FAIL 0x25bafc15
/** Magic value written to indicate that a copy is done. */
#define PCD_CMD_MAGIC_DONE 0xf103ce5d
/** Magic value written to indicate that a version number read should take place. */
#define PCD_CMD_MAGIC_READ_VERSION 0xdca345ea

struct pcd_cmd {
uint32_t magic; /* Magic value to identify this structure in memory */
const void *data; /* Data to copy*/
size_t len; /* Number of bytes to copy */
__INTPTR_TYPE__ offset; /* Offset to store the flash image in */
} __aligned(4);

#define PCD_CMD ((volatile struct pcd_cmd * const)(PCD_CMD_ADDRESS))

static inline void pcd_write_cmd_lock_debug(void)
{
*PCD_CMD = (struct pcd_cmd){
.magic = PCD_CMD_MAGIC_LOCK_DEBUG,
};
}

static inline bool pcd_read_cmd_done(void)
{
return PCD_CMD->magic == PCD_CMD_MAGIC_DONE;
}

static inline bool pcd_read_cmd_lock_debug(void)
{
return PCD_CMD->magic == PCD_CMD_MAGIC_LOCK_DEBUG;
}

#endif /* PCD_COMMON_H__ */

/**@} */
53 changes: 53 additions & 0 deletions modules/trusted-firmware-m/tfm_boards/common/nrf_provisioning.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,51 @@
#include "nrf_provisioning.h"
#include <identity_key.h>
#include <tfm_spm_log.h>
#include <pm_config.h>
#if defined(NRF53_SERIES) && defined(PM_CPUNET_APP_ADDRESS)
#include <dfu/pcd_common.h>
#include <spu.h>
#include <hal/nrf_reset.h>

#define DEBUG_LOCK_TIMEOUT_MS 3000
#define USEC_IN_MSEC 1000
MarkusLassila marked this conversation as resolved.
Show resolved Hide resolved
#define USEC_IN_SEC 1000000

static enum tfm_plat_err_t disable_netcore_debug(void)
{
/* NRF_RESET to secure.
* It will be configured to the original value after the provisioning is done.
*/
spu_peripheral_config_secure(NRF_RESET_S_BASE, SPU_LOCK_CONF_UNLOCKED);

/* Ensure that the network core is stopped. */
nrf_reset_network_force_off(NRF_RESET, true);

/* Debug lock command will be read in b0n startup. */
pcd_write_cmd_lock_debug();

/* Start the network core. */
nrf_reset_network_force_off(NRF_RESET, false);

/* 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);
Comment on lines +45 to +53
Copy link
Contributor

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?

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.

}

if (!pcd_read_cmd_done()) {
SPMLOG_ERRMSG("Failed to lock debug in network core.");
return TFM_PLAT_ERR_SYSTEM_ERR;
}

return TFM_PLAT_ERR_SUCCESS;
}
#endif /* NRF53_SERIES && PM_CPUNET_APP_ADDRESS */

static enum tfm_plat_err_t verify_debug_disabled(void)
{
Expand Down Expand Up @@ -71,10 +116,18 @@ enum tfm_plat_err_t tfm_plat_provisioning_perform(void)
* that secure boot is already enabled at this stage
*/

/* Application debug should already be disabled */
if (verify_debug_disabled() != TFM_PLAT_ERR_SUCCESS) {
return TFM_PLAT_ERR_SYSTEM_ERR;
}

#if defined(NRF53_SERIES) && defined(PM_CPUNET_APP_ADDRESS)
/* Disable network core debug in here */
if (disable_netcore_debug() != TFM_PLAT_ERR_SUCCESS) {
return TFM_PLAT_ERR_SYSTEM_ERR;
}
#endif

/* Transition to the SECURED lifecycle state */
if (tfm_attest_update_security_lifecycle_otp(TFM_SLC_SECURED) != 0) {
return TFM_PLAT_ERR_SYSTEM_ERR;
Expand Down
10 changes: 5 additions & 5 deletions modules/trusted-firmware-m/tfm_boards/partition/region_defs.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,23 +151,23 @@

#ifdef PM_MCUBOOT_ADDRESS
#define REGION_MCUBOOT_ADDRESS PM_MCUBOOT_ADDRESS
#define REGION_MCUBOOT_END_ADDRESS PM_MCUBOOT_END_ADDRESS
#define REGION_MCUBOOT_LIMIT PM_MCUBOOT_END_ADDRESS - 1
MarkusLassila marked this conversation as resolved.
Show resolved Hide resolved
#endif
#ifdef PM_B0_ADDRESS
#define REGION_B0_ADDRESS PM_B0_ADDRESS
#define REGION_B0_END_ADDRESS PM_B0_END_ADDRESS
#define REGION_B0_LIMIT PM_B0_END_ADDRESS - 1
#endif
#ifdef PM_S0_ADDRESS
#define REGION_S0_ADDRESS PM_S0_ADDRESS
#define REGION_S0_END_ADDRESS PM_S0_END_ADDRESS
#define REGION_S0_LIMIT PM_S0_END_ADDRESS - 1
#endif
#ifdef PM_S1_ADDRESS
#define REGION_S1_ADDRESS PM_S1_ADDRESS
#define REGION_S1_END_ADDRESS PM_S1_END_ADDRESS
#define REGION_S1_LIMIT PM_S1_END_ADDRESS - 1
#endif
#ifdef PM_PCD_SRAM_ADDRESS
#define REGION_PCD_SRAM_ADDRESS PM_PCD_SRAM_ADDRESS
#define REGION_PCD_SRAM_END_ADDRESS PM_PCD_SRAM_END_ADDRESS
#define REGION_PCD_SRAM_LIMIT PM_PCD_SRAM_END_ADDRESS - 1
#endif

#endif /* __REGION_DEFS_H__ */
31 changes: 27 additions & 4 deletions samples/nrf5340/netboot/src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
#include <dfu/pcd.h>
#include <zephyr/device.h>
#include <zephyr/devicetree.h>
#ifdef CONFIG_PCD_LOCK_NETCORE_APPROTECT
#include <nrfx_nvmc.h>
#endif

int main(void)
{
Expand All @@ -39,10 +42,26 @@ int main(void)

uint32_t s0_addr = s0_address_read();
bool valid = false;
uint8_t status = pcd_fw_copy_status_get();

switch (pcd_fw_copy_status_get()) {
#ifdef CONFIG_PCD_LOCK_NETCORE_DEBUG
MarkusLassila marked this conversation as resolved.
Show resolved Hide resolved
case PCD_STATUS_LOCK_DEBUG:
nrfx_nvmc_word_write((uint32_t)&NRF_UICR_NS->APPROTECT,
MarkusLassila marked this conversation as resolved.
Show resolved Hide resolved
nordicjm marked this conversation as resolved.
Show resolved Hide resolved
UICR_APPROTECT_PALL_Protected);
while (!nrfx_nvmc_write_done_check())
;

pcd_done();
Copy link
Contributor

Choose a reason for hiding this comment

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

whilst not needed for this PR (or any future PRs), I assume this could fail to write e.g. if the device is purposely ran from too low a voltage that flash writing is available at or a clock/voltage glitch is used to skip the write, and it does not check that the write was successful before booting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that your point holds. If you could pass the writing to the the UICR part, and still hit the pcd_done, it could be problematic. However, this particular piece of code should still be run by the manufacturer so that the device will transition form provisioning to secure life-cycle-state.


/* Success, waiting to be rebooted */
while (1)
;
CODE_UNREACHABLE;
break;
#endif

#ifdef CONFIG_PCD_READ_NETCORE_APP_VERSION
if (status == PCD_STATUS_READ_VERSION) {
case PCD_STATUS_READ_VERSION:
err = pcd_find_fw_version();
if (err < 0) {
printk("Unable to find valid firmware version %d\n\r", err);
Expand All @@ -54,10 +73,10 @@ int main(void)
while (1)
;
CODE_UNREACHABLE;
}
break;
#endif

if (status == PCD_STATUS_COPY) {
case PCD_STATUS_COPY:
/* First we validate the data where the PCD CMD tells
* us that we can find it.
*/
Expand Down Expand Up @@ -94,6 +113,10 @@ int main(void)
while (1)
;
CODE_UNREACHABLE;
break;

default:
break;
}

err = fprotect_area(PM_APP_ADDRESS, PM_APP_SIZE);
Expand Down
30 changes: 30 additions & 0 deletions samples/tfm/tfm_psa_template/Kconfig.sysbuild
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#
# Copyright (c) 2024 Nordic Semiconductor
#
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

source "${ZEPHYR_BASE}/share/sysbuild/Kconfig"

if BOARD_NRF5340DK_NRF5340_CPUAPP_NS

choice NETCORE
default NETCORE_EMPTY
endchoice

config SECURE_BOOT_NETCORE
default y

config NETCORE_APP_UPDATE
default y

config MCUBOOT_APP_SYNC_UPDATEABLE_IMAGES
default y

config PM_EXTERNAL_FLASH_MCUBOOT_SECONDARY
default y

config MCUBOOT_USE_ALL_AVAILABLE_RAM
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is supposed to call this:
image
before passing ctrl to application.

Copy link
Contributor Author

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.

default y

endif
Loading
Loading