subsystem: settings: its: Add ITS backend

[seankyer]

This PR adds an internal trusted storage (ITS) backend to settings subsystem, allowing users to store persistent data utilizing TF-M ITS transparently through the settings subsystem.

This allows for a non-secure app which may normally not have access to the Flash controller, for security reasons, to access persistent storage.

[MaureenHelm]

@de-nordic @tomi-font can you take a look?

[tomi-font]

The description doesn't seem to quite say what this achieves.
The way I understand this is, this is meant to redirect settings to TF-M's ITS.

What is the intent exactly behind that? Is it to somewhat transparently secure whatever is stored in settings?

[seankyer]

The description doesn't seem to quite say what this achieves. The way I understand this is, this is meant to redirect settings to TF-M's ITS.

What is the intent exactly behind that? Is it to somewhat transparently secure whatever is stored in settings?

Just updated the PR description. You are right. The main point is so that a Zephyr app running in non-secure domain can make use of the settings subsystem by using TF-M ITS to store potentially sensitive information (BLE Keys/Addresses/etc). By defining it as a settings backend, it integrates with Bluetooth subsystem seamlessly.

[tomi-font]

Just updated the PR description. You are right. The main point is so that a Zephyr app running in non-secure domain can make use of the settings subsystem by using TF-M ITS to store potentially sensitive information (BLE Keys/Addresses/etc). By defining it as a settings backend, it integrates with Bluetooth subsystem seamlessly.

Though why wouldn't code store potentially sensitive information through the PSA ITS/PS APIs? Now that those APIs are available also when not using TF-M (through the secure storage subsystem), it's not limited to TF-M-only use cases.
For example I know Bluetooth Mesh has started using persistent keys in the PSA Crypto API, which makes use of the ITS under the hood. (cc @alxelax)

One additional potential concern is if starting to store every setting in the ITS, it will add a significant delay to the calls, especially if using TF-M (partly because of its rather slow ITS implementation). And I'm wondering if that could cause problems because of added delays or also the fact that some implementations integrate with flash drivers to schedule NVM writes.

I mean I know it's not the new default and just adding one settings backend option, but trying to think about how usable that would be and if there is some other, better solution (e.g. making relevant code start using the PSA ITS/PS APIs).

[seankyer]

Just updated the PR description. You are right. The main point is so that a Zephyr app running in non-secure domain can make use of the settings subsystem by using TF-M ITS to store potentially sensitive information (BLE Keys/Addresses/etc). By defining it as a settings backend, it integrates with Bluetooth subsystem seamlessly.

Though why wouldn't code store potentially sensitive information through the PSA ITS/PS APIs? Now that those APIs are available also when not using TF-M (through the secure storage subsystem), it's not limited to TF-M-only use cases. For example I know Bluetooth Mesh has started using persistent keys in the PSA Crypto API, which makes use of the ITS under the hood. (cc @alxelax)

One additional potential concern is if starting to store every setting in the ITS, it will add a significant delay to the calls, especially if using TF-M (partly because of its rather slow ITS implementation). And I'm wondering if that could cause problems because of added delays or also the fact that some implementations integrate with flash drivers to schedule NVM writes.

I mean I know it's not the new default and just adding one settings backend option, but trying to think about how usable that would be and if there is some other, better solution (e.g. making relevant code start using the PSA ITS/PS APIs).

Yeah, this PR doesn't enforce that you must be running in non-secure context. A secure build could also make use of this backend. Although I use the example of non-secure and Bluetooth since that was the inspiration for creating this backend. Of course with this method, other OS-services that use settings subsystem would immediately work for a non-secure build.

Performance is a con of PSA ITS. I considered that the actual write to storage will take some time and could block some time sensitive operation (Bluetooth pairing for example). So, I added the SETTINGS_ITS_LAZY_PERSIST_DELAY_MS Kconfig. The setting will immediately be represented in memory, but the actual scheduled write to flash can be configured depending on the use case.

I supposed this is more about choice, as you said it's not a default option. Maybe I should add some documentation to the C file explaining the situation in which you'd use this backend? Or do you think it's required?

[tomi-font]

I see. I think you could elaborate in the help message of the SETTINGS_ITS Kconfig option rather. Citing use case(s), limitations... Documenting this, basically.
I'm not against this change. I'll just wait for other security folks to have a look to see if someone raises concerns before giving this a proper review.

[seankyer]

I see. I think you could elaborate in the help message of the SETTINGS_ITS Kconfig option rather. Citing use case(s), limitations... Documenting this, basically. I'm not against this change. I'll just wait for other security folks to have a look to see if someone raises concerns before giving this a proper review.

Thanks. I will update that Kconfig help message.

[MaureenHelm]

I'm not against this change. I'll just wait for other security folks to have a look to see if someone raises concerns before giving this a proper review.

@ceolin @d3zd3z @dleach02 can you take a look?

[alxelax]

I guess this Settings backend will behave differently to the others since it has compile time configured available entities for writing.
Some Zephyr subsystems do not consider this limitation in their implementations. For example, Bluetooth Mesh stores replay protection cache in Settings. The cache size depends on network topology and not known in advance.

I'm sure this should be mentioned clearly in Zephyr's documentation. Until this PR all Settings backends were changeable without impact on workability of Settings' API users.
FYI @Laczen

IMHO, to hide keys it is better to use PSA API for key management with Persistent lifetime property (it is how PSA API has been designed to hide keys in persistent memory) rather to create custom solutions.

[Laczen]

I guess this Settings backend will behave differently to the others since it has compile time configured available entities for writing.

All settings backends have this limitations although it might not be directly visible. Settings on nvs only support 16383 settings to be stored (but normally the storage would be exhausted before that). Settings on zms only allows 16 entries with the same name hash value (a rogue device can easily use this to block further storage. Settings on file/fcb is limited by the available storage.

There is no real difference between the proposal and other settings backends.

Some Zephyr subsystems do not consider this limitation in their implementations. For example, Bluetooth Mesh stores >replay protection cache in Settings. The cache size depends on network topology and not known in advance.

The replay protection cache is IMHO a design flaw in btmesh because it supposes unlimited storage possibility. On small devices this unlimited storage possibility cannot be guaranteed.

I'm sure this should be mentioned clearly in Zephyr's documentation. Until this PR all Settings backends were changeable without impact on workability of Settings' API users. FYI @Laczen

The settings backend solutions in no way guarantee that everything that needs to be written can be written to it. When it is impossible to store something the settings subsystem returns that it cannot write the data. All subsystems should be able to handle this in a correct way.

[alxelax]

Some Zephyr subsystems do not consider this limitation in their implementations. For example, Bluetooth Mesh stores >replay protection cache in Settings. The cache size depends on network topology and not known in advance.

The replay protection cache is IMHO a design flaw in btmesh because it supposes unlimited storage possibility. On small devices this unlimited storage possibility cannot be guaranteed.

@Laczen, kind of arguable to me, if you chose wrong platform (low capacity) for complex network solution it is not problem of design. Naive to expect that 128kb flash device can manage hundreds node network as provisioner. Uncontrolled garbage collection and not ability to do it in background thread is a more serious settings design problem. It is not possible to solve by platform selection.
However, this is off-topic and not related to this PR. We can discuss it in separate topic if you wish.

[de-nordic]

Looks OK, the code is nicely commented.

@kartben Can you take a look at Kconfig helps, you are better at describing these things.

[Laczen]

Some Zephyr subsystems do not consider this limitation in their implementations. For example, Bluetooth Mesh stores >replay protection cache in Settings. The cache size depends on network topology and not known in advance.

The replay protection cache is IMHO a design flaw in btmesh because it supposes unlimited storage possibility. On small devices this unlimited storage possibility cannot be guaranteed.

@Laczen, kind of arguable to me, if you chose wrong platform (low capacity) for complex network solution it is not problem of design. Naive to expect that 128kb flash device can manage hundreds node network as provisioner. Uncontrolled garbage collection and not ability to do it in background thread is a more serious settings design problem. It is not possible to solve by platform selection. However, this is off-topic and not related to this PR. We can discuss it in separate topic if you wish.

@alxelax I only commented because you mentioned me. Maybe it is off-topic, but I don't think so. You stated that the PR should not be added because of a builtin limitation that is not considered by the btmesh subsystem for the replay protection cache. But in this case the btmesh subsystem is making some expectation that is not provided by the settings subsystem at all. The problem is not the size of the network (this is easily solved), the problem is that if a device somehow can block storage (e.g. by exhausting the storage) the replay protection will fail. It is up to the btmesh subsystem to solve this problem.

Other items like timely response for critical settings and or uncontrolled garbage collections are also valid issues where in the current state subsystems have to take into account that the settings subsystem has its limitations. I am no longer working on any of these issues, sorry.

[Copilot]

Copilot reviewed 4 out of 9 changed files in this pull request and generated no comments.

Files not reviewed (5)

• subsys/settings/Kconfig: Language not supported
• subsys/settings/src/CMakeLists.txt: Language not supported
• tests/subsys/settings/its/CMakeLists.txt: Language not supported
• tests/subsys/settings/its/prj.conf: Language not supported
• tests/subsys/settings/its/src/CMakeLists.txt: Language not supported

Comments suppressed due to low confidence (2)

subsys/settings/src/settings_its.c:55

• The format specifiers for 'write_size' and 'remaining' use '%d' even though they are of type size_t. Consider using '%zu' for size_t to ensure accurate logging.

LOG_ERR("Error storing %d bytes of metadata! Bytes Remaining: %d, status: %d", write_size, remaining, status);

subsys/settings/src/settings_its.c:67

• The use of '%d' for 'sizeof(entries)' (a size_t) and '%lld' for uid may lead to incorrect formatting. Consider using '%zu' for size_t and verifying the correct format for uid.

LOG_DBG("ITS entries stored successfully - bytes_saved: %d num_entries: %d max_uid: %lld", sizeof(entries), entries_count, uid);

[seankyer]

TBH I didn't investigate the failure reason, but is this expected?

Can you try again? I pushed an update that should fix it. I've verified functionality on my max board.

[valeriosetti]

TBH I didn't investigate the failure reason, but is this expected?

Can you try again? I pushed an update that should fix it. I've verified functionality on my max board.

Yes, I retested on the nrf5340dk and now it works fine. Thanks!

[seankyer]

@de-nordic @ceolin Can you re-review/refresh your +1s?
@tomi-font Can you take a final look as PR assignee if this is good to merge now?

[tomi-font]

Some comments about the commits.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
22.9% Duplication on New Code

See analysis details on SonarQube Cloud

[seankyer]

@tomi-font I cleaned up the git history.
+and fixed a missing copyright

[hakehuang]

build failure for NXP platform lpcxpresso55s69/lpc55s69/cpu0/ns with tests/subsys/settings/nvs

[seankyer]

build failure for NXP platform lpcxpresso55s69/lpc55s69/cpu0/ns with tests/subsys/settings/nvs

I have not had time to look into this yet, but I suspect it is because that board has defined kconfig in a defconfig somewhere. Setting TFM_PARTITION_INTERNAL_TRUSTED_STORAGE causes the settings subsystem to use its backend instead of the nvs backend.

That board either needs to not use that Kconfig when it does not intend to use the ITS backend, or we need to come up with a new Kconfig to enable settings_its (if there is a use-case to use TFM ITS without the settings ITS backend)

[tomi-font]

Thinking after the fact, but it would be good to have this select EXPERIMENTAL.

[tomi-font]

@seankyer

[seankyer]

For this and the other discussion, I opened #94819

[seankyer]

@tomi-font I wanted to ask you, based on the issue that came in where this backend was being built automatically when it shouldnt of been (#93475) should we change the if TFM_PARTITION_INTERNAL_TRUSTED_STORAGE back to if TFM_ITS or something settings namespaced?

Thinking about it, it is reasonable that the user app could be using ITS, but not need to use ITS for the settings backend. As it is, we kind of force that upon them. Thoughts?

[tomi-font]

Hmm, no we are not forcing it because there are other defaults before SETTINGS_TFM_ITS. In fact, all the other possible values except SETTINGS_CUSTOM and SETTINGS_NONE.
And SETTINGS_BACKEND depends on SETTINGS (because of the enclosing if SETTINGS). So I don't think there's much wrong done here.
Maybe the failing test was expecting SETTINGS_NONE. I would have a look at what the test is trying to achieve. A solution could be to just add CONFIG_SETTINGS_NONE=y to that particular board target's overlay file.
Or then to make sure there are no such other breakages you could remove this line completely.

Suggested change

default SETTINGS_TFM_ITS if TFM_PARTITION_INTERNAL_TRUSTED_STORAGE