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

secure_storage: add a ZMS-based implementation of the ITS store module #81235

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion subsys/fs/zms/zms.c
Original file line number Diff line number Diff line change
Expand Up @@ -1362,7 +1362,7 @@ int zms_mount(struct zms_fs *fs)
* 1 close ATE, 1 empty ATE, 1 GC done ATE, 1 Delete ATE, 1 ID/Value ATE
*/
if (fs->sector_size < ZMS_MIN_ATE_NUM * fs->ate_size) {
LOG_ERR("Invalid sector size, should be at least %u",
LOG_ERR("Invalid sector size, should be at least %zu",
ZMS_MIN_ATE_NUM * fs->ate_size);
}

Expand Down
41 changes: 38 additions & 3 deletions subsys/secure_storage/Kconfig.its_store
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,33 @@
choice SECURE_STORAGE_ITS_STORE_IMPLEMENTATION
prompt "ITS store module implementation"

DT_ITS_PARTITION := $(dt_chosen_path,secure_storage_its_partition)
DT_CHOSEN_Z_SETTINGS_PARTITION := zephyr,settings-partition
DT_SETTINGS_PARTITIION := $(dt_chosen_path,$(DT_CHOSEN_Z_SETTINGS_PARTITION))
DT_STORAGE_PARTITION := $(dt_nodelabel_path,storage_partition)

config SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_ZMS
bool "ITS store module implementation using ZMS for storage"
depends on FLASH_HAS_DRIVER_ENABLED \
&& $(dt_path_enabled,$(DT_ITS_PARTITION)) \
&& $(dt_node_has_compat,$(dt_node_parent,$(DT_ITS_PARTITION)),fixed-partitions)
select ZMS
help
This implementation of the ITS store module makes direct use of ZMS for storage.
It needs a `secure_storage_its_partition` devicetree chosen property that points
to a fixed storage partition that will be dedicated to the ITS. It has lower
overhead compared to the settings-based implementation, both in terms of runtime
execution and storage space, and also ROM footprint if the settings subsystem is disabled.
As this implementations directly maps the PSA storage UIDs to ZMS entry IDs, it limits
their values to the first 30 bits.

config SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS
bool "ITS store module implementation using the settings subsystem for storage"
DT_STORAGE_PARTITION := $(dt_nodelabel_path,storage_partition)
depends on FLASH_HAS_DRIVER_ENABLED \
&& $(dt_path_enabled,$(DT_STORAGE_PARTITION)) \
&& $(dt_node_has_compat,$(dt_node_parent,$(DT_STORAGE_PARTITION)),fixed-partitions)
&& (($(dt_path_enabled,$(DT_SETTINGS_PARTITIION)) \
&& $(dt_node_has_compat,$(dt_node_parent,$(DT_SETTINGS_PARTITIION)),fixed-partitions))\
|| ($(dt_path_enabled,$(DT_STORAGE_PARTITION)) \
&& $(dt_node_has_compat,$(dt_node_parent,$(DT_STORAGE_PARTITION)),fixed-partitions)))
rghaddab marked this conversation as resolved.
Show resolved Hide resolved
imply FLASH_MAP
imply NVS
select SETTINGS
Expand All @@ -25,6 +46,20 @@ config SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_CUSTOM

endchoice # SECURE_STORAGE_ITS_STORE_IMPLEMENTATION

if SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_ZMS

config SECURE_STORAGE_ITS_STORE_ZMS_SECTOR_SIZE
int "Sector size of the ZMS partition"
default 4096
help
The sector size impacts the runtime behavior of ZMS and restricts the maximum
ITS entry data size (which is the sector size minus ZMS and ITS overhead).
Changing it will result in loss of existing data stored on a partition.
It must be a multiple of the flash page size on devices that require an erase.
See the ZMS documentation for more information.

endif # SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_ZMS

if SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS

config SECURE_STORAGE_ITS_STORE_SETTINGS_PREFIX
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ typedef enum {
SECURE_STORAGE_ITS_CALLER_PSA_ITS,
SECURE_STORAGE_ITS_CALLER_PSA_PS,
SECURE_STORAGE_ITS_CALLER_MBEDTLS,
SECURE_STORAGE_ITS_CALLER_COUNT
} secure_storage_its_caller_id_t;

/** The UID (caller + entry IDs) of an ITS entry. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@
* @param data_length The number of bytes in `data`.
* @param data The data to store.
*
* @retval `PSA_SUCCESS` on success.
* @retval `PSA_ERROR_INSUFFICIENT_STORAGE` if there is insufficient storage space.
* @retval `PSA_ERROR_STORAGE_FAILURE` on any other failure.
* @return One of the return values of `psa_its_set()`.
*/
psa_status_t secure_storage_its_store_set(secure_storage_its_uid_t uid,
size_t data_length, const void *data);
Expand All @@ -34,9 +32,7 @@ psa_status_t secure_storage_its_store_set(secure_storage_its_uid_t uid,
* @param[out] data_length On success, the number of bytes written to `data`.
* May be less than `data_size`.
*
* @retval `PSA_SUCCESS` on success.
* @retval `PSA_ERROR_DOES_NOT_EXIST` if no entry with the given UID exists.
* @retval `PSA_ERROR_STORAGE_FAILURE` on any other failure.
* @return One of the return values of `psa_its_get()`.
*/
psa_status_t secure_storage_its_store_get(secure_storage_its_uid_t uid, size_t data_size,
void *data, size_t *data_length);
Expand Down
11 changes: 10 additions & 1 deletion subsys/secure_storage/src/its/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ if (NOT CONFIG_SECURE_STORAGE_ITS_TRANSFORM_AEAD_NO_INSECURE_KEY_WARNING)
endif()
endif()

zephyr_library_sources_ifdef(CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_ZMS
store/zms.c
)
zephyr_library_sources_ifdef(CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS
store_settings.c
store/settings.c
)
if (CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_NONE)
message(ERROR "
The secure storage ITS module is enabled but has no implementation.
(CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_NONE)
")
endif()
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

#ifdef CONFIG_SECURE_STORAGE_ITS_IMPLEMENTATION_ZEPHYR
#include <zephyr/secure_storage/its/transform.h>
BUILD_ASSERT(SECURE_STORAGE_ITS_TRANSFORM_MAX_STORED_DATA_SIZE <= SETTINGS_MAX_VAL_LEN);
#endif

LOG_MODULE_DECLARE(secure_storage, CONFIG_SECURE_STORAGE_LOG_LEVEL);
Expand All @@ -21,7 +20,7 @@ static int init_settings_subsys(void)
const int ret = settings_subsys_init();

if (ret) {
LOG_DBG("Failed to initialize the settings subsystem. (%d)", ret);
LOG_DBG("Failed. (%d)", ret);
}
return ret;
}
Expand Down Expand Up @@ -108,9 +107,7 @@ psa_status_t secure_storage_its_store_remove(secure_storage_its_uid_t uid)

make_name(uid, name);
ret = settings_delete(name);
if (ret) {
LOG_DBG("Failed to delete %s. (%d)", name, ret);
return PSA_ERROR_STORAGE_FAILURE;
}
return PSA_SUCCESS;

LOG_DBG("%s %s. (%d)", ret ? "Failed to delete" : "Deleted", name, ret);
return ret ? PSA_ERROR_STORAGE_FAILURE : PSA_SUCCESS;
}
122 changes: 122 additions & 0 deletions subsys/secure_storage/src/its/store/zms.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/* Copyright (c) 2024 Nordic Semiconductor
* SPDX-License-Identifier: Apache-2.0
*/
#include <zephyr/secure_storage/its/store.h>
#include <zephyr/logging/log.h>
#include <zephyr/fs/zms.h>
#include <zephyr/storage/flash_map.h>
#ifdef CONFIG_SECURE_STORAGE_ITS_IMPLEMENTATION_ZEPHYR
#include <zephyr/secure_storage/its/transform.h>
#endif

LOG_MODULE_DECLARE(secure_storage, CONFIG_SECURE_STORAGE_LOG_LEVEL);

BUILD_ASSERT(CONFIG_SECURE_STORAGE_ITS_STORE_ZMS_SECTOR_SIZE
> 2 * CONFIG_SECURE_STORAGE_ITS_MAX_DATA_SIZE);

#define PARTITION_DT_NODE DT_CHOSEN(secure_storage_its_partition)

static struct zms_fs s_zms = {
.flash_device = FIXED_PARTITION_NODE_DEVICE(PARTITION_DT_NODE),
.offset = FIXED_PARTITION_NODE_OFFSET(PARTITION_DT_NODE),
.sector_size = CONFIG_SECURE_STORAGE_ITS_STORE_ZMS_SECTOR_SIZE,
};

static int init_zms(void)
{
int ret;

s_zms.sector_count = FIXED_PARTITION_NODE_SIZE(PARTITION_DT_NODE) / s_zms.sector_size;

ret = zms_mount(&s_zms);
if (ret) {
LOG_DBG("Failed. (%d)", ret);
}
return ret;
}
SYS_INIT(init_zms, APPLICATION, CONFIG_APPLICATION_INIT_PRIORITY);

/* Bit position of the ITS caller ID in the ZMS entry ID. */
#define ITS_CALLER_ID_POS 30
rghaddab marked this conversation as resolved.
Show resolved Hide resolved
/* Make sure that every ITS caller ID fits in ZMS entry IDs at the defined position. */
BUILD_ASSERT(1 << (32 - ITS_CALLER_ID_POS) >= SECURE_STORAGE_ITS_CALLER_COUNT);

static bool has_forbidden_bits_set(secure_storage_its_uid_t uid)
{
if (uid.uid & GENMASK64(63, ITS_CALLER_ID_POS)) {
LOG_DBG("UID %u/0x%llx cannot be used as it has bits set past "
"the first " STRINGIFY(ITS_CALLER_ID_POS) " ones.",
uid.caller_id, (unsigned long long)uid.uid);
return true;
}
return false;
}

static uint32_t zms_id_from(secure_storage_its_uid_t uid)
{
return (uint32_t)uid.uid | (uid.caller_id << ITS_CALLER_ID_POS);
rghaddab marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

uid.uid is a psa_storage_uid_t which is a uint64_t, this cannot be casted to a (uint32_t) without duplicate uses of zms_id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The result obtained from this is not used before uid is checked to not have any upper bits set (in has_forbidden_bits_set()), so here uid.uid can be considered to be (even less than) a uint32_t.

Copy link
Collaborator

@Laczen Laczen Nov 27, 2024

Choose a reason for hiding this comment

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

  1. Where is it documented that this specific implementation of the ITS store module has this limitation on bits?
  2. How to handle that one implementation of the ITS store module has this limitation, while another does not?
  3. Why use a 64bit uid when only 30bit are supported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. subsys/secure_storage/Kconfig.its_store:24
  2. By letting the users of the API only use a well-defined range of UIDs. Will come in a follow-up PR as stated previously.
  3. The 64-bit size originally comes from the specification. It could indeed be reduced when allowed values are officially defined.

}

psa_status_t secure_storage_its_store_set(secure_storage_its_uid_t uid,
size_t data_length, const void *data)
{
psa_status_t psa_ret;
ssize_t zms_ret;
const uint32_t zms_id = zms_id_from(uid);

if (has_forbidden_bits_set(uid)) {
return PSA_ERROR_INVALID_ARGUMENT;
}

zms_ret = zms_write(&s_zms, zms_id, data, data_length);
if (zms_ret == data_length) {
psa_ret = PSA_SUCCESS;
} else if (zms_ret == -ENOSPC) {
psa_ret = PSA_ERROR_INSUFFICIENT_STORAGE;
} else {
psa_ret = PSA_ERROR_STORAGE_FAILURE;
}
LOG_DBG("%s 0x%x with %zu bytes. (%zd)", (psa_ret == PSA_SUCCESS) ?
"Wrote" : "Failed to write", zms_id, data_length, zms_ret);
return psa_ret;
}

psa_status_t secure_storage_its_store_get(secure_storage_its_uid_t uid, size_t data_size,
void *data, size_t *data_length)
{
psa_status_t psa_ret;
ssize_t zms_ret;
const uint32_t zms_id = zms_id_from(uid);

if (has_forbidden_bits_set(uid)) {
return PSA_ERROR_INVALID_ARGUMENT;
}

zms_ret = zms_read(&s_zms, zms_id, data, data_size);
if (zms_ret > 0) {
*data_length = zms_ret;
psa_ret = PSA_SUCCESS;
} else if (zms_ret == -ENOENT) {
psa_ret = PSA_ERROR_DOES_NOT_EXIST;
} else {
psa_ret = PSA_ERROR_STORAGE_FAILURE;
}
LOG_DBG("%s 0x%x for up to %zu bytes. (%zd)", (psa_ret != PSA_ERROR_STORAGE_FAILURE) ?
"Read" : "Failed to read", zms_id, data_size, zms_ret);
return psa_ret;
}

psa_status_t secure_storage_its_store_remove(secure_storage_its_uid_t uid)
{
int zms_ret;
const uint32_t zms_id = zms_id_from(uid);

if (has_forbidden_bits_set(uid)) {
return PSA_ERROR_INVALID_ARGUMENT;
}

zms_ret = zms_delete(&s_zms, zms_id);
LOG_DBG("%s 0x%x. (%d)", zms_ret ? "Failed to delete" : "Deleted", zms_id, zms_ret);
BUILD_ASSERT(PSA_SUCCESS == 0);
return zms_ret;
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS=y

# 256 - flags (1) - CONFIG_SECURE_STORAGE_ITS_TRANSFORM_OUTPUT_OVERHEAD (28)
CONFIG_SECURE_STORAGE_ITS_MAX_DATA_SIZE=227

# Limit the space available for the maximum entry test to not take too long with NVS.
CONFIG_SETTINGS_NVS_SECTOR_COUNT=2
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
CONFIG_SECURE_STORAGE_ITS_TRANSFORM_IMPLEMENTATION_CUSTOM=y
CONFIG_SECURE_STORAGE_ITS_TRANSFORM_OUTPUT_OVERHEAD=0

# SETTINGS_MAX_VAL_LEN (256) - flags (1)
CONFIG_SECURE_STORAGE_ITS_MAX_DATA_SIZE=255
CONFIG_SECURE_STORAGE_ITS_MAX_DATA_SIZE=500
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,4 @@ CONFIG_TIMER_RANDOM_GENERATOR=y
CONFIG_COMMON_LIBC_MALLOC_ARENA_SIZE=2048
CONFIG_MBEDTLS_PSA_CRYPTO_C=y

# SETTINGS_MAX_VAL_LEN (256) - flags (1) - CONFIG_SECURE_STORAGE_ITS_TRANSFORM_OUTPUT_OVERHEAD (28)
CONFIG_SECURE_STORAGE_ITS_MAX_DATA_SIZE=227
CONFIG_SECURE_STORAGE_ITS_MAX_DATA_SIZE=300
2 changes: 1 addition & 1 deletion tests/subsys/secure_storage/psa/its/src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ ZTEST(secure_storage_psa_its, test_write_once_flag)
{
psa_status_t ret;
/* Use a UID that isn't used in the other tests for the write-once entry. */
const psa_storage_uid_t uid = ~UID;
const psa_storage_uid_t uid = 1 << 16;
const uint8_t data[MAX_DATA_SIZE] = {};
struct psa_storage_info_t info;

Expand Down
43 changes: 23 additions & 20 deletions tests/subsys/secure_storage/psa/its/testcase.yaml
Original file line number Diff line number Diff line change
@@ -1,42 +1,45 @@
common:
integration_platforms:
- native_sim
- nrf54l15dk/nrf54l15/cpuapp
platform_exclude:
- qemu_cortex_m0 # settings subsystem initialization fails
timeout: 600
tags:
- psa.secure_storage
tests:
secure_storage.psa.its.secure_storage:
filter: CONFIG_SECURE_STORAGE and not CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_NONE
secure_storage.psa.its.secure_storage.zms:
# DT-based filtering is not possible for this test scenario.
# Platforms with a storage_partition must be manually added here.
platform_allow: native_sim mps2/an385 qemu_x86/atom qemu_x86_64/atom
nrf54l15dk/nrf54l15/cpuapp nrf5340dk/nrf5340/cpuapp nrf52840dk/nrf52840
nrf9151dk/nrf9151 nrf9160dk/nrf9160 nrf9161dk/nrf9161
extra_args:
- EXTRA_DTC_OVERLAY_FILE=zms.overlay
- EXTRA_CONF_FILE=overlay-secure_storage.conf;overlay-transform_default.conf
- CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_ZMS=y

secure_storage.psa.its.secure_storage.settings.nvs:
filter: CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS
extra_args: "EXTRA_CONF_FILE=\
overlay-secure_storage.conf;overlay-default_transform.conf;overlay-default_store.conf"
integration_platforms:
- native_sim
- nrf54l15dk/nrf54l15/cpuapp
overlay-secure_storage.conf;overlay-transform_default.conf;overlay-store_settings.conf"

secure_storage.psa.its.secure_storage.custom.transform:
filter: CONFIG_SECURE_STORAGE and not CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_NONE
extra_args: "EXTRA_CONF_FILE=\
overlay-secure_storage.conf;overlay-custom_transform.conf;overlay-default_store.conf"
integration_platforms:
- native_sim
- nrf54l15dk/nrf54l15/cpuapp
extra_args: EXTRA_CONF_FILE=overlay-secure_storage.conf;overlay-transform_custom.conf

secure_storage.psa.its.secure_storage.custom.store:
filter: CONFIG_SECURE_STORAGE
extra_args: "EXTRA_CONF_FILE=\
overlay-secure_storage.conf;overlay-default_transform.conf;overlay-custom_store.conf"
integration_platforms:
- native_sim
- nrf54l15dk/nrf54l15/cpuapp
overlay-secure_storage.conf;overlay-transform_default.conf;overlay-store_custom.conf"

secure_storage.psa.its.secure_storage.custom.both:
filter: CONFIG_SECURE_STORAGE
extra_args: "EXTRA_CONF_FILE=\
overlay-secure_storage.conf;overlay-custom_transform.conf;overlay-custom_store.conf"
integration_platforms:
- native_sim
- nrf54l15dk/nrf54l15/cpuapp
overlay-secure_storage.conf;overlay-transform_custom.conf;overlay-store_custom.conf"

secure_storage.psa.its.tfm:
filter: CONFIG_BUILD_WITH_TFM
extra_args: EXTRA_CONF_FILE=overlay-tfm.conf
integration_platforms:
- nrf9151dk/nrf9151/ns
extra_args: EXTRA_CONF_FILE=overlay-tfm.conf
5 changes: 5 additions & 0 deletions tests/subsys/secure_storage/psa/its/zms.overlay
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
/ {
chosen {
secure_storage_its_partition = &storage_partition;
};
};
Loading