tfm: Move TF-M attestation data to provisioned OTP region #17522
Conversation
MarkusLassila
requested review from
a team,
nordicjm and
tejlmand
as code owners
github-actions
bot
added
the
changelog-entry-required
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 760fdc483c9f628a1a610efcbf879e79a6aa9213 more detailssdk-nrf:
Github labels
List of changed files detected by CI (17)Outputs:ToolchainVersion: 342151af73 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. |
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
from
5f309ff to
956c258
Compare
include/bl_storage.h
Outdated
| #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. */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Functions moved to bl_storage.c.
| @@ -0,0 +1,38 @@ | |||
| # Copyright (c) 2024 Nordic Semiconductor | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sysbuild files go in sysbuild, this is the correct folder
include/bl_storage.h
Outdated
| @@ -336,6 +458,124 @@ NRFX_STATIC_INLINE int update_life_cycle_state(enum lcs next_lcs) | |||
| return -EINVALIDLCS; | |||
| } | |||
|
|
|||
| #if CONFIG_BUILD_WITH_TFM | |||
There was a problem hiding this comment.
This should is be defined in the TFM build I think.
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
6 times, most recently
from
12eebf3 to
db6ad7e
Compare
Memory footprint analysis revealed the following potential issuessample.matter.template.debug[nrf7002dk/nrf5340/cpuapp]: High ROM usage: 912202[B] - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-17522/27) |
Tried running: 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: |
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
from
db6ad7e to
9e47ba2
Compare
Yeah, unfortunately it's a little bit useless warning, because we never know which commit it applies to... |
| 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); |
There was a problem hiding this comment.
Is it intentional here that url_len does not contain NUL byte?
| uint32_t url_len = strlen(url); | |
| uint32_t url_len = sizeof(CONFIG_TFM_ATTEST_VERIFICATION_SERVICE_URL); |
There was a problem hiding this comment.
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.
| memcpy(buf, url, url_len); | ||
| *size = url_len; |
There was a problem hiding this comment.
If you used strlen( ) in the beginning, then memcpy() would only copy the string without the terminating NUL character.
There was a problem hiding this comment.
This is intentional.
| /* 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); |
There was a problem hiding this comment.
Is it really that we cannot guarantee the word alignment anymore?
There was a problem hiding this comment.
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.
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
from
9e47ba2 to
6b5e913
Compare
|
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. |
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
from
6b5e913 to
ad7a4f0
Compare
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
from
ad7a4f0 to
c15aa85
Compare
github-actions
bot
added
doc-required
|
Ping: @Vge0rge, @nrfconnect/ncs-pluto |
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst
Outdated
Show resolved
Hide resolved
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst
Outdated
Show resolved
Hide resolved
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
from
c15aa85 to
4b311e5
Compare
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
from
4b311e5 to
2cf35a2
Compare
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
from
2cf35a2 to
80f74a9
Compare
scripts/bootloader/provision.py
Outdated
| @@ -151,23 +172,77 @@ def get_hashes(public_key_files, verify_hashes): | |||
|
|
|||
| return hashes | |||
|
|
|||
| def get_variable_data(psa_certification_reference): | |||
| # Get variable data to be written to the OTP-region. | |||
There was a problem hiding this comment.
| # Get variable data to be written to the OTP-region. | |
| # Get variable data to be written to the OTP region. |
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
2 times, most recently
from
760301e to
1de188d
Compare
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
2 times, most recently
from
e9e934e to
4fd7c82
Compare
|
include/bl_storage.h
Outdated
| } | ||
| int update_life_cycle_state(enum lcs next_lcs); | ||
|
|
||
| #if CONFIG_BUILD_WITH_TFM |
There was a problem hiding this comment.
this means doc cannot be generated
There was a problem hiding this comment.
Do you mean that the documentation cannot be generated for the below functions? What would you suggest?
I added the check for TF-M, because it seemed it seemed to save some space on mcuboot on non-tfm builds.
There was a problem hiding this comment.
remove the #ifdef from the header file, it is enough to just have that in the .c file
| #elif defined(CONFIG_NRFX_RRAMC) | ||
| uint32_t word = nrfx_rramc_otp_word_read(index_from_address(address)); | ||
|
|
||
| if (!(address & 0x3)) { |
There was a problem hiding this comment.
nice to replace magic numbers with defines
| static void otp_copy32(uint8_t *restrict dst, uint32_t volatile * restrict src, | ||
| size_t size) | ||
| { | ||
| for (int i = 0; i < size / 4; i++) { |
There was a problem hiding this comment.
This function is removed in the next commit. Will fix on otp_copy.
There was a problem hiding this comment.
why are things being changed to be changed again in fixup commits?
| static void otp_copy32(uint8_t *restrict dst, uint32_t volatile * restrict src, | ||
| size_t size) |
There was a problem hiding this comment.
max line length is 100, this fits on one line with space to spare (plus for 2 lines the alignment of the second line is off)
There was a problem hiding this comment.
This function is removed in the next commit. Will fix on otp_copy.
| #define STATE_ENTERED 0x0000 | ||
| #define STATE_NOT_ENTERED 0xFFFF |
There was a problem hiding this comment.
defines at top of file with other defines
MarkusLassila
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
2 times, most recently
from
e802768 to
6be875a
Compare
Add bl_storage.c to TF-M to reduce complexity on bl_storage.h. Signed-off-by: Markus Lassila <[email protected]>
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
force-pushed
the
tfm-remove-deprecated-tf-m-otp-nv-counters
branch
from
6be875a to
760fdc4
Compare
|
Ping @nordicjm |
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