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: 9e47ba20692227086d1d95b67a3274ea2f7514cb more detailssdk-nrf:
Github labels
List of changed files detected by CI (14)Outputs:ToolchainVersion: f51bdba1d9 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
| @@ -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.release[nrf7002dk/nrf5340/cpuapp]: High ROM usage: 811902[B] - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-17522/10) |
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: |
Add bl_storage.c to TF-M to reduce complexity on bl_storage.h. Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>
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 <markus.lassila@nordicsemi.no>
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... |
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
The PR is still lacking the following:
However, I would like to get feedback at this point as I am unsure whether the direction I have taken this is on line what we are intending to do in regards to bl_storage and T-FM. I am hoping to get comments for this at least from @frkv and @Vge0rge.