tfm: Move TF-M attestation data to provisioned OTP region #17522

MarkusLassila · 2024-09-27T08:49:37Z

Optional fields to TF-M attestation were previously stored in tfm_otp_nv_counters region, which we were not able to provision. This moves the psa_certification_reference to the provisioned OTP-region and adds support for accessing the variable data in
bl_storage.h.

Verification service URL and profile may change with device upgrades, for this reason they are added as Kconfigs.

Note that we still need to keep the tfm_otp_nv_counters region when TFM_PARTITION_PROTECTED_STORAGE and TFM_PS_ROLLBACK_PROTECTION are enabled. TF-M will increase monotonic counters every time new data is written and given the limited size of our OTP-region it would not support many updates.

NCSDK-17932

bl_storage tests -> Update: Variable data fields are currently only present with TF-M builds. With TF-M build, the application cannot access the provisioned data due provisioned data being secure. So bl_storage_tests are currently not possible.

NordicBuilder · 2024-09-27T08:52:18Z

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: 80f74a9c9e7e93affb1c977345cd857bcfa92fa1

more details

sdk-nrf:

PR head: 80f74a9c9e7e93affb1c977345cd857bcfa92fa1
merge base: ba929f92625962fe73cd6eae0a2daec3f0c119f9
target head (main): 6d1c94053cb10139f35ae0269ba6f737cf80142b
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 (16)

cmake
│  ├── sysbuild
│  │  │ provision_hex.cmake
doc
│  ├── nrf
│  │  ├── releases_and_maturity
│  │  │  ├── releases
│  │  │  │  │ release-notes-changelog.rst
include
│  │ bl_storage.h
modules
│  ├── trusted-firmware-m
│  │  ├── CMakeLists.txt
│  │  ├── Kconfig
│  │  ├── Kconfig.tfm.pm
│  │  ├── tfm_boards
│  │  │  ├── CMakeLists.txt
│  │  │  ├── common
│  │  │  │  │ attest_hal.c
│  │  │  ├── partition
│  │  │  │  │ flash_layout.h
samples
│  ├── tfm
│  │  ├── tfm_psa_template
│  │  │  ├── README.rst
│  │  │  ├── prj.conf
│  │  │  │ sysbuild.conf
scripts
│  ├── bootloader
│  │  │ provision.py
subsys
│  ├── bootloader
│  │  ├── bl_storage
│  │  │  │ bl_storage.c
sysbuild
│  ├── Kconfig.sysbuild
│  │ Kconfig.tfm

Outputs:

Toolchain

Version: b77d8c1312
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b77d8c1312_912848a074

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

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
❌ Integration tests
- ✅ test-sdk-audio
- ✅ desktop52_verification
- ❌ test-fw-nrfconnect-boot
- ✅ test-fw-nrfconnect-apps
- ✅ test_ble_nrf_config
- ✅ test-fw-nrfconnect-ble_mesh
- ✅ test-fw-nrfconnect-ble_samples
- ✅ test-fw-nrfconnect-chip
- ✅ test-fw-nrfconnect-nfc
- ✅ test-fw-nrfconnect-nrf-iot_libmodem-nrf
- ✅ test-fw-nrfconnect-nrf-iot_serial_lte_modem
- ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- ✅ test-fw-nrfconnect-nrf-iot_samples
- ✅ test-fw-nrfconnect-nrf-iot_lwm2m
- ✅ doc-internal
- ✅ test-fw-nrfconnect-nrf-iot_thingy91
- ❌ test-fw-nrfconnect-nrf_crypto
- ✅ test-fw-nrfconnect-rpc
- ❌ test-fw-nrfconnect-rs
- ✅ test-fw-nrfconnect-fem
- ✅ test-fw-nrfconnect-tfm
- ✅ test-fw-nrfconnect-thread
- ✅ test-fw-nrfconnect-zigbee
- ✅ test-sdk-find-my
- ✅ test-fw-nrfconnect-nrf-iot_mosh
- ✅ test-fw-nrfconnect-nrf-iot_positioning
- ✅ test-sdk-sidewalk
- ✅ test-sdk-wifi
- ✅ test-low-level
- ❌ test-fw-nrfconnect-nrf-iot_nrf_provisioning
- ✅ test-sdk-pmic-samples
- ✅ test-sdk-mcuboot
- ✅ test-sdk-dfu
- ✅ test-fw-nrfconnect-ps
- ✅ test-secdom-samples-public
- ⚠️ test-fw-nrfconnect-fw-update
- ⚠️ test-fw-nrfconnect-nrf-iot_cloud

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

NordicBuilder · 2024-09-27T09:08:23Z

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.

SeppoTakalo · 2024-10-14T10:07:58Z

include/bl_storage.h

+#if CONFIG_BUILD_WITH_TFM
+
+static uint32_t get_monotonic_counter_collection_size(const struct counter_collection *collection)
+{
+	/* Add only the constant part of the counter_collection. */


Why are these functions introduced inside header file? These are not marked as inline. Also, as these are static, it means that multiple implementations might end up in the flash. One for each .c file that uses these.

The functions are inside header file as they are used both in MCUboot and in application (TF-M). This is how it was previously achieved and I did not change this (yet). Having the functions inside bl_storage.c would mean that the bl_storage.c would be split with functions / headers that are available in MCUboot and functions / headers that are available in TF-M.

I guess we are already kind of doing the same in header file, so if you think that moving this split to bl_storage.c would be an improvement, I can give it a go.

Functions moved to bl_storage.c.

SeppoTakalo

Otherwise looks OK, but I don't understand why the implementation is inside header file and not in bl_storage.c

Vge0rge

Overall this looks good!
Some comments but I don't see anything seriously wrong with this.
Next time if you can please split such a change to more commits so that it is a bit more easy to follow.
I will give a second look to this before I approve.

Vge0rge · 2024-10-16T10:47:13Z

sysbuild/Kconfig.tfm

@@ -0,0 +1,38 @@
+# Copyright (c) 2024 Nordic Semiconductor


Doesn't that seem a bit out of place here?
I think that placing this in the same file with the rest of the TFM configurations makes it more logical to me:

sdk-nrf/modules/trusted-firmware-m/Kconfig

Line 473 in 9190d3a

I am not an expert on sysbuild but is the the only way to have this information to be visible by all the images in the build?

The provisioning is done for the MCUboot, so it does not see the Kconfigs of the application. It might be possible to change this to include Kconfigs of the application, but whether this makes sense would need input from @nordicjm.

sysbuild files go in sysbuild, this is the correct folder

Vge0rge · 2024-10-16T11:41:04Z

include/bl_storage.h

@@ -336,6 +458,124 @@ NRFX_STATIC_INLINE int update_life_cycle_state(enum lcs next_lcs)
 	return -EINVALIDLCS;
 }

+#if CONFIG_BUILD_WITH_TFM


This should is be defined in the TFM build I think.

include/bl_storage.h

NordicBuilder · 2024-11-13T14:39:13Z

Memory footprint analysis revealed the following potential issues

sample.matter.template.release[nrf7002dk/nrf5340/cpuapp]: High ROM usage: 820682[B] - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic)
sample.matter.template.debug[nrf7002dk/nrf5340/cpuapp]: High ROM usage: 911854[B] - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic)

Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-17522/17)

MarkusLassila · 2024-11-14T09:46:35Z

sample.matter.template.release

Tried running:
cd /ncs/nrf/samples/matter/template
west build -p -b nrf7002dk/nrf5340/cpuapp . -T sample.matter.template.release

With my commit and without my commits. There was no difference, except saving 14 bytes with b0n, so I think that the space warning is due to other commits on main branch.

I did have to disable the following:
CONFIG_CHIP_FACTORY_DATA=n
CONFIG_CHIP_FACTORY_DATA_BUILD=n
MATTER_FACTORY_DATA_GENERATE=n

kkasperczyk-no · 2024-11-15T10:39:58Z

so I think that the space warning is due to other commits on main branch.

Yeah, unfortunately it's a little bit useless warning, because we never know which commit it applies to...

SeppoTakalo

Looks OK in general.
I left few remarks.

include/bl_storage.h

SeppoTakalo · 2024-12-06T11:05:32Z

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

+enum tfm_plat_err_t tfm_attest_hal_get_verification_service(uint32_t *size, uint8_t *buf)
+{
+	const char *url = CONFIG_TFM_ATTEST_VERIFICATION_SERVICE_URL;
+	uint32_t url_len = strlen(url);


Is it intentional here that url_len does not contain NUL byte?

Suggested change

uint32_t url_len = strlen(url);

uint32_t url_len = sizeof(CONFIG_TFM_ATTEST_VERIFICATION_SERVICE_URL);

Yes. This is intentional. The data is fed to be CBOR encoded, which includes the length of the data. The null byte is not needed.

SeppoTakalo · 2024-12-06T11:07:22Z

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

+	memcpy(buf, url, url_len);
+	*size = url_len;


If you used strlen( ) in the beginning, then memcpy() would only copy the string without the terminating NUL character.

This is intentional.

scripts/bootloader/provision.py

subsys/bootloader/bl_storage/bl_storage.c

SeppoTakalo · 2024-12-06T11:27:04Z

subsys/bootloader/bl_storage/bl_storage.c

-	/* Ensure word alignment, since the data is stored in memory region
-	 * with word sized read limitation. Perform both build time and run
-	 * time asserts to catch the issue as soon as possible.
-	 */
-	BUILD_ASSERT(offsetof(struct bl_storage_data, key_data) % 4 == 0);
-	__ASSERT(((uint32_t)p_key % 4 == 0), "Key address is not word aligned");
-
-	otp_copy32(p_buf, (volatile uint32_t *restrict)p_key, SB_PUBLIC_KEY_HASH_LEN);
+	otp_copy(p_buf, p_key, SB_PUBLIC_KEY_HASH_LEN);


Is it really that we cannot guarantee the word alignment anymore?

I restored the check. I thought the check was originally added as the otp_copy32 required the alignment, but now I am unsure about this.

NordicBuilder · 2024-12-18T14:47:23Z

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 Publish GitHub Action.

Add bl_storage.c to TF-M to reduce complexity on bl_storage.h. Signed-off-by: Markus Lassila <[email protected]>

MarkusLassila · 2024-12-19T07:25:59Z

Ping: @Vge0rge, @nrfconnect/ncs-pluto

modules/trusted-firmware-m/Kconfig

sysbuild/Kconfig.tfm

include/bl_storage.h

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst

Vge0rge · 2024-12-22T22:13:13Z

subsys/bootloader/bl_storage/bl_storage.c

@@ -69,7 +135,11 @@ static bool key_is_valid(uint32_t key_idx)
 	}

 	/* Invalid value. */
+#if defined(CONFIG_BUILD_WITH_TFM)
+	abort();


I am curious, why abort instead of tfm_core_panic here?

Oversight. This can be tfm_core_panic, but it will need to be forward declared.

include/bl_storage.h

Vge0rge · 2024-12-22T23:42:51Z

scripts/bootloader/provision.py

@@ -39,7 +48,7 @@ def add_hw_counters(provision_data, num_counter_slots_version, mcuboot_counters_
    assert num_counter_slots_version % 2 == 0, "--num-counters-slots-version must be an even number"
    assert mcuboot_counters_slots    % 2 == 0, "--mcuboot-counters-slots     must be an even number"

-    provision_data += struct.pack('H', 1) # Type "counter collection"
+    provision_data += struct.pack('H', BL_COLLECTION_TYPE_MONOTONIC_COUNTERS)


This line is fine but I think that this function will not work for nRF54L15. The counters for this have 32 bit length but here in python when we add the counters in line 57 we do:
provision_data += bytes(2 * num_counter_slots_version * [0xFF])

Which should be wrong I think, we need to update the function.

Optional fields to TF-M attestation were previously stored in tfm_otp_nv_counters region, which we were not able to provision. This moves the psa_certification_reference to the provisioned OTP-region and adds support for accessing the variable data in bl_storage.h. Verification service URL and profile may change with device upgrades, for this reason they are added as Kconfigs. Note that we still need to keep the tfm_otp_nv_counters region when TFM_PARTITION_PROTECTED_STORAGE and TFM_PS_ROLLBACK_PROTECTION are enabled. TF-M will increase monotonic counters every time new data is written and given the limited size of our OTP-region it would not support many updates. NCSDK-17932 Signed-off-by: Markus Lassila <[email protected]>

MarkusLassila requested review from a team, nordicjm and tejlmand as code owners September 27, 2024 08:49

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

MarkusLassila force-pushed the tfm-remove-deprecated-tf-m-otp-nv-counters branch from 5f309ff to 956c258 Compare September 27, 2024 09:28

SeppoTakalo reviewed Oct 14, 2024

View reviewed changes

Vge0rge reviewed Oct 16, 2024

View reviewed changes

MarkusLassila force-pushed the tfm-remove-deprecated-tf-m-otp-nv-counters branch 6 times, most recently from 12eebf3 to db6ad7e Compare November 13, 2024 13:38

MarkusLassila requested review from frkv, Vge0rge and SeppoTakalo November 13, 2024 13:39

MarkusLassila force-pushed the tfm-remove-deprecated-tf-m-otp-nv-counters branch from db6ad7e to 9e47ba2 Compare November 14, 2024 09:51

SeppoTakalo approved these changes Dec 6, 2024

View reviewed changes

MarkusLassila force-pushed the tfm-remove-deprecated-tf-m-otp-nv-counters branch from 9e47ba2 to 6b5e913 Compare December 18, 2024 14:28

tfm: Include bl_storage.c

2f7cd3e

Add bl_storage.c to TF-M to reduce complexity on bl_storage.h. Signed-off-by: Markus Lassila <[email protected]>

MarkusLassila force-pushed the tfm-remove-deprecated-tf-m-otp-nv-counters branch 2 times, most recently from ad7a4f0 to c15aa85 Compare December 19, 2024 07:23

MarkusLassila requested review from a team as code owners December 19, 2024 07:23

github-actions bot added doc-required PR must not be merged without tech writer approval. and removed changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Dec 19, 2024

nordicjm reviewed Dec 19, 2024

View reviewed changes

modules/trusted-firmware-m/Kconfig Outdated Show resolved Hide resolved

nordicjm reviewed Dec 19, 2024

View reviewed changes

sysbuild/Kconfig.tfm Outdated Show resolved Hide resolved

peknis requested changes Dec 19, 2024

View reviewed changes

MarkusLassila force-pushed the tfm-remove-deprecated-tf-m-otp-nv-counters branch from c15aa85 to 4b311e5 Compare December 19, 2024 08:03

peknis approved these changes Dec 19, 2024

View reviewed changes

MarkusLassila force-pushed the tfm-remove-deprecated-tf-m-otp-nv-counters branch from 4b311e5 to 2cf35a2 Compare December 20, 2024 13:48

Vge0rge reviewed Dec 22, 2024

View reviewed changes

MarkusLassila force-pushed the tfm-remove-deprecated-tf-m-otp-nv-counters branch from 2cf35a2 to 80f74a9 Compare December 23, 2024 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tfm: Move TF-M attestation data to provisioned OTP region #17522

tfm: Move TF-M attestation data to provisioned OTP region #17522

MarkusLassila commented Sep 27, 2024 •

edited

Loading

NordicBuilder commented Sep 27, 2024 •

edited

Loading

NordicBuilder commented Sep 27, 2024

SeppoTakalo Oct 14, 2024

MarkusLassila Nov 8, 2024

MarkusLassila Nov 13, 2024

SeppoTakalo left a comment

Vge0rge left a comment

Vge0rge Oct 16, 2024

MarkusLassila Nov 8, 2024

nordicjm Nov 8, 2024

Vge0rge Oct 16, 2024

NordicBuilder commented Nov 13, 2024 •

edited

Loading

MarkusLassila commented Nov 14, 2024

kkasperczyk-no commented Nov 15, 2024

SeppoTakalo left a comment

SeppoTakalo Dec 6, 2024

MarkusLassila Dec 18, 2024

SeppoTakalo Dec 6, 2024

MarkusLassila Dec 18, 2024

SeppoTakalo Dec 6, 2024

MarkusLassila Dec 18, 2024

NordicBuilder commented Dec 18, 2024

MarkusLassila commented Dec 19, 2024

Vge0rge Dec 22, 2024

MarkusLassila Dec 23, 2024

Vge0rge Dec 22, 2024

	uint32_t url_len = strlen(url);
	uint32_t url_len = sizeof(CONFIG_TFM_ATTEST_VERIFICATION_SERVICE_URL);

tfm: Move TF-M attestation data to provisioned OTP region #17522

Are you sure you want to change the base?

tfm: Move TF-M attestation data to provisioned OTP region #17522

Conversation

MarkusLassila commented Sep 27, 2024 • edited Loading

NordicBuilder commented Sep 27, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

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

NordicBuilder commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeppoTakalo left a comment

Choose a reason for hiding this comment

Vge0rge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NordicBuilder commented Nov 13, 2024 • edited Loading

Memory footprint analysis revealed the following potential issues

MarkusLassila commented Nov 14, 2024

kkasperczyk-no commented Nov 15, 2024

SeppoTakalo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NordicBuilder commented Dec 18, 2024

MarkusLassila commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusLassila commented Sep 27, 2024 •

edited

Loading

NordicBuilder commented Sep 27, 2024 •

edited

Loading

NordicBuilder commented Nov 13, 2024 •

edited

Loading