From 9e9e1f60e262183d0a606c56bb0a6821d59f657a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 31 May 2024 18:38:36 +0200 Subject: [PATCH 01/15] Dynamic key store: new compilation option Create a new compilation option for a dynamically resized key store. The implementation will follow in subsequent commits. This option is off by default with custom configuration files, which is best for typical deployments on highly constrained platforms. This option is on by default with the provided configuration file, which is best for typical deployments on relatively high-end platforms. Signed-off-by: Gilles Peskine --- include/mbedtls/mbedtls_config.h | 23 ++++++++++++++++++- ...test_suite_psa_crypto_slot_management.data | 4 ++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 9382849f7c..e3fe02f151 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -1291,6 +1291,23 @@ */ //#define MBEDTLS_PSA_CRYPTO_SPM +/** + * \def MBEDTLS_PSA_KEY_STORE_DYNAMIC + * + * Dynamically resize the PSA key store to accommodate any number of + * volatile keys (until the heap memory is exhausted). + * + * If this option is disabled, the key store has a fixed size + * #MBEDTLS_PSA_KEY_SLOT_COUNT for volatile keys and loaded persistent keys + * together. + * + * This option has no effect when #MBEDTLS_PSA_CRYPTO_C is disabled. + * + * Module: library/psa_crypto.c + * Requires: MBEDTLS_PSA_CRYPTO_C + */ +#define MBEDTLS_PSA_KEY_STORE_DYNAMIC + /** * Uncomment to enable p256-m. This is an alternative implementation of * key generation, ECDH and (randomized) ECDSA on the curve SECP256R1. @@ -3884,9 +3901,13 @@ /** \def MBEDTLS_PSA_KEY_SLOT_COUNT * - * The maximum amount of PSA keys simultaneously in memory. This counts all + * When #MBEDTLS_PSA_KEY_STORE_DYNAMIC is disabled, + * the maximum amount of PSA keys simultaneously in memory. This counts all * volatile keys, plus loaded persistent keys. * + * When #MBEDTLS_PSA_KEY_STORE_DYNAMIC is enabled, + * the maximum number of loaded persistent keys. + * * Currently, persistent keys do not need to be loaded all the time while * a multipart operation is in progress, only while the operation is being * set up. This may change in future versions of the library. diff --git a/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.data b/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.data index af3b946754..1bf300ade6 100644 --- a/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.data +++ b/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.data @@ -129,9 +129,9 @@ depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C # writing, this happens in builds where AES uses a PSA driver and the # PSA RNG uses AES-CTR_DRBG through the PSA AES. # Pick a key id that's in the middle of the volatile key ID range. -# That works out both when MBEDTLS_PSA_KEY_SLOT_DYNAMIC is enabled and +# That works out both when MBEDTLS_PSA_KEY_STORE_DYNAMIC is enabled and # volatile key IDs are assigned starting with the lowest value, and when -# MBEDTLS_PSA_KEY_SLOT_DYNAMIC is disabled and volatile key IDs are assigned +# MBEDTLS_PSA_KEY_STORE_DYNAMIC is disabled and volatile key IDs are assigned # starting with the highest values. open_fail:(PSA_KEY_ID_VOLATILE_MIN + PSA_KEY_ID_VOLATILE_MAX) / 2:PSA_ERROR_DOES_NOT_EXIST From 54ee98c06787eae06681eb112770613b8d696add Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 7 Jun 2024 13:53:28 +0200 Subject: [PATCH 02/15] Dynamic key store: preparatory refactoring Add some abstractions around code that traverses the key store, in preparation for adding support for MBEDTLS_PSA_KEY_STORE_DYNAMIC. No intended behavior change. The generated machine code should be almost the same with an optimizing compiler (in principle, it could be the same with sufficient constant folding and inlining, but in practice it will likely be a few byes larger), Signed-off-by: Gilles Peskine --- .../core/psa_crypto_slot_management.c | 150 +++++++++++++----- 1 file changed, 109 insertions(+), 41 deletions(-) diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.c b/tf-psa-crypto/core/psa_crypto_slot_management.c index 9b297c9c08..7e0a1c12c1 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.c +++ b/tf-psa-crypto/core/psa_crypto_slot_management.c @@ -58,6 +58,11 @@ MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VOLATILE_MAX < MBEDTLS_PSA_KEY_ID_BUILTIN_MIN | +#define PERSISTENT_KEY_CACHE_COUNT MBEDTLS_PSA_KEY_SLOT_COUNT +#define KEY_SLICE_COUNT 1u +#define KEY_SLOT_CACHE_SLICE_INDEX 0 + + typedef struct { psa_key_slot_t key_slots[MBEDTLS_PSA_KEY_SLOT_COUNT]; uint8_t key_slots_initialized; @@ -65,10 +70,6 @@ typedef struct { static psa_global_data_t global_data; -MBEDTLS_STATIC_ASSERT(ARRAY_LENGTH(global_data.key_slots) <= - PSA_KEY_ID_VOLATILE_MAX - PSA_KEY_ID_VOLATILE_MIN + 1, - "The key slot array is larger than the volatile key ID range"); - static uint8_t psa_get_key_slots_initialized(void) { uint8_t initialized; @@ -86,6 +87,73 @@ static uint8_t psa_get_key_slots_initialized(void) return initialized; } + + +/** The length of the given slice in the key slot table. + * + * \param slice_idx The slice number. It must satisfy + * 0 <= slice_idx < KEY_SLICE_COUNT. + * + * \return The number of elements in the given slice. + */ +static inline size_t key_slice_length(size_t slice_idx); + +/** Get a pointer to the slot where the given volatile key is located. + * + * \param key_id The key identifier. It must be a valid volatile key + * identifier. + * \return A pointer to the only slot that the given key + * can be in. Note that the slot may be empty or + * contain a different key. + */ +static inline psa_key_slot_t *get_volatile_key_slot(psa_key_id_t key_id); + +/** Get a pointer to an entry in the persistent key cache. + * + * \param slot_idx The index in the table. It must satisfy + * 0 <= slot_idx < PERSISTENT_KEY_CACHE_COUNT. + * \return A pointer to the slot containing the given + * persistent key cache entry. + */ +static inline psa_key_slot_t *get_persistent_key_slot(size_t slot_idx); + +/** Get a pointer to a slot given by slice and index. + * + * \param slice_idx The slice number. It must satisfy + * 0 <= slice_idx < KEY_SLICE_COUNT. + * \param slot_idx An index in the given slice. It must satisfy + * 0 <= slot_idx < key_slice_length(slice_idx). + * + * \return A pointer to the given slot. + */ +static inline psa_key_slot_t *get_key_slot(size_t slice_idx, size_t slot_idx); + +static inline size_t key_slice_length(size_t slice_idx) +{ + (void) slice_idx; + return ARRAY_LENGTH(global_data.key_slots); +} + +static inline psa_key_slot_t *get_volatile_key_slot(psa_key_id_t key_id) +{ + MBEDTLS_STATIC_ASSERT(ARRAY_LENGTH(global_data.key_slots) <= + PSA_KEY_ID_VOLATILE_MAX - PSA_KEY_ID_VOLATILE_MIN + 1, + "The key slot array is larger than the volatile key ID range"); + return &global_data.key_slots[key_id - PSA_KEY_ID_VOLATILE_MIN]; +} + +static inline psa_key_slot_t *get_persistent_key_slot(size_t slot_idx) +{ + return &global_data.key_slots[slot_idx]; +} + +static inline psa_key_slot_t *get_key_slot(size_t slice_idx, size_t slot_idx) +{ + (void) slice_idx; + return &global_data.key_slots[slot_idx]; +} + + int psa_is_valid_key_id(mbedtls_svc_key_id_t key, int vendor_ok) { psa_key_id_t key_id = MBEDTLS_SVC_KEY_ID_GET_KEY_ID(key); @@ -147,7 +215,7 @@ static psa_status_t psa_get_and_lock_key_slot_in_memory( psa_key_slot_t *slot = NULL; if (psa_key_id_is_volatile(key_id)) { - slot = &global_data.key_slots[key_id - PSA_KEY_ID_VOLATILE_MIN]; + slot = get_volatile_key_slot(key_id); /* Check if both the PSA key identifier key_id and the owner * identifier of key match those of the key slot. */ @@ -162,8 +230,8 @@ static psa_status_t psa_get_and_lock_key_slot_in_memory( return PSA_ERROR_INVALID_HANDLE; } - for (slot_idx = 0; slot_idx < MBEDTLS_PSA_KEY_SLOT_COUNT; slot_idx++) { - slot = &global_data.key_slots[slot_idx]; + for (slot_idx = 0; slot_idx < PERSISTENT_KEY_CACHE_COUNT; slot_idx++) { + slot = get_persistent_key_slot(slot_idx); /* Only consider slots which are in a full state. */ if ((slot->state == PSA_SLOT_FULL) && (mbedtls_svc_key_id_equal(key, slot->attr.id))) { @@ -197,13 +265,13 @@ psa_status_t psa_initialize_key_slots(void) void psa_wipe_all_key_slots(void) { - size_t slot_idx; - - for (slot_idx = 0; slot_idx < MBEDTLS_PSA_KEY_SLOT_COUNT; slot_idx++) { - psa_key_slot_t *slot = &global_data.key_slots[slot_idx]; - slot->registered_readers = 1; - slot->state = PSA_SLOT_PENDING_DELETION; - (void) psa_wipe_key_slot(slot); + for (size_t slice_idx = 0; slice_idx < KEY_SLICE_COUNT; slice_idx++) { + for (size_t slot_idx = 0; slot_idx < key_slice_length(slice_idx); slot_idx++) { + psa_key_slot_t *slot = get_key_slot(slice_idx, slot_idx); + slot->registered_readers = 1; + slot->state = PSA_SLOT_PENDING_DELETION; + (void) psa_wipe_key_slot(slot); + } } /* The global data mutex is already held when calling this function. */ global_data.key_slots_initialized = 0; @@ -222,8 +290,8 @@ psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, } selected_slot = unused_persistent_key_slot = NULL; - for (slot_idx = 0; slot_idx < MBEDTLS_PSA_KEY_SLOT_COUNT; slot_idx++) { - psa_key_slot_t *slot = &global_data.key_slots[slot_idx]; + for (slot_idx = 0; slot_idx < PERSISTENT_KEY_CACHE_COUNT; slot_idx++) { + psa_key_slot_t *slot = get_key_slot(KEY_SLOT_CACHE_SLICE_INDEX, slot_idx); if (slot->state == PSA_SLOT_EMPTY) { selected_slot = slot; break; @@ -689,34 +757,34 @@ psa_status_t psa_purge_key(mbedtls_svc_key_id_t key) void mbedtls_psa_get_stats(mbedtls_psa_stats_t *stats) { - size_t slot_idx; - memset(stats, 0, sizeof(*stats)); - for (slot_idx = 0; slot_idx < MBEDTLS_PSA_KEY_SLOT_COUNT; slot_idx++) { - const psa_key_slot_t *slot = &global_data.key_slots[slot_idx]; - if (psa_key_slot_has_readers(slot)) { - ++stats->locked_slots; - } - if (slot->state == PSA_SLOT_EMPTY) { - ++stats->empty_slots; - continue; - } - if (PSA_KEY_LIFETIME_IS_VOLATILE(slot->attr.lifetime)) { - ++stats->volatile_slots; - } else { - psa_key_id_t id = MBEDTLS_SVC_KEY_ID_GET_KEY_ID(slot->attr.id); - ++stats->persistent_slots; - if (id > stats->max_open_internal_key_id) { - stats->max_open_internal_key_id = id; + for (size_t slice_idx = 0; slice_idx < KEY_SLICE_COUNT; slice_idx++) { + for (size_t slot_idx = 0; slot_idx < key_slice_length(slice_idx); slot_idx++) { + const psa_key_slot_t *slot = get_key_slot(slice_idx, slot_idx); + if (psa_key_slot_has_readers(slot)) { + ++stats->locked_slots; } - } - if (PSA_KEY_LIFETIME_GET_LOCATION(slot->attr.lifetime) != - PSA_KEY_LOCATION_LOCAL_STORAGE) { - psa_key_id_t id = MBEDTLS_SVC_KEY_ID_GET_KEY_ID(slot->attr.id); - ++stats->external_slots; - if (id > stats->max_open_external_key_id) { - stats->max_open_external_key_id = id; + if (slot->state == PSA_SLOT_EMPTY) { + ++stats->empty_slots; + continue; + } + if (PSA_KEY_LIFETIME_IS_VOLATILE(slot->attr.lifetime)) { + ++stats->volatile_slots; + } else { + psa_key_id_t id = MBEDTLS_SVC_KEY_ID_GET_KEY_ID(slot->attr.id); + ++stats->persistent_slots; + if (id > stats->max_open_internal_key_id) { + stats->max_open_internal_key_id = id; + } + } + if (PSA_KEY_LIFETIME_GET_LOCATION(slot->attr.lifetime) != + PSA_KEY_LOCATION_LOCAL_STORAGE) { + psa_key_id_t id = MBEDTLS_SVC_KEY_ID_GET_KEY_ID(slot->attr.id); + ++stats->external_slots; + if (id > stats->max_open_external_key_id) { + stats->max_open_external_key_id = id; + } } } } From 5c81b9403da3997f70d398c6e7bbc810356719a3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Jul 2024 20:49:36 +0200 Subject: [PATCH 03/15] Dynamic key store: disable full-key-store tests It's impractical to fill the key store when it can grow to accommodate millions of keys. A later commit will restore those tests in test configurations with the dynamic key store. Signed-off-by: Gilles Peskine --- ...test_suite_psa_crypto_slot_management.function | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.function b/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.function index f679f2e889..b2d3f29735 100644 --- a/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.function +++ b/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.function @@ -101,7 +101,11 @@ exit: /* Currently, there is always a maximum number of volatile keys that can * realistically be reached in tests. When we add configurations where this * is not true, undefine the macro in such configurations. */ +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) +#undef MAX_VOLATILE_KEYS +#else /* Static key store */ #define MAX_VOLATILE_KEYS MBEDTLS_PSA_KEY_SLOT_COUNT +#endif /* END_HEADER */ @@ -1028,7 +1032,7 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C */ +/* BEGIN_CASE depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C:!MBEDTLS_PSA_KEY_STORE_DYNAMIC */ void non_reusable_key_slots_integrity_in_case_of_key_slot_starvation() { psa_status_t status; @@ -1068,7 +1072,14 @@ void non_reusable_key_slots_integrity_in_case_of_key_slot_starvation() TEST_ASSERT(mbedtls_svc_key_id_equal(returned_key_id, persistent_key)); /* - * Create the maximum available number of volatile keys + * Create the maximum available number of keys that are locked in + * memory. This can be: + * - volatile keys, when MBEDTLS_PSA_KEY_STORE_DYNAMIC is disabled; + * - opened persistent keys (could work, but not currently implemented + * in this test function); + * - keys in use by another thread (we don't do this because it would + * be hard to arrange and we can't control how long the keys are + * locked anyway). */ psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_VOLATILE); for (i = 0; i < available_key_slots; i++) { From dc41fff6694569a517be7849b0e66b2dfccfe0f2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 10 Jun 2024 11:42:41 +0200 Subject: [PATCH 04/15] psa_key_slot_t: different fields in free vs occupied slots Place some fields of psa_key_slot_t in a union, to prepare for a new field in free slots that should not require extra memory. For occupied slots, place only the registered_readers field in the union, not other fields, to minimize textual changes. All fields could move to the union except state (also needed in free slots) and attr (which must stay first to reduce the code size, because it is accessed at many call sites). Signed-off-by: Gilles Peskine --- tf-psa-crypto/core/psa_crypto.c | 8 +-- tf-psa-crypto/core/psa_crypto_core.h | 62 +++++++++++-------- .../core/psa_crypto_slot_management.c | 10 +-- .../core/psa_crypto_slot_management.h | 4 +- 4 files changed, 46 insertions(+), 38 deletions(-) diff --git a/tf-psa-crypto/core/psa_crypto.c b/tf-psa-crypto/core/psa_crypto.c index 9ca592831f..c6f805ba2a 100644 --- a/tf-psa-crypto/core/psa_crypto.c +++ b/tf-psa-crypto/core/psa_crypto.c @@ -1210,15 +1210,15 @@ psa_status_t psa_wipe_key_slot(psa_key_slot_t *slot) case PSA_SLOT_PENDING_DELETION: /* In this state psa_wipe_key_slot() must only be called if the * caller is the last reader. */ - if (slot->registered_readers != 1) { - MBEDTLS_TEST_HOOK_TEST_ASSERT(slot->registered_readers == 1); + if (slot->var.occupied.registered_readers != 1) { + MBEDTLS_TEST_HOOK_TEST_ASSERT(slot->var.occupied.registered_readers == 1); status = PSA_ERROR_CORRUPTION_DETECTED; } break; case PSA_SLOT_FILLING: /* In this state registered_readers must be 0. */ - if (slot->registered_readers != 0) { - MBEDTLS_TEST_HOOK_TEST_ASSERT(slot->registered_readers == 0); + if (slot->var.occupied.registered_readers != 0) { + MBEDTLS_TEST_HOOK_TEST_ASSERT(slot->var.occupied.registered_readers == 0); status = PSA_ERROR_CORRUPTION_DETECTED; } break; diff --git a/tf-psa-crypto/core/psa_crypto_core.h b/tf-psa-crypto/core/psa_crypto_core.h index e7ff151a8b..a893c149ab 100644 --- a/tf-psa-crypto/core/psa_crypto_core.h +++ b/tf-psa-crypto/core/psa_crypto_core.h @@ -59,6 +59,8 @@ typedef enum { * and metadata for one key. */ typedef struct { + /* This field is accessed in a lot of places. Putting it first + * reduces the code size. */ psa_key_attributes_t attr; /* @@ -81,32 +83,38 @@ typedef struct { * PSA_SLOT_FULL. */ psa_key_slot_state_t state; - /* - * Number of functions registered as reading the material in the key slot. - * - * Library functions must not write directly to registered_readers - * - * A function must call psa_register_read(slot) before reading the current - * contents of the slot for an operation. - * They then must call psa_unregister_read(slot) once they have finished - * reading the current contents of the slot. If the key slot mutex is not - * held (when mutexes are enabled), this call must be done via a call to - * psa_unregister_read_under_mutex(slot). - * A function must call psa_key_slot_has_readers(slot) to check if - * the slot is in use for reading. - * - * This counter is used to prevent resetting the key slot while the library - * may access it. For example, such control is needed in the following - * scenarios: - * . In case of key slot starvation, all key slots contain the description - * of a key, and the library asks for the description of a persistent - * key not present in the key slots, the key slots currently accessed by - * the library cannot be reclaimed to free a key slot to load the - * persistent key. - * . In case of a multi-threaded application where one thread asks to close - * or purge or destroy a key while it is in use by the library through - * another thread. */ - size_t registered_readers; + union { + struct { + /* + * Number of functions registered as reading the material in the key slot. + * + * Library functions must not write directly to registered_readers + * + * A function must call psa_register_read(slot) before reading + * the current contents of the slot for an operation. + * They then must call psa_unregister_read(slot) once they have + * finished reading the current contents of the slot. If the key + * slot mutex is not held (when mutexes are enabled), this call + * must be done via a call to + * psa_unregister_read_under_mutex(slot). + * A function must call psa_key_slot_has_readers(slot) to check if + * the slot is in use for reading. + * + * This counter is used to prevent resetting the key slot while + * the library may access it. For example, such control is needed + * in the following scenarios: + * . In case of key slot starvation, all key slots contain the + * description of a key, and the library asks for the + * description of a persistent key not present in the + * key slots, the key slots currently accessed by the + * library cannot be reclaimed to free a key slot to load + * the persistent key. + * . In case of a multi-threaded application where one thread + * asks to close or purge or destroy a key while it is in use + * by the library through another thread. */ + size_t registered_readers; + } occupied; + } var; /* Dynamically allocated key data buffer. * Format as specified in psa_export_key(). */ @@ -169,7 +177,7 @@ typedef struct { */ static inline int psa_key_slot_has_readers(const psa_key_slot_t *slot) { - return slot->registered_readers > 0; + return slot->var.occupied.registered_readers > 0; } #if defined(MBEDTLS_PSA_CRYPTO_SE_C) diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.c b/tf-psa-crypto/core/psa_crypto_slot_management.c index 7e0a1c12c1..327be0a4f3 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.c +++ b/tf-psa-crypto/core/psa_crypto_slot_management.c @@ -268,7 +268,7 @@ void psa_wipe_all_key_slots(void) for (size_t slice_idx = 0; slice_idx < KEY_SLICE_COUNT; slice_idx++) { for (size_t slot_idx = 0; slot_idx < key_slice_length(slice_idx); slot_idx++) { psa_key_slot_t *slot = get_key_slot(slice_idx, slot_idx); - slot->registered_readers = 1; + slot->var.occupied.registered_readers = 1; slot->state = PSA_SLOT_PENDING_DELETION; (void) psa_wipe_key_slot(slot); } @@ -568,12 +568,12 @@ psa_status_t psa_unregister_read(psa_key_slot_t *slot) /* If we are the last reader and the slot is marked for deletion, * we must wipe the slot here. */ if ((slot->state == PSA_SLOT_PENDING_DELETION) && - (slot->registered_readers == 1)) { + (slot->var.occupied.registered_readers == 1)) { return psa_wipe_key_slot(slot); } if (psa_key_slot_has_readers(slot)) { - slot->registered_readers--; + slot->var.occupied.registered_readers--; return PSA_SUCCESS; } @@ -707,7 +707,7 @@ psa_status_t psa_close_key(psa_key_handle_t handle) return status; } - if (slot->registered_readers == 1) { + if (slot->var.occupied.registered_readers == 1) { status = psa_wipe_key_slot(slot); } else { status = psa_unregister_read(slot); @@ -742,7 +742,7 @@ psa_status_t psa_purge_key(mbedtls_svc_key_id_t key) } if ((!PSA_KEY_LIFETIME_IS_VOLATILE(slot->attr.lifetime)) && - (slot->registered_readers == 1)) { + (slot->var.occupied.registered_readers == 1)) { status = psa_wipe_key_slot(slot); } else { status = psa_unregister_read(slot); diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.h b/tf-psa-crypto/core/psa_crypto_slot_management.h index 88b7c837cc..cccf8f2f19 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.h +++ b/tf-psa-crypto/core/psa_crypto_slot_management.h @@ -174,10 +174,10 @@ static inline psa_status_t psa_key_slot_state_transition( static inline psa_status_t psa_register_read(psa_key_slot_t *slot) { if ((slot->state != PSA_SLOT_FULL) || - (slot->registered_readers >= SIZE_MAX)) { + (slot->var.occupied.registered_readers >= SIZE_MAX)) { return PSA_ERROR_CORRUPTION_DETECTED; } - slot->registered_readers++; + slot->var.occupied.registered_readers++; return PSA_SUCCESS; } From eab36d1d0627c2298ac8fd0779167ceed65d47f0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 10 Jun 2024 11:53:33 +0200 Subject: [PATCH 05/15] Dynamic key store: implementation When MBEDTLS_PSA_KEY_STORE_DYNAMIC is enabled, key slots are now organized in multiple slices. The slices are allocated on demand, which allows the key store to grow. The size of slices grows exponentially, which allows reaching a large number of slots with a small (static) number of slices without too much overhead. Maintain a linked list of free slots in each slice. This way, allocating a slot takes O(1) time unless a slice needs to be allocated. In this commit, slices are only ever freed when deinitializing the key store. This should be improved in the future to free empty slices. To avoid growing the persistent key cache without control, the persistent key cache has a fixed size (reusing MBEDTLS_PSA_KEY_SLOT_COUNT to avoid creating yet another option). When MBEDTLS_PSA_KEY_STORE_DYNAMIC is disabled. no semantic change and minimal changes to the code. Signed-off-by: Gilles Peskine --- tf-psa-crypto/core/psa_crypto.c | 29 +- tf-psa-crypto/core/psa_crypto_core.h | 30 +- .../core/psa_crypto_slot_management.c | 315 +++++++++++++++++- .../core/psa_crypto_slot_management.h | 44 ++- 4 files changed, 398 insertions(+), 20 deletions(-) diff --git a/tf-psa-crypto/core/psa_crypto.c b/tf-psa-crypto/core/psa_crypto.c index c6f805ba2a..2066d93b25 100644 --- a/tf-psa-crypto/core/psa_crypto.c +++ b/tf-psa-crypto/core/psa_crypto.c @@ -1232,6 +1232,11 @@ psa_status_t psa_wipe_key_slot(psa_key_slot_t *slot) status = PSA_ERROR_CORRUPTION_DETECTED; } +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + size_t slice_index = slot->slice_index; +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + + /* Multipart operations may still be using the key. This is safe * because all multipart operation objects are independent from * the key slot: if they need to access the key after the setup @@ -1242,6 +1247,17 @@ psa_status_t psa_wipe_key_slot(psa_key_slot_t *slot) * zeroize because the metadata is not particularly sensitive. * This memset also sets the slot's state to PSA_SLOT_EMPTY. */ memset(slot, 0, sizeof(*slot)); + +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + /* If the slot is already corrupted, something went deeply wrong, + * like a thread still using the slot or a stray pointer leading + * to the slot's memory being used for another object. Let the slot + * leak rather than make the corruption worse. */ + if (status == PSA_SUCCESS) { + status = psa_free_key_slot(slice_index, slot); + } +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + return status; } @@ -1753,8 +1769,6 @@ static psa_status_t psa_start_key_creation( psa_se_drv_table_entry_t **p_drv) { psa_status_t status; - psa_key_id_t volatile_key_id; - psa_key_slot_t *slot; (void) method; *p_drv = NULL; @@ -1764,11 +1778,16 @@ static psa_status_t psa_start_key_creation( return status; } + int key_is_volatile = PSA_KEY_LIFETIME_IS_VOLATILE(attributes->lifetime); + psa_key_id_t volatile_key_id; + #if defined(MBEDTLS_THREADING_C) PSA_THREADING_CHK_RET(mbedtls_mutex_lock( &mbedtls_threading_key_slot_mutex)); #endif - status = psa_reserve_free_key_slot(&volatile_key_id, p_slot); + status = psa_reserve_free_key_slot( + key_is_volatile ? &volatile_key_id : NULL, + p_slot); #if defined(MBEDTLS_THREADING_C) PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( &mbedtls_threading_key_slot_mutex)); @@ -1776,7 +1795,7 @@ static psa_status_t psa_start_key_creation( if (status != PSA_SUCCESS) { return status; } - slot = *p_slot; + psa_key_slot_t *slot = *p_slot; /* We're storing the declared bit-size of the key. It's up to each * creation mechanism to verify that this information is correct. @@ -1787,7 +1806,7 @@ static psa_status_t psa_start_key_creation( * definition. */ slot->attr = *attributes; - if (PSA_KEY_LIFETIME_IS_VOLATILE(slot->attr.lifetime)) { + if (key_is_volatile) { #if !defined(MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER) slot->attr.id = volatile_key_id; #else diff --git a/tf-psa-crypto/core/psa_crypto_core.h b/tf-psa-crypto/core/psa_crypto_core.h index a893c149ab..91ef0bfb78 100644 --- a/tf-psa-crypto/core/psa_crypto_core.h +++ b/tf-psa-crypto/core/psa_crypto_core.h @@ -80,10 +80,38 @@ typedef struct { * slots that are in a suitable state for the function. * For example, psa_get_and_lock_key_slot_in_memory, which finds a slot * containing a given key ID, will only check slots whose state variable is - * PSA_SLOT_FULL. */ + * PSA_SLOT_FULL. + */ psa_key_slot_state_t state; +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + /* The index of the slice containing this slot. + * This field must be filled if the slot contains a key + * (including keys being created or destroyed), and can be either + * filled or 0 when the slot is free. */ + uint8_t slice_index; +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + union { + struct { + /* The index of the next slot in the free list for this + * slice, relative * to the next array element. + * + * That is, 0 means the next slot, 1 means the next slot + * but one, etc. -1 would mean the slot itself. -2 means + * the previous slot, etc. + * + * If this is beyond the array length, the free list ends with the + * current element. + * + * The reason for this strange encoding is that 0 means the next + * element. This way, when we allocate a slice and initialize it + * to all-zero, the slice is ready for use, with a free list that + * consists of all the slots in order. + */ + int32_t next_free_relative_to_next; + } free; + struct { /* * Number of functions registered as reading the material in the key slot. diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.c b/tf-psa-crypto/core/psa_crypto_slot_management.c index 327be0a4f3..eef65dc142 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.c +++ b/tf-psa-crypto/core/psa_crypto_slot_management.c @@ -58,13 +58,110 @@ MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VOLATILE_MAX < MBEDTLS_PSA_KEY_ID_BUILTIN_MIN | +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + +/* Dynamic key store. + * + * The key store consists of multiple slices. + * + * The volatile keys are stored in variable-sized tables called slices. + * Slices are allocated on demand and deallocated when possible. + * The size of slices increases exponentially, so the average overhead + * (number of slots that are allocated but not used) is roughly + * proportional to the number of keys (with a factor that grows + * when the key store is fragmented). + * + * One slice is dedicated to the cache of persistent and built-in keys. + * For simplicity, they are separated from volatile keys. This cache + * slice has a fixed size and has the slice index KEY_SLOT_CACHE_SLICE_INDEX, + * located after the slices for volatile keys. + */ + +/* Size of slice 0 containing the cache of persistent and built-in keys. */ +#define PERSISTENT_KEY_CACHE_COUNT MBEDTLS_PSA_KEY_SLOT_COUNT + +/* Volatile keys are stored in slices 1 through KEY_SLICE_COUNT inclusive. + * Each slice is twice the size of the previous slice. + * Volatile key identifiers encode the slice number as follows: + * bits 30..31: 0b10 (mandated by the PSA Crypto specification). + * bits 25..29: slice index (0...KEY_SLOT_VOLATILE_SLICE_COUNT-1) + * bits 0..24: slot index in slice + */ +#define KEY_ID_SLOT_INDEX_WIDTH 25u +#define KEY_ID_SLICE_INDEX_WIDTH 5u + +#define KEY_SLOT_VOLATILE_SLICE_BASE_LENGTH 16u +#define KEY_SLOT_VOLATILE_SLICE_COUNT 22u +#define KEY_SLICE_COUNT (KEY_SLOT_VOLATILE_SLICE_COUNT + 1u) +#define KEY_SLOT_CACHE_SLICE_INDEX KEY_SLOT_VOLATILE_SLICE_COUNT + +#if KEY_ID_SLICE_INDEX_WIDTH + KEY_ID_SLOT_INDEX_WIDTH > 30 +#error "Not enough room in volatile key IDs for slice index and slot index" +#endif +#if KEY_SLICE_COUNT >= (1 << KEY_ID_SLICE_INDEX_WIDTH) - 1 +#error "Too many slices to fit the slice index in a volatile key ID" +#endif +#define KEY_SLICE_LENGTH_MAX \ + (KEY_SLOT_VOLATILE_SLICE_BASE_LENGTH << (KEY_SLOT_VOLATILE_SLICE_COUNT - 1)) +#if KEY_SLICE_LENGTH_MAX > 1 << KEY_ID_SLOT_INDEX_WIDTH +#error "Not enough room in volatile key IDs for a slot index in the largest slice" +#endif +#if KEY_ID_SLICE_INDEX_WIDTH > 8 +#error "Slice index does not fit in uint8_t for psa_key_slot_t::slice_index" +#endif + + +/* Calculate the volatile key id to use for a given slot. + * This function assumes valid parameter values. */ +static psa_key_id_t volatile_key_id_of_index(size_t slice_idx, + size_t slot_idx) +{ + return 0x40000000u | (slice_idx << KEY_ID_SLOT_INDEX_WIDTH) | slot_idx; +} + +/* Calculate the slice containing the given volatile key. + * This function assumes valid parameter values. */ +static size_t slice_index_of_volatile_key_id(psa_key_id_t key_id) +{ + size_t mask = (1LU << KEY_ID_SLICE_INDEX_WIDTH) - 1; + return (key_id >> KEY_ID_SLOT_INDEX_WIDTH) & mask; +} + +/* Calculate the index of the slot containing the given volatile key. + * This function assumes valid parameter values. */ +static size_t slot_index_of_volatile_key_id(psa_key_id_t key_id) +{ + return key_id & ((1LU << KEY_ID_SLOT_INDEX_WIDTH) - 1); +} + +/* In global_data.first_free_slot_index, use this special value to + * indicate that the slice is full. */ +#define FREE_SLOT_INDEX_NONE ((size_t) -1) + +#else /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + +/* Static key store. + * + * All the keys (volatile or persistent) are in a single slice. + * We only use slices as a concept to allow some differences between + * static and dynamic key store management to be buried in auxiliary + * functions. + */ + #define PERSISTENT_KEY_CACHE_COUNT MBEDTLS_PSA_KEY_SLOT_COUNT #define KEY_SLICE_COUNT 1u #define KEY_SLOT_CACHE_SLICE_INDEX 0 +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + typedef struct { +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + psa_key_slot_t *key_slices[KEY_SLICE_COUNT]; + size_t first_free_slot_index[KEY_SLOT_VOLATILE_SLICE_COUNT]; +#else /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ psa_key_slot_t key_slots[MBEDTLS_PSA_KEY_SLOT_COUNT]; +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ uint8_t key_slots_initialized; } psa_global_data_t; @@ -128,6 +225,46 @@ static inline psa_key_slot_t *get_persistent_key_slot(size_t slot_idx); */ static inline psa_key_slot_t *get_key_slot(size_t slice_idx, size_t slot_idx); +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + +static inline size_t key_slice_length(size_t slice_idx) +{ + if (slice_idx == KEY_SLOT_CACHE_SLICE_INDEX) { + return PERSISTENT_KEY_CACHE_COUNT; + } else { + return KEY_SLOT_VOLATILE_SLICE_BASE_LENGTH << slice_idx; + } +} + +static inline psa_key_slot_t *get_volatile_key_slot(psa_key_id_t key_id) +{ + size_t slice_idx = slice_index_of_volatile_key_id(key_id); + if (slice_idx >= KEY_SLOT_VOLATILE_SLICE_COUNT) { + return NULL; + } + size_t slot_idx = slot_index_of_volatile_key_id(key_id); + if (slot_idx >= key_slice_length(slice_idx)) { + return NULL; + } + psa_key_slot_t *slice = global_data.key_slices[slice_idx]; + if (slice == NULL) { + return NULL; + } + return &slice[slot_idx]; +} + +static inline psa_key_slot_t *get_persistent_key_slot(size_t slot_idx) +{ + return &global_data.key_slices[KEY_SLOT_CACHE_SLICE_INDEX][slot_idx]; +} + +static inline psa_key_slot_t *get_key_slot(size_t slice_idx, size_t slot_idx) +{ + return &global_data.key_slices[slice_idx][slot_idx]; +} + +#else /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + static inline size_t key_slice_length(size_t slice_idx) { (void) slice_idx; @@ -153,6 +290,9 @@ static inline psa_key_slot_t *get_key_slot(size_t slice_idx, size_t slot_idx) return &global_data.key_slots[slot_idx]; } +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + + int psa_is_valid_key_id(mbedtls_svc_key_id_t key, int vendor_ok) { @@ -219,8 +359,9 @@ static psa_status_t psa_get_and_lock_key_slot_in_memory( /* Check if both the PSA key identifier key_id and the owner * identifier of key match those of the key slot. */ - if ((slot->state == PSA_SLOT_FULL) && - (mbedtls_svc_key_id_equal(key, slot->attr.id))) { + if (slot != NULL && + slot->state == PSA_SLOT_FULL && + mbedtls_svc_key_id_equal(key, slot->attr.id)) { status = PSA_SUCCESS; } else { status = PSA_ERROR_DOES_NOT_EXIST; @@ -254,11 +395,21 @@ static psa_status_t psa_get_and_lock_key_slot_in_memory( psa_status_t psa_initialize_key_slots(void) { +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + global_data.key_slices[KEY_SLOT_CACHE_SLICE_INDEX] = + mbedtls_calloc(PERSISTENT_KEY_CACHE_COUNT, + sizeof(*global_data.key_slices[KEY_SLOT_CACHE_SLICE_INDEX])); + if (global_data.key_slices[KEY_SLOT_CACHE_SLICE_INDEX] == NULL) { + return PSA_ERROR_INSUFFICIENT_MEMORY; + } +#else /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ /* Nothing to do: program startup and psa_wipe_all_key_slots() both * guarantee that the key slots are initialized to all-zero, which * means that all the key slots are in a valid, empty state. The global * data mutex is already held when calling this function, so no need to * lock it here, to set the flag. */ +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + global_data.key_slots_initialized = 1; return PSA_SUCCESS; } @@ -266,17 +417,137 @@ psa_status_t psa_initialize_key_slots(void) void psa_wipe_all_key_slots(void) { for (size_t slice_idx = 0; slice_idx < KEY_SLICE_COUNT; slice_idx++) { +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + if (global_data.key_slices[slice_idx] == NULL) { + continue; + } +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ for (size_t slot_idx = 0; slot_idx < key_slice_length(slice_idx); slot_idx++) { psa_key_slot_t *slot = get_key_slot(slice_idx, slot_idx); + if (slot->state == PSA_SLOT_EMPTY) { + /* Don't call psa_wipe_key_slot() on an already-empty slot. + * It rejects that case anyway, though we bypass it by setting + * the slot state to PENDING_DELETION. + * Also, when MBEDTLS_PSA_KEY_STORE_DYNAMIC is enabled, + * psa_wipe_key_slot() needs to have a valid slice_index + * field, but that value might not be correct in a + * free slot. */ + continue; + } slot->var.occupied.registered_readers = 1; slot->state = PSA_SLOT_PENDING_DELETION; (void) psa_wipe_key_slot(slot); } +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + mbedtls_free(global_data.key_slices[slice_idx]); + global_data.key_slices[slice_idx] = NULL; +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ } + +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + for (size_t slice_idx = 0; slice_idx < KEY_SLOT_VOLATILE_SLICE_COUNT; slice_idx++) { + global_data.first_free_slot_index[slice_idx] = 0; + } +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + /* The global data mutex is already held when calling this function. */ global_data.key_slots_initialized = 0; } +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + +static psa_status_t psa_allocate_volatile_key_slot(psa_key_id_t *key_id, + psa_key_slot_t **p_slot) +{ + size_t slice_idx; + for (slice_idx = 0; slice_idx < KEY_SLOT_VOLATILE_SLICE_COUNT; slice_idx++) { + if (global_data.first_free_slot_index[slice_idx] != FREE_SLOT_INDEX_NONE) { + break; + } + } + if (slice_idx == KEY_SLOT_VOLATILE_SLICE_COUNT) { + return PSA_ERROR_INSUFFICIENT_MEMORY; + } + + if (global_data.key_slices[slice_idx] == NULL) { + global_data.key_slices[slice_idx] = + mbedtls_calloc(key_slice_length(slice_idx), + sizeof(psa_key_slot_t)); + if (global_data.key_slices[slice_idx] == NULL) { + return PSA_ERROR_INSUFFICIENT_MEMORY; + } + } + psa_key_slot_t *slice = global_data.key_slices[slice_idx]; + + size_t slot_idx = global_data.first_free_slot_index[slice_idx]; + *key_id = volatile_key_id_of_index(slice_idx, slot_idx); + + psa_key_slot_t *slot = &slice[slot_idx]; + size_t next_free = slot_idx + 1 + slot->var.free.next_free_relative_to_next; + if (next_free >= key_slice_length(slice_idx)) { + next_free = FREE_SLOT_INDEX_NONE; + } + global_data.first_free_slot_index[slice_idx] = next_free; + /* The .next_free field is not meaningful when the slot is not free, + * so give it the same content as freshly initialized memory. */ + slot->var.free.next_free_relative_to_next = 0; + + psa_status_t status = psa_key_slot_state_transition(slot, + PSA_SLOT_EMPTY, + PSA_SLOT_FILLING); + if (status != PSA_SUCCESS) { + /* The only reason for failure is if the slot state was not empty. + * This indicates that something has gone horribly wrong. + * In this case, we leave the slot out of the free list, and stop + * modifying it. This minimizes any further corruption. The slot + * is a memory leak, but that's a lesser evil. */ + return status; + } + + *p_slot = slot; + slot->slice_index = slice_idx; + return PSA_SUCCESS; +} + +psa_status_t psa_free_key_slot(size_t slice_idx, + psa_key_slot_t *slot) +{ + + if (slice_idx == KEY_SLOT_CACHE_SLICE_INDEX) { + /* This is a cache entry. We don't maintain a free list, so + * there's nothing to do. */ + return PSA_SUCCESS; + } + if (slice_idx >= KEY_SLOT_VOLATILE_SLICE_COUNT) { + return PSA_ERROR_CORRUPTION_DETECTED; + } + + psa_key_slot_t *slice = global_data.key_slices[slice_idx]; + psa_key_slot_t *slice_end = slice + key_slice_length(slice_idx); + if (slot < slice || slot >= slice_end) { + /* The slot isn't actually in the slice! We can't detect that + * condition for sure, because the pointer comparison itself is + * undefined behavior in that case. That same condition makes the + * subtraction to calculate the slot index also UB. + * Give up now to avoid causing further corruption. + */ + return PSA_ERROR_CORRUPTION_DETECTED; + } + size_t slot_idx = slot - slice; + + size_t next_free = global_data.first_free_slot_index[slice_idx]; + if (next_free >= key_slice_length(slice_idx)) { + /* The slot was full. The newly freed slot thus becomes the + * end of the free list. */ + next_free = key_slice_length(slice_idx); + } + global_data.first_free_slot_index[slice_idx] = slot_idx; + slot->var.free.next_free_relative_to_next = next_free - slot_idx - 1; + + return PSA_SUCCESS; +} +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, psa_key_slot_t **p_slot) { @@ -289,6 +560,17 @@ psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, goto error; } + if (volatile_key_id != NULL) { + *volatile_key_id = 0; +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + return psa_allocate_volatile_key_slot(volatile_key_id, p_slot); +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + } + + /* With a dynamic key store, allocate an entry in the cache slice, + * applicable only to non-volatile keys that get cached in RAM. + * With a static key store, allocate an entry in the sole slice, + * applicable to all keys. */ selected_slot = unused_persistent_key_slot = NULL; for (slot_idx = 0; slot_idx < PERSISTENT_KEY_CACHE_COUNT; slot_idx++) { psa_key_slot_t *slot = get_key_slot(KEY_SLOT_CACHE_SLICE_INDEX, slot_idx); @@ -329,8 +611,18 @@ psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, goto error; } - *volatile_key_id = PSA_KEY_ID_VOLATILE_MIN + - ((psa_key_id_t) (selected_slot - global_data.key_slots)); +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + selected_slot->slice_index = KEY_SLOT_CACHE_SLICE_INDEX; +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + +#if !defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + if (volatile_key_id != NULL) { + /* Refresh slot_idx, for when the slot is not the original + * selected_slot but rather unused_persistent_key_slot. */ + slot_idx = selected_slot - global_data.key_slots; + *volatile_key_id = PSA_KEY_ID_VOLATILE_MIN + slot_idx; + } +#endif *p_slot = selected_slot; return PSA_SUCCESS; @@ -339,7 +631,6 @@ psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, error: *p_slot = NULL; - *volatile_key_id = 0; return status; } @@ -498,9 +789,8 @@ psa_status_t psa_get_and_lock_key_slot(mbedtls_svc_key_id_t key, /* Loading keys from storage requires support for such a mechanism */ #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) || \ defined(MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS) - psa_key_id_t volatile_key_id; - status = psa_reserve_free_key_slot(&volatile_key_id, p_slot); + status = psa_reserve_free_key_slot(NULL, p_slot); if (status != PSA_SUCCESS) { #if defined(MBEDTLS_THREADING_C) PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( @@ -760,15 +1050,20 @@ void mbedtls_psa_get_stats(mbedtls_psa_stats_t *stats) memset(stats, 0, sizeof(*stats)); for (size_t slice_idx = 0; slice_idx < KEY_SLICE_COUNT; slice_idx++) { +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + if (global_data.key_slices[slice_idx] == NULL) { + continue; + } +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ for (size_t slot_idx = 0; slot_idx < key_slice_length(slice_idx); slot_idx++) { const psa_key_slot_t *slot = get_key_slot(slice_idx, slot_idx); - if (psa_key_slot_has_readers(slot)) { - ++stats->locked_slots; - } if (slot->state == PSA_SLOT_EMPTY) { ++stats->empty_slots; continue; } + if (psa_key_slot_has_readers(slot)) { + ++stats->locked_slots; + } if (PSA_KEY_LIFETIME_IS_VOLATILE(slot->attr.lifetime)) { ++stats->volatile_slots; } else { diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.h b/tf-psa-crypto/core/psa_crypto_slot_management.h index cccf8f2f19..1e6e935b59 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.h +++ b/tf-psa-crypto/core/psa_crypto_slot_management.h @@ -17,8 +17,10 @@ * * The first #MBEDTLS_PSA_KEY_SLOT_COUNT identifiers of the implementation * range of key identifiers are reserved for volatile key identifiers. - * A volatile key identifier is equal to #PSA_KEY_ID_VOLATILE_MIN plus the - * index of the key slot containing the volatile key definition. + * + * If \c id is a a volatile key identifier, #PSA_KEY_ID_VOLATILE_MIN - \c id + * indicates the key slot containing the volatile key definition. See + * psa_crypto_slot_management.c for details. */ /** The minimum value for a volatile key identifier. @@ -27,8 +29,12 @@ /** The maximum value for a volatile key identifier. */ +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) +#define PSA_KEY_ID_VOLATILE_MAX (MBEDTLS_PSA_KEY_ID_BUILTIN_MIN - 1) +#else /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ #define PSA_KEY_ID_VOLATILE_MAX \ (PSA_KEY_ID_VOLATILE_MIN + MBEDTLS_PSA_KEY_SLOT_COUNT - 1) +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ /** Test whether a key identifier is a volatile key identifier. * @@ -113,13 +119,20 @@ void psa_wipe_all_key_slots(void); * If multi-threading is enabled, the caller must hold the * global key slot mutex. * - * \param[out] volatile_key_id On success, volatile key identifier - * associated to the returned slot. + * \param[out] volatile_key_id If null, reserve a cache slot for + * a persistent or built-in key. + * If non-null, allocate a slot for + * a volatile key. + * If non-null, on success, the volatile key + * identifier corresponding with the + * returned slot. * \param[out] p_slot On success, a pointer to the slot. * * \retval #PSA_SUCCESS \emptydescription * \retval #PSA_ERROR_INSUFFICIENT_MEMORY * There were no free key slots. + * When #MBEDTLS_PSA_KEY_STORE_DYNAMIC is enabled, there was not + * enough memory to allocate more slots. * \retval #PSA_ERROR_BAD_STATE \emptydescription * \retval #PSA_ERROR_CORRUPTION_DETECTED * This function attempted to operate on a key slot which was in an @@ -128,6 +141,29 @@ void psa_wipe_all_key_slots(void); psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, psa_key_slot_t **p_slot); +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) +/** Return a key slot to the free list. + * + * Call this function when a slot obtained from psa_reserve_free_key_slot() + * is no longer in use. + * + * If multi-threading is enabled, the caller must hold the + * global key slot mutex. + * + * \param slice_idx The slice containing the slot. + * This is `slot->slice_index` when the slot + * is obtained from psa_reserve_free_key_slot(). + * \param slot The key slot. + * + * \retval #PSA_SUCCESS \emptydescription + * \retval #PSA_ERROR_CORRUPTION_DETECTED + * This function attempted to operate on a key slot which was in an + * unexpected state. + */ +psa_status_t psa_free_key_slot(size_t slice_idx, + psa_key_slot_t *slot); +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ + /** Change the state of a key slot. * * This function changes the state of the key slot from expected_state to From aadeeb3e76edf31848731962330c537bc2da0e55 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 10 Jun 2024 15:41:38 +0200 Subject: [PATCH 06/15] Microoptimizations when MBEDTLS_PSA_KEY_STORE_DYNAMIC is disabled Compensate some of the code size increase from implementing dynamic key slots. Signed-off-by: Gilles Peskine --- .../core/psa_crypto_slot_management.c | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.c b/tf-psa-crypto/core/psa_crypto_slot_management.c index eef65dc142..5b4539ca7b 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.c +++ b/tf-psa-crypto/core/psa_crypto_slot_management.c @@ -424,16 +424,24 @@ void psa_wipe_all_key_slots(void) #endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ for (size_t slot_idx = 0; slot_idx < key_slice_length(slice_idx); slot_idx++) { psa_key_slot_t *slot = get_key_slot(slice_idx, slot_idx); +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + /* When MBEDTLS_PSA_KEY_STORE_DYNAMIC is disabled, calling + * psa_wipe_key_slot() on an unused slot is useless, but it + * happens to work (because we flip the state to PENDING_DELETION). + * + * When MBEDTLS_PSA_KEY_STORE_DYNAMIC is enabled, + * psa_wipe_key_slot() needs to have a valid slice_index + * field, but that value might not be correct in a + * free slot, so we must not call it. + * + * Bypass the call to psa_wipe_key_slot() if the slot is empty, + * but only if MBEDTLS_PSA_KEY_STORE_DYNAMIC is enabled, to save + * a few bytes of code size otherwise. + */ if (slot->state == PSA_SLOT_EMPTY) { - /* Don't call psa_wipe_key_slot() on an already-empty slot. - * It rejects that case anyway, though we bypass it by setting - * the slot state to PENDING_DELETION. - * Also, when MBEDTLS_PSA_KEY_STORE_DYNAMIC is enabled, - * psa_wipe_key_slot() needs to have a valid slice_index - * field, but that value might not be correct in a - * free slot. */ continue; } +#endif slot->var.occupied.registered_readers = 1; slot->state = PSA_SLOT_PENDING_DELETION; (void) psa_wipe_key_slot(slot); @@ -560,12 +568,11 @@ psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, goto error; } - if (volatile_key_id != NULL) { - *volatile_key_id = 0; #if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) + if (volatile_key_id != NULL) { return psa_allocate_volatile_key_slot(volatile_key_id, p_slot); -#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ } +#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ /* With a dynamic key store, allocate an entry in the cache slice, * applicable only to non-volatile keys that get cached in RAM. From 75071066f2c25eed94903ffe093d7b6df8cd9042 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 21 Jun 2024 00:09:07 +0200 Subject: [PATCH 07/15] Dynamic key store: make full-key-store tests work effectively Add a practical way to fill the dynamic key store by artificially limiting the slice length through a test hook. Signed-off-by: Gilles Peskine --- .../core/psa_crypto_slot_management.c | 16 ++++++++++ .../core/psa_crypto_slot_management.h | 18 ++++++++++++ ...test_suite_psa_crypto_slot_management.data | 5 ++++ ..._suite_psa_crypto_slot_management.function | 29 +++++++++++++++++-- 4 files changed, 65 insertions(+), 3 deletions(-) diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.c b/tf-psa-crypto/core/psa_crypto_slot_management.c index 5b4539ca7b..d740960dd5 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.c +++ b/tf-psa-crypto/core/psa_crypto_slot_management.c @@ -138,6 +138,13 @@ static size_t slot_index_of_volatile_key_id(psa_key_id_t key_id) * indicate that the slice is full. */ #define FREE_SLOT_INDEX_NONE ((size_t) -1) +#if defined(MBEDTLS_TEST_HOOKS) +size_t psa_key_slot_volatile_slice_count(void) +{ + return KEY_SLOT_VOLATILE_SLICE_COUNT; +} +#endif + #else /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ /* Static key store. @@ -227,11 +234,20 @@ static inline psa_key_slot_t *get_key_slot(size_t slice_idx, size_t slot_idx); #if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) +#if defined(MBEDTLS_TEST_HOOKS) +size_t (*mbedtls_test_hook_psa_volatile_key_slice_length)(size_t slice_idx) = NULL; +#endif + static inline size_t key_slice_length(size_t slice_idx) { if (slice_idx == KEY_SLOT_CACHE_SLICE_INDEX) { return PERSISTENT_KEY_CACHE_COUNT; } else { +#if defined(MBEDTLS_TEST_HOOKS) + if (mbedtls_test_hook_psa_volatile_key_slice_length != NULL) { + return mbedtls_test_hook_psa_volatile_key_slice_length(slice_idx); + } +#endif return KEY_SLOT_VOLATILE_SLICE_BASE_LENGTH << slice_idx; } } diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.h b/tf-psa-crypto/core/psa_crypto_slot_management.h index 1e6e935b59..26b73969b4 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.h +++ b/tf-psa-crypto/core/psa_crypto_slot_management.h @@ -100,6 +100,24 @@ psa_status_t psa_get_and_lock_key_slot(mbedtls_svc_key_id_t key, */ psa_status_t psa_initialize_key_slots(void); +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) +/* Allow test code to customize the key slice length. We use this in tests + * that exhaust the key store to reach a full key store in reasonable time + * and memory. + * + * The length of each slice must be between 1 and + * (1 << KEY_ID_SLOT_INDEX_WIDTH) inclusive. + * + * The length for a given slice index must not change while + * the key store is initialized. + */ +extern size_t (*mbedtls_test_hook_psa_volatile_key_slice_length)( + size_t slice_idx); + +/* The number of volatile key slices. */ +size_t psa_key_slot_volatile_slice_count(void); +#endif + /** Delete all data from key slots in memory. * This function is not thread safe, it wipes every key slot regardless of * state and reader count. It should only be called when no slot is in use. diff --git a/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.data b/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.data index 1bf300ade6..f379dba020 100644 --- a/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.data +++ b/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.data @@ -228,6 +228,11 @@ invalid_handle:INVALID_HANDLE_HUGE:PSA_ERROR_INVALID_HANDLE Key slot count: maximum many_transient_keys:MBEDTLS_PSA_KEY_SLOT_COUNT - MBEDTLS_TEST_PSA_INTERNAL_KEYS +Key slot count: dynamic: more than MBEDTLS_PSA_KEY_SLOT_COUNT +depends_on:MBEDTLS_PSA_KEY_STORE_DYNAMIC +# Check that MBEDTLS_PSA_KEY_SLOT_COUNT doesn't apply to volatile keys. +many_transient_keys:MBEDTLS_PSA_KEY_SLOT_COUNT + 1 + Key slot count: try to overfill, destroy first fill_key_store:0 diff --git a/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.function b/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.function index b2d3f29735..604c4bd5de 100644 --- a/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.function +++ b/tf-psa-crypto/tests/suites/test_suite_psa_crypto_slot_management.function @@ -98,11 +98,27 @@ exit: return 0; } -/* Currently, there is always a maximum number of volatile keys that can - * realistically be reached in tests. When we add configurations where this - * is not true, undefine the macro in such configurations. */ #if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) +#if defined(MBEDTLS_TEST_HOOKS) +/* Artificially restrictable dynamic key store */ +#define KEY_SLICE_1_LENGTH 4 +#define KEY_SLICE_2_LENGTH 10 +static size_t tiny_key_slice_length(size_t slice_idx) +{ + switch (slice_idx) { + case 1: return KEY_SLICE_1_LENGTH; + case 2: return KEY_SLICE_2_LENGTH; + default: return 1; + } +} +#define MAX_VOLATILE_KEYS \ + (KEY_SLICE_1_LENGTH + KEY_SLICE_2_LENGTH + \ + psa_key_slot_volatile_slice_count() - 2) + +#else /* Effectively unbounded dynamic key store */ #undef MAX_VOLATILE_KEYS +#endif + #else /* Static key store */ #define MAX_VOLATILE_KEYS MBEDTLS_PSA_KEY_SLOT_COUNT #endif @@ -871,6 +887,10 @@ void fill_key_store(int key_to_destroy_arg) uint8_t exported[sizeof(size_t)]; size_t exported_length; +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) && defined(MBEDTLS_TEST_HOOKS) + mbedtls_test_hook_psa_volatile_key_slice_length = &tiny_key_slice_length; +#endif + PSA_ASSERT(psa_crypto_init()); mbedtls_psa_stats_t stats; @@ -953,6 +973,9 @@ void fill_key_store(int key_to_destroy_arg) exit: PSA_DONE(); mbedtls_free(keys); +#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) && defined(MBEDTLS_TEST_HOOKS) + mbedtls_test_hook_psa_volatile_key_slice_length = NULL; +#endif } /* END_CASE */ From a9dda7e3d05ebb906c7c18228293c9f6c9d4fe2a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 21 Jun 2024 11:25:01 +0200 Subject: [PATCH 08/15] Add test components with the PSA static key store We were only testing the static key store (MBEDTLS_PSA_KEY_STORE_DYNAMIC disabled) with configs/*.h. Add a component with the static key store and everything else (including built-in keys), and a component with the static key store and CTR_DBRG using PSA for AES (which means PSA uses a volatile key internally). Signed-off-by: Gilles Peskine --- .../components-configuration-crypto.sh | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/scripts/components-configuration-crypto.sh b/tests/scripts/components-configuration-crypto.sh index 049e817869..c6b2d544df 100644 --- a/tests/scripts/components-configuration-crypto.sh +++ b/tests/scripts/components-configuration-crypto.sh @@ -2069,6 +2069,40 @@ common_block_cipher_dispatch () { scripts/config.py set MBEDTLS_DEPRECATED_REMOVED } +component_test_full_block_cipher_psa_dispatch_static_keystore () { + msg "build: full + PSA dispatch in block_cipher with static keystore" + # Check that the static key store works well when CTR_DRBG uses a + # PSA key for AES. + scripts/config.py unset MBEDTLS_PSA_KEY_STORE_DYNAMIC + + loc_accel_list="ALG_ECB_NO_PADDING \ + KEY_TYPE_AES KEY_TYPE_ARIA KEY_TYPE_CAMELLIA" + + # Configure + # --------- + + common_block_cipher_dispatch 1 + + # Build + # ----- + + helper_libtestdriver1_make_drivers "$loc_accel_list" + + helper_libtestdriver1_make_main "$loc_accel_list" + + # Make sure disabled components were not re-enabled by accident (additive + # config) + not grep mbedtls_aes_ library/aes.o + not grep mbedtls_aria_ library/aria.o + not grep mbedtls_camellia_ library/camellia.o + + # Run the tests + # ------------- + + msg "test: full + PSA dispatch in block_cipher with static keystore" + make test +} + component_test_full_block_cipher_psa_dispatch () { msg "build: full + PSA dispatch in block_cipher" @@ -2595,6 +2629,16 @@ component_test_se_default () { make test } +component_test_full_static_keystore () { + msg "build: full config - MBEDTLS_PSA_KEY_STORE_DYNAMIC" + scripts/config.py full + scripts/config.py unset MBEDTLS_PSA_KEY_STORE_DYNAMIC + make CC=clang CFLAGS="$ASAN_CFLAGS -Os" LDFLAGS="$ASAN_CFLAGS" + + msg "test: full config - MBEDTLS_PSA_KEY_STORE_DYNAMIC" + make test +} + component_test_psa_crypto_drivers () { msg "build: full + test drivers dispatching to builtins" scripts/config.py full From 75fd2401e595d97a0dd2cbb0d17f255bed35c917 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 10 Jun 2024 18:50:00 +0200 Subject: [PATCH 09/15] Changelog entry for MBEDTLS_PSA_KEY_STORE_DYNAMIC Signed-off-by: Gilles Peskine --- ChangeLog.d/dynamic-keystore.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog.d/dynamic-keystore.txt b/ChangeLog.d/dynamic-keystore.txt index d576dcd86f..c6aac3c991 100644 --- a/ChangeLog.d/dynamic-keystore.txt +++ b/ChangeLog.d/dynamic-keystore.txt @@ -1,3 +1,9 @@ +Features + * When the new compilation option MBEDTLS_PSA_KEY_STORE_DYNAMIC is enabled, + the number of volatile PSA keys is virtually unlimited, at the expense + of increased code size. This option is off by default, but enabled in + the default mbedtls_config.h. Fixes #9216. + Bugfix * Fix interference between PSA volatile keys and built-in keys when MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS is enabled and From 5abeb8c77bb4a1c90fc0ede372d6ef8230dfc5ea Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 13 Jun 2024 20:36:50 +0200 Subject: [PATCH 10/15] Make integer downsizing explicit Reassure both humans and compilers that the places where we assign an integer to a smaller type are safe. Signed-off-by: Gilles Peskine --- tf-psa-crypto/core/psa_crypto_slot_management.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.c b/tf-psa-crypto/core/psa_crypto_slot_management.c index d740960dd5..feedbb5ea4 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.c +++ b/tf-psa-crypto/core/psa_crypto_slot_management.c @@ -116,7 +116,12 @@ MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VOLATILE_MAX < MBEDTLS_PSA_KEY_ID_BUILTIN_MIN | static psa_key_id_t volatile_key_id_of_index(size_t slice_idx, size_t slot_idx) { - return 0x40000000u | (slice_idx << KEY_ID_SLOT_INDEX_WIDTH) | slot_idx; + /* We assert above that the slice and slot indexes fit in separate + * bit-fields inside psa_key_id_t, which is a 32-bit type per the + * PSA Cryptography specification. */ + return (psa_key_id_t) (0x40000000u | + (slice_idx << KEY_ID_SLOT_INDEX_WIDTH) | + slot_idx); } /* Calculate the slice containing the given volatile key. @@ -529,7 +534,8 @@ static psa_status_t psa_allocate_volatile_key_slot(psa_key_id_t *key_id, } *p_slot = slot; - slot->slice_index = slice_idx; + /* We assert at compile time that the slice index fits in uint8_t. */ + slot->slice_index = (uint8_t) slice_idx; return PSA_SUCCESS; } @@ -566,7 +572,8 @@ psa_status_t psa_free_key_slot(size_t slice_idx, next_free = key_slice_length(slice_idx); } global_data.first_free_slot_index[slice_idx] = slot_idx; - slot->var.free.next_free_relative_to_next = next_free - slot_idx - 1; + slot->var.free.next_free_relative_to_next = + (int32_t) next_free - (int32_t) slot_idx - 1; return PSA_SUCCESS; } From d339aefd917d4503ff068e88de853cb21c2a43e3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 9 Aug 2024 14:04:46 +0200 Subject: [PATCH 11/15] Clarify some internal documentation Signed-off-by: Gilles Peskine --- tf-psa-crypto/core/psa_crypto_core.h | 10 +++++++++- tf-psa-crypto/core/psa_crypto_slot_management.h | 16 +++++++++------- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/tf-psa-crypto/core/psa_crypto_core.h b/tf-psa-crypto/core/psa_crypto_core.h index 91ef0bfb78..21e7559f01 100644 --- a/tf-psa-crypto/core/psa_crypto_core.h +++ b/tf-psa-crypto/core/psa_crypto_core.h @@ -88,7 +88,15 @@ typedef struct { /* The index of the slice containing this slot. * This field must be filled if the slot contains a key * (including keys being created or destroyed), and can be either - * filled or 0 when the slot is free. */ + * filled or 0 when the slot is free. + * + * In most cases, the slice index can be deduced from the key identifer. + * We keep it in a separate field for robustness (it reduces the chance + * that a coding mistake in the key store will result in accessing the + * wrong slice), and also so that it's available even on code paths + * during creation or destruction where the key identifier might not be + * filled in. + * */ uint8_t slice_index; #endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */ diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.h b/tf-psa-crypto/core/psa_crypto_slot_management.h index 26b73969b4..af1208e3ae 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.h +++ b/tf-psa-crypto/core/psa_crypto_slot_management.h @@ -137,13 +137,15 @@ void psa_wipe_all_key_slots(void); * If multi-threading is enabled, the caller must hold the * global key slot mutex. * - * \param[out] volatile_key_id If null, reserve a cache slot for - * a persistent or built-in key. - * If non-null, allocate a slot for - * a volatile key. - * If non-null, on success, the volatile key - * identifier corresponding with the - * returned slot. + * \param[out] volatile_key_id - If null, reserve a cache slot for + * a persistent or built-in key. + * - If non-null, allocate a slot for + * a volatile key. On success, + * \p *volatile_key_id is the + * identifier corresponding to the + * returned slot. It is the caller's + * responsibility to set this key identifier + * in the attributes. * \param[out] p_slot On success, a pointer to the slot. * * \retval #PSA_SUCCESS \emptydescription From fdcc47c426e47da74702479d89dfdc82034e581a Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 21 Aug 2024 14:41:06 +0100 Subject: [PATCH 12/15] Fix incorrect comments on slice numbering The persistent key cache slice is the last slice (not the first as previously stated). Update the numbering-related comments accordingly. Signed-off-by: David Horstmann --- tf-psa-crypto/core/psa_crypto_slot_management.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.c b/tf-psa-crypto/core/psa_crypto_slot_management.c index feedbb5ea4..101a976847 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.c +++ b/tf-psa-crypto/core/psa_crypto_slot_management.c @@ -77,10 +77,11 @@ MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VOLATILE_MAX < MBEDTLS_PSA_KEY_ID_BUILTIN_MIN | * located after the slices for volatile keys. */ -/* Size of slice 0 containing the cache of persistent and built-in keys. */ +/* Size of the last slice containing the cache of persistent and built-in keys. */ #define PERSISTENT_KEY_CACHE_COUNT MBEDTLS_PSA_KEY_SLOT_COUNT -/* Volatile keys are stored in slices 1 through KEY_SLICE_COUNT inclusive. +/* Volatile keys are stored in slices 0 through + * (KEY_SLOT_VOLATILE_SLICE_COUNT - 1) inclusive. * Each slice is twice the size of the previous slice. * Volatile key identifiers encode the slice number as follows: * bits 30..31: 0b10 (mandated by the PSA Crypto specification). From 68a4b7453f0c57e7b5753b709a2814e219caa806 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 21 Aug 2024 14:48:32 +0100 Subject: [PATCH 13/15] Tweak macro check to allow 3 extra key slices We are technically allowed to use all possible values of key slice index that will fit into the bit width we have allocated, so allow all values. Signed-off-by: David Horstmann --- tf-psa-crypto/core/psa_crypto_slot_management.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.c b/tf-psa-crypto/core/psa_crypto_slot_management.c index 101a976847..7857aad77a 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.c +++ b/tf-psa-crypto/core/psa_crypto_slot_management.c @@ -99,7 +99,7 @@ MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VOLATILE_MAX < MBEDTLS_PSA_KEY_ID_BUILTIN_MIN | #if KEY_ID_SLICE_INDEX_WIDTH + KEY_ID_SLOT_INDEX_WIDTH > 30 #error "Not enough room in volatile key IDs for slice index and slot index" #endif -#if KEY_SLICE_COUNT >= (1 << KEY_ID_SLICE_INDEX_WIDTH) - 1 +#if KEY_SLOT_VOLATILE_SLICE_COUNT > (1 << KEY_ID_SLICE_INDEX_WIDTH) #error "Too many slices to fit the slice index in a volatile key ID" #endif #define KEY_SLICE_LENGTH_MAX \ From 0b2bd071f8de72e9ce144d47dab32e8e942adda0 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 21 Aug 2024 15:38:44 +0100 Subject: [PATCH 14/15] Add overflow check for maximum key slot length Signed-off-by: David Horstmann --- tf-psa-crypto/core/psa_crypto_slot_management.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.c b/tf-psa-crypto/core/psa_crypto_slot_management.c index 7857aad77a..216e0c27cf 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.c +++ b/tf-psa-crypto/core/psa_crypto_slot_management.c @@ -111,6 +111,11 @@ MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VOLATILE_MAX < MBEDTLS_PSA_KEY_ID_BUILTIN_MIN | #error "Slice index does not fit in uint8_t for psa_key_slot_t::slice_index" #endif +MBEDTLS_STATIC_ASSERT((KEY_SLOT_VOLATILE_SLICE_BASE_LENGTH + & (SIZE_MAX >> (KEY_SLOT_VOLATILE_SLICE_COUNT - 1))) + == KEY_SLOT_VOLATILE_SLICE_BASE_LENGTH, + "Maximum slice length overflows size_t"); + /* Calculate the volatile key id to use for a given slot. * This function assumes valid parameter values. */ From 4c9fccff5a41b2f35a584eb5fa6c9df6edb03cbc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 21 Aug 2024 21:46:26 +0200 Subject: [PATCH 15/15] Simplify and explain the overflow check for maximum slice length Signed-off-by: Gilles Peskine --- tf-psa-crypto/core/psa_crypto_slot_management.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/tf-psa-crypto/core/psa_crypto_slot_management.c b/tf-psa-crypto/core/psa_crypto_slot_management.c index 216e0c27cf..9850d8c750 100644 --- a/tf-psa-crypto/core/psa_crypto_slot_management.c +++ b/tf-psa-crypto/core/psa_crypto_slot_management.c @@ -96,6 +96,18 @@ MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VOLATILE_MAX < MBEDTLS_PSA_KEY_ID_BUILTIN_MIN | #define KEY_SLICE_COUNT (KEY_SLOT_VOLATILE_SLICE_COUNT + 1u) #define KEY_SLOT_CACHE_SLICE_INDEX KEY_SLOT_VOLATILE_SLICE_COUNT + +/* Check that the length of the largest slice (calculated as + * KEY_SLICE_LENGTH_MAX below) does not overflow size_t. We use + * an indirect method in case the calculation of KEY_SLICE_LENGTH_MAX + * itself overflows uintmax_t: if (BASE_LENGTH << c) + * overflows size_t then BASE_LENGTH > SIZE_MAX >> c. + */ +#if (KEY_SLOT_VOLATILE_SLICE_BASE_LENGTH > \ + SIZE_MAX >> (KEY_SLOT_VOLATILE_SLICE_COUNT - 1)) +#error "Maximum slice length overflows size_t" +#endif + #if KEY_ID_SLICE_INDEX_WIDTH + KEY_ID_SLOT_INDEX_WIDTH > 30 #error "Not enough room in volatile key IDs for slice index and slot index" #endif @@ -111,11 +123,6 @@ MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VOLATILE_MAX < MBEDTLS_PSA_KEY_ID_BUILTIN_MIN | #error "Slice index does not fit in uint8_t for psa_key_slot_t::slice_index" #endif -MBEDTLS_STATIC_ASSERT((KEY_SLOT_VOLATILE_SLICE_BASE_LENGTH - & (SIZE_MAX >> (KEY_SLOT_VOLATILE_SLICE_COUNT - 1))) - == KEY_SLOT_VOLATILE_SLICE_BASE_LENGTH, - "Maximum slice length overflows size_t"); - /* Calculate the volatile key id to use for a given slot. * This function assumes valid parameter values. */