From 077fd87748fdf19df979d6501cf42fa8eaac9222 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Thu, 22 Feb 2024 16:55:03 +0000 Subject: [PATCH 01/12] Add new global mutex for PSA global_data Signed-off-by: Paul Elliott --- include/mbedtls/threading.h | 7 +++++++ library/threading.c | 3 +++ 2 files changed, 10 insertions(+) diff --git a/include/mbedtls/threading.h b/include/mbedtls/threading.h index b4e050241b..fbd7ad2e47 100644 --- a/include/mbedtls/threading.h +++ b/include/mbedtls/threading.h @@ -112,6 +112,13 @@ extern mbedtls_threading_mutex_t mbedtls_threading_gmtime_mutex; * psa_key_slot_state_transition(), psa_register_read(), psa_unregister_read(), * psa_key_slot_has_readers() and psa_wipe_key_slot(). */ extern mbedtls_threading_mutex_t mbedtls_threading_key_slot_mutex; + +/* + * A mutex used to make the PSA global_data struct members thread safe. + * + * This mutex must be held when any read or write to a any of the PSA + * global_data structure members. */ +extern mbedtls_threading_mutex_t mbedtls_threading_psa_globaldata_mutex; #endif #endif /* MBEDTLS_THREADING_C */ diff --git a/library/threading.c b/library/threading.c index c28290fb76..06b474726c 100644 --- a/library/threading.c +++ b/library/threading.c @@ -150,6 +150,7 @@ void mbedtls_threading_set_alt(void (*mutex_init)(mbedtls_threading_mutex_t *), #endif #if defined(MBEDTLS_PSA_CRYPTO_C) mbedtls_mutex_init(&mbedtls_threading_key_slot_mutex); + mbedtls_mutex_init(&mbedtls_threading_psa_globaldata_mutex); #endif } @@ -166,6 +167,7 @@ void mbedtls_threading_free_alt(void) #endif #if defined(MBEDTLS_PSA_CRYPTO_C) mbedtls_mutex_free(&mbedtls_threading_key_slot_mutex); + mbedtls_mutex_free(&mbedtls_threading_psa_globaldata_mutex); #endif } #endif /* MBEDTLS_THREADING_ALT */ @@ -184,6 +186,7 @@ mbedtls_threading_mutex_t mbedtls_threading_gmtime_mutex MUTEX_INIT; #endif #if defined(MBEDTLS_PSA_CRYPTO_C) mbedtls_threading_mutex_t mbedtls_threading_key_slot_mutex MUTEX_INIT; +mbedtls_threading_mutex_t mbedtls_threading_psa_globaldata_mutex MUTEX_INIT; #endif #endif /* MBEDTLS_THREADING_C */ From b8e38e0e27b0dc2e5932fbef3b6c352500a44fb7 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Mon, 11 Mar 2024 12:09:49 +0000 Subject: [PATCH 02/12] Add new mutex for PSA global rng data Signed-off-by: Paul Elliott --- include/mbedtls/threading.h | 13 ++++++++++--- library/threading.c | 3 +++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/threading.h b/include/mbedtls/threading.h index fbd7ad2e47..d50d04ead1 100644 --- a/include/mbedtls/threading.h +++ b/include/mbedtls/threading.h @@ -114,11 +114,18 @@ extern mbedtls_threading_mutex_t mbedtls_threading_gmtime_mutex; extern mbedtls_threading_mutex_t mbedtls_threading_key_slot_mutex; /* - * A mutex used to make the PSA global_data struct members thread safe. + * A mutex used to make the non-rng PSA global_data struct members thread safe. * - * This mutex must be held when any read or write to a any of the PSA - * global_data structure members. */ + * This mutex must be held when reading or writing to any of the PSA global_data + * structure members, other than the rng_state or rng struct. */ extern mbedtls_threading_mutex_t mbedtls_threading_psa_globaldata_mutex; + +/* + * A mutex used to make the PSA global_data rng data thread safe. + * + * This mutex must be held when reading or writing to the PSA + * global_data rng_state or rng struct members. */ +extern mbedtls_threading_mutex_t mbedtls_threading_psa_rngdata_mutex; #endif #endif /* MBEDTLS_THREADING_C */ diff --git a/library/threading.c b/library/threading.c index 06b474726c..85db243f21 100644 --- a/library/threading.c +++ b/library/threading.c @@ -151,6 +151,7 @@ void mbedtls_threading_set_alt(void (*mutex_init)(mbedtls_threading_mutex_t *), #if defined(MBEDTLS_PSA_CRYPTO_C) mbedtls_mutex_init(&mbedtls_threading_key_slot_mutex); mbedtls_mutex_init(&mbedtls_threading_psa_globaldata_mutex); + mbedtls_mutex_init(&mbedtls_threading_psa_rngdata_mutex); #endif } @@ -168,6 +169,7 @@ void mbedtls_threading_free_alt(void) #if defined(MBEDTLS_PSA_CRYPTO_C) mbedtls_mutex_free(&mbedtls_threading_key_slot_mutex); mbedtls_mutex_free(&mbedtls_threading_psa_globaldata_mutex); + mbedtls_mutex_free(&mbedtls_threading_psa_rngdata_mutex); #endif } #endif /* MBEDTLS_THREADING_ALT */ @@ -187,6 +189,7 @@ mbedtls_threading_mutex_t mbedtls_threading_gmtime_mutex MUTEX_INIT; #if defined(MBEDTLS_PSA_CRYPTO_C) mbedtls_threading_mutex_t mbedtls_threading_key_slot_mutex MUTEX_INIT; mbedtls_threading_mutex_t mbedtls_threading_psa_globaldata_mutex MUTEX_INIT; +mbedtls_threading_mutex_t mbedtls_threading_psa_rngdata_mutex MUTEX_INIT; #endif #endif /* MBEDTLS_THREADING_C */ From 600472b4438601029e2f77c8336840306b2119db Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 23 Feb 2024 17:26:58 +0000 Subject: [PATCH 03/12] Protect PSA global initialized flag with mutex. Unfortunately this requires holding the mutex for the entire psa_crypto_init() function, as calling psa_crypto_free() from another thread should block until init has ended, then run. Signed-off-by: Paul Elliott --- library/psa_crypto.c | 55 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 4 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index ec9d115b3b..3019522f29 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -71,6 +71,7 @@ #include "mbedtls/sha256.h" #include "mbedtls/sha512.h" #include "mbedtls/psa_util.h" +#include "mbedtls/threading.h" #if defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF) || \ defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF_EXTRACT) || \ @@ -101,8 +102,25 @@ typedef struct { static psa_global_data_t global_data; +static uint8_t psa_get_initialized(void) +{ + uint8_t initialized; + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + initialized = global_data.initialized; + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + return initialized; +} + #define GUARD_MODULE_INITIALIZED \ - if (global_data.initialized == 0) \ + if (psa_get_initialized() == 0) \ return PSA_ERROR_BAD_STATE; int psa_can_do_hash(psa_algorithm_t hash_alg) @@ -7189,7 +7207,7 @@ psa_status_t psa_generate_random(uint8_t *output, psa_status_t mbedtls_psa_inject_entropy(const uint8_t *seed, size_t seed_size) { - if (global_data.initialized) { + if (psa_get_initialized()) { return PSA_ERROR_NOT_PERMITTED; } @@ -7442,15 +7460,27 @@ psa_status_t mbedtls_psa_crypto_configure_entropy_sources( void mbedtls_psa_crypto_free(void) { + /* Need to hold the mutex here to prevent this going ahead before + * psa_crypto_init() has completed, and to ensure integrity of + * global_data. */ +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + psa_wipe_all_key_slots(); if (global_data.rng_state != RNG_NOT_INITIALIZED) { mbedtls_psa_random_free(&global_data.rng); } + /* Wipe all remaining data, including configuration. * In particular, this sets all state indicator to the value * indicating "uninitialized". */ mbedtls_platform_zeroize(&global_data, sizeof(global_data)); +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + /* Terminate drivers */ psa_driver_wrapper_free(); } @@ -7484,8 +7514,20 @@ psa_status_t psa_crypto_init(void) { psa_status_t status; - /* Double initialization is explicitly allowed. */ - if (global_data.initialized != 0) { + /* Need to hold the mutex for the entire function to prevent incomplete + * initialisation before someone calls psa_crypto_free() or calls this + * function again before we set global_data.initialised to 1. */ + #if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + /* We cannot use psa_get_initialized() here as we already hold the mutex. */ + if (global_data.initialized == 1) { +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + /* Double initialization is explicitly allowed. */ return PSA_SUCCESS; } @@ -7528,6 +7570,11 @@ psa_status_t psa_crypto_init(void) global_data.initialized = 1; exit: + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + if (status != PSA_SUCCESS) { mbedtls_psa_crypto_free(); } From 8e15153637a226f46b4301a706f6abdce1d7bb34 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 23 Feb 2024 17:35:29 +0000 Subject: [PATCH 04/12] Protect PSA global rng data with mutex. Reads and writes of rng_state in psa_crypto_init() and psa_crypto_free() were already covered by mutex. Signed-off-by: Paul Elliott --- library/psa_crypto.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 3019522f29..8f41641325 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7449,12 +7449,25 @@ psa_status_t mbedtls_psa_crypto_configure_entropy_sources( void (* entropy_init)(mbedtls_entropy_context *ctx), void (* entropy_free)(mbedtls_entropy_context *ctx)) { + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_psa_rngdata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + if (global_data.rng_state != RNG_NOT_INITIALIZED) { - return PSA_ERROR_BAD_STATE; + status = PSA_ERROR_BAD_STATE; + } else { + global_data.rng.entropy_init = entropy_init; + global_data.rng.entropy_free = entropy_free; + status = PSA_SUCCESS; } - global_data.rng.entropy_init = entropy_init; - global_data.rng.entropy_free = entropy_free; - return PSA_SUCCESS; + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_psa_rngdata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + return status; } #endif /* !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG) */ From 358165246b39222edbb3e2ef51282a71e99a6897 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 23 Feb 2024 17:47:42 +0000 Subject: [PATCH 05/12] Protect PSA drivers_initialized with mutex Writes to this in psa_crypto_init() were again already covered. Signed-off-by: Paul Elliott --- library/psa_crypto.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 8f41641325..63fd05c59b 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -119,6 +119,23 @@ static uint8_t psa_get_initialized(void) return initialized; } +static uint8_t psa_get_drivers_initialized(void) +{ + uint8_t initialized; + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + initialized = global_data.drivers_initialized; + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + return initialized; +} + #define GUARD_MODULE_INITIALIZED \ if (psa_get_initialized() == 0) \ return PSA_ERROR_BAD_STATE; @@ -126,14 +143,14 @@ static uint8_t psa_get_initialized(void) int psa_can_do_hash(psa_algorithm_t hash_alg) { (void) hash_alg; - return global_data.drivers_initialized; + return psa_get_drivers_initialized(); } int psa_can_do_cipher(psa_key_type_t key_type, psa_algorithm_t cipher_alg) { (void) key_type; (void) cipher_alg; - return global_data.drivers_initialized; + return psa_get_drivers_initialized(); } From 47cee8e2ee084645ab00d56db5ffea5d428af289 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 8 Mar 2024 21:38:02 +0000 Subject: [PATCH 06/12] Add mbedtls_psa_crypto_init_subsystem() Internal only for now, but can be made external with some more work. Break up psa_crypto_init into chunks to prevent deadlocks when initialising RNG, likewise break up mbedtls_crypto_free() to stop having to hold more than one mutex at a time. Signed-off-by: Paul Elliott --- library/psa_crypto.c | 266 +++++++++++++++++++++++++++++++++---------- 1 file changed, 207 insertions(+), 59 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 63fd05c59b..88842678f5 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -93,10 +93,25 @@ static int key_type_is_raw_bytes(psa_key_type_t type) #define RNG_INITIALIZED 1 #define RNG_SEEDED 2 +typedef enum { + PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS = 0, + PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS, + PSA_CRYPTO_SUBSYSTEM_RNG, + PSA_CRYPTO_SUBSYSTEM_TRANSACTION, +} mbedtls_psa_crypto_subsystem; + +#define PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED 0x01 +#define PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS_INITIALIZED 0x02 +#define PSA_CRYPTO_SUBSYSTEM_TRANSACTION_INITIALIZED 0x04 + +#define PSA_CRYPTO_SUBSYSTEM_ALL_INITIALISED ( \ + PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED | \ + PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS_INITIALIZED | \ + PSA_CRYPTO_SUBSYSTEM_TRANSACTION_INITIALIZED) + typedef struct { uint8_t initialized; uint8_t rng_state; - uint8_t drivers_initialized; mbedtls_psa_random_context_t rng; } psa_global_data_t; @@ -106,11 +121,22 @@ static uint8_t psa_get_initialized(void) { uint8_t initialized; +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_psa_rngdata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + initialized = global_data.rng_state == RNG_SEEDED; + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_psa_rngdata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex); #endif /* defined(MBEDTLS_THREADING_C) */ - initialized = global_data.initialized; + initialized = + (initialized && (global_data.initialized == PSA_CRYPTO_SUBSYSTEM_ALL_INITIALISED)); #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); @@ -127,7 +153,7 @@ static uint8_t psa_get_drivers_initialized(void) mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex); #endif /* defined(MBEDTLS_THREADING_C) */ - initialized = global_data.drivers_initialized; + initialized = (global_data.initialized & PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED) != 0; #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); @@ -7490,29 +7516,53 @@ psa_status_t mbedtls_psa_crypto_configure_entropy_sources( void mbedtls_psa_crypto_free(void) { - /* Need to hold the mutex here to prevent this going ahead before - * psa_crypto_init() has completed, and to ensure integrity of - * global_data. */ + #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex); #endif /* defined(MBEDTLS_THREADING_C) */ - psa_wipe_all_key_slots(); - if (global_data.rng_state != RNG_NOT_INITIALIZED) { - mbedtls_psa_random_free(&global_data.rng); + /* Nothing to do to free transaction. */ + if (global_data.initialized & PSA_CRYPTO_SUBSYSTEM_TRANSACTION_INITIALIZED) { + global_data.initialized &= ~PSA_CRYPTO_SUBSYSTEM_TRANSACTION_INITIALIZED; } - /* Wipe all remaining data, including configuration. - * In particular, this sets all state indicator to the value - * indicating "uninitialized". */ - mbedtls_platform_zeroize(&global_data, sizeof(global_data)); + if (global_data.initialized & PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS_INITIALIZED) { + psa_wipe_all_key_slots(); + global_data.initialized &= ~PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS_INITIALIZED; + } #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); #endif /* defined(MBEDTLS_THREADING_C) */ +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_psa_rngdata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + if (global_data.rng_state != RNG_NOT_INITIALIZED) { + mbedtls_psa_random_free(&global_data.rng); + } + global_data.rng_state = RNG_NOT_INITIALIZED; + mbedtls_platform_zeroize(&global_data.rng, sizeof(global_data.rng)); + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_psa_rngdata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + /* Terminate drivers */ - psa_driver_wrapper_free(); + if (global_data.initialized & PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED) { + psa_driver_wrapper_free(); + global_data.initialized &= ~PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED; + } + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + } #if defined(PSA_CRYPTO_STORAGE_HAS_TRANSACTIONS) @@ -7540,74 +7590,172 @@ static psa_status_t psa_crypto_recover_transaction( } #endif /* PSA_CRYPTO_STORAGE_HAS_TRANSACTIONS */ +static psa_status_t mbedtls_psa_crypto_init_subsystem(mbedtls_psa_crypto_subsystem subsystem) +{ + psa_status_t status = PSA_SUCCESS; + uint8_t driver_wrappers_initialized = 0; + + switch (subsystem) { + case PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS: + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex)); +#endif /* defined(MBEDTLS_THREADING_C) */ + + if (!(global_data.initialized & PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED)) { + /* Init drivers */ + status = psa_driver_wrapper_init(); + + /* Drivers need shutdown regardless of startup errors. */ + global_data.initialized |= PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED; + + + } +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_unlock( + &mbedtls_threading_psa_globaldata_mutex)); +#endif /* defined(MBEDTLS_THREADING_C) */ + + break; + + case PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS: + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex)); +#endif /* defined(MBEDTLS_THREADING_C) */ + + if (!(global_data.initialized & PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS_INITIALIZED)) { + status = psa_initialize_key_slots(); + + /* Need to wipe keys even if initialization fails. */ + global_data.initialized |= PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS_INITIALIZED; + + } +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_unlock( + &mbedtls_threading_psa_globaldata_mutex)); +#endif /* defined(MBEDTLS_THREADING_C) */ + + break; + + case PSA_CRYPTO_SUBSYSTEM_RNG: + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex)); +#endif /* defined(MBEDTLS_THREADING_C) */ + + driver_wrappers_initialized = + (global_data.initialized & PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED); + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_unlock( + &mbedtls_threading_psa_globaldata_mutex)); +#endif /* defined(MBEDTLS_THREADING_C) */ + + /* Need to use separate mutex here, as initialisation can require + * testing of init flags, which requires locking the global data + * mutex. */ +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_lock(&mbedtls_threading_psa_rngdata_mutex)); +#endif /* defined(MBEDTLS_THREADING_C) */ + + /* Initialize and seed the random generator. */ + if (global_data.rng_state == RNG_NOT_INITIALIZED && driver_wrappers_initialized) { + mbedtls_psa_random_init(&global_data.rng); + global_data.rng_state = RNG_INITIALIZED; + + status = mbedtls_psa_random_seed(&global_data.rng); + if (status == PSA_SUCCESS) { + global_data.rng_state = RNG_SEEDED; + } + } + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_unlock( + &mbedtls_threading_psa_rngdata_mutex)); +#endif /* defined(MBEDTLS_THREADING_C) */ + + + break; + + case PSA_CRYPTO_SUBSYSTEM_TRANSACTION: + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex)); +#endif /* defined(MBEDTLS_THREADING_C) */ + + if (!(global_data.initialized & PSA_CRYPTO_SUBSYSTEM_TRANSACTION_INITIALIZED)) { +#if defined(PSA_CRYPTO_STORAGE_HAS_TRANSACTIONS) + status = psa_crypto_load_transaction(); + if (status == PSA_SUCCESS) { + status = psa_crypto_recover_transaction(&psa_crypto_transaction); + if (status == PSA_SUCCESS) { + global_data.initialized |= PSA_CRYPTO_SUBSYSTEM_TRANSACTION_INITIALIZED; + } + status = psa_crypto_stop_transaction(); + } else if (status == PSA_ERROR_DOES_NOT_EXIST) { + /* There's no transaction to complete. It's all good. */ + global_data.initialized |= PSA_CRYPTO_SUBSYSTEM_TRANSACTION_INITIALIZED; + status = PSA_SUCCESS; + } +#else /* defined(PSA_CRYPTO_STORAGE_HAS_TRANSACTIONS) */ + global_data.initialized |= PSA_CRYPTO_SUBSYSTEM_TRANSACTION_INITIALIZED; + status = PSA_SUCCESS; +#endif /* defined(PSA_CRYPTO_STORAGE_HAS_TRANSACTIONS) */ + } + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_unlock( + &mbedtls_threading_psa_globaldata_mutex)); +#endif /* defined(MBEDTLS_THREADING_C) */ + + break; + + default: + status = PSA_ERROR_CORRUPTION_DETECTED; + } + + /* Exit label only required when using threading macros. */ +#if defined(MBEDTLS_THREADING_C) +exit: +#endif /* defined(MBEDTLS_THREADING_C) */ + + return status; +} + psa_status_t psa_crypto_init(void) { psa_status_t status; - /* Need to hold the mutex for the entire function to prevent incomplete - * initialisation before someone calls psa_crypto_free() or calls this - * function again before we set global_data.initialised to 1. */ - #if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex); -#endif /* defined(MBEDTLS_THREADING_C) */ - - /* We cannot use psa_get_initialized() here as we already hold the mutex. */ - if (global_data.initialized == 1) { -#if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); -#endif /* defined(MBEDTLS_THREADING_C) */ - - /* Double initialization is explicitly allowed. */ + /* Double initialization is explicitly allowed. Early out if everything is + * done. */ + if (psa_get_initialized()) { return PSA_SUCCESS; } - /* Init drivers */ - status = psa_driver_wrapper_init(); - if (status != PSA_SUCCESS) { - goto exit; - } - global_data.drivers_initialized = 1; - - status = psa_initialize_key_slots(); + status = mbedtls_psa_crypto_init_subsystem(PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS); if (status != PSA_SUCCESS) { goto exit; } - /* Initialize and seed the random generator. */ - mbedtls_psa_random_init(&global_data.rng); - global_data.rng_state = RNG_INITIALIZED; - status = mbedtls_psa_random_seed(&global_data.rng); + status = mbedtls_psa_crypto_init_subsystem(PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS); if (status != PSA_SUCCESS) { goto exit; } - global_data.rng_state = RNG_SEEDED; -#if defined(PSA_CRYPTO_STORAGE_HAS_TRANSACTIONS) - status = psa_crypto_load_transaction(); - if (status == PSA_SUCCESS) { - status = psa_crypto_recover_transaction(&psa_crypto_transaction); - if (status != PSA_SUCCESS) { - goto exit; - } - status = psa_crypto_stop_transaction(); - } else if (status == PSA_ERROR_DOES_NOT_EXIST) { - /* There's no transaction to complete. It's all good. */ - status = PSA_SUCCESS; + status = mbedtls_psa_crypto_init_subsystem(PSA_CRYPTO_SUBSYSTEM_RNG); + if (status != PSA_SUCCESS) { + goto exit; } -#endif /* PSA_CRYPTO_STORAGE_HAS_TRANSACTIONS */ - /* All done. */ - global_data.initialized = 1; + status = mbedtls_psa_crypto_init_subsystem(PSA_CRYPTO_SUBSYSTEM_TRANSACTION); exit: -#if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); -#endif /* defined(MBEDTLS_THREADING_C) */ - if (status != PSA_SUCCESS) { mbedtls_psa_crypto_free(); } + return status; } From 838886da64e23f81ffbe82b987a347f467ecc9ad Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Tue, 12 Mar 2024 15:26:04 +0000 Subject: [PATCH 07/12] Protect the key slot management initialised flag Use the global data mutex, as the key slot mutex has to be held in some of the functions where we are testing the flag, and we already hold the global data mutex when calling the functions where the flag is set. Signed-off-by: Paul Elliott --- library/psa_crypto_slot_management.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 5dee32ffe3..6a51644027 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -34,6 +34,24 @@ typedef struct { static psa_global_data_t global_data; +static uint8_t psa_get_key_slots_initialized(void) +{ + + uint8_t initialized; + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + initialized = global_data.key_slots_initialized; + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_psa_globaldata_mutex); +#endif /* defined(MBEDTLS_THREADING_C) */ + + return initialized; +} + 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); @@ -136,7 +154,9 @@ psa_status_t psa_initialize_key_slots(void) { /* 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. */ + * 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. */ global_data.key_slots_initialized = 1; return PSA_SUCCESS; } @@ -151,6 +171,7 @@ void psa_wipe_all_key_slots(void) 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; } @@ -161,7 +182,7 @@ psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, size_t slot_idx; psa_key_slot_t *selected_slot, *unused_persistent_key_slot; - if (!global_data.key_slots_initialized) { + if (!psa_get_key_slots_initialized()) { status = PSA_ERROR_BAD_STATE; goto error; } @@ -344,7 +365,7 @@ psa_status_t psa_get_and_lock_key_slot(mbedtls_svc_key_id_t key, psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; *p_slot = NULL; - if (!global_data.key_slots_initialized) { + if (!psa_get_key_slots_initialized()) { return PSA_ERROR_BAD_STATE; } From 0493ab56a424d886c2b63552dda5da21736ec939 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Wed, 6 Mar 2024 12:26:58 +0000 Subject: [PATCH 08/12] Add PSA threaded init tests Signed-off-by: Paul Elliott --- tests/suites/test_suite_psa_crypto_init.data | 3 + .../test_suite_psa_crypto_init.function | 115 ++++++++++++++++++ 2 files changed, 118 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto_init.data b/tests/suites/test_suite_psa_crypto_init.data index 8c5b41d6cb..147d03fbed 100644 --- a/tests/suites/test_suite_psa_crypto_init.data +++ b/tests/suites/test_suite_psa_crypto_init.data @@ -10,6 +10,9 @@ deinit_without_init:0 PSA deinit twice deinit_without_init:1 +PSA threaded init checks +psa_threaded_init:100 + No random without init validate_module_init_generate_random:0 diff --git a/tests/suites/test_suite_psa_crypto_init.function b/tests/suites/test_suite_psa_crypto_init.function index 7a434322ae..9ff33a6d84 100644 --- a/tests/suites/test_suite_psa_crypto_init.function +++ b/tests/suites/test_suite_psa_crypto_init.function @@ -1,6 +1,7 @@ /* BEGIN_HEADER */ #include +#include "psa_crypto_core.h" /* Some tests in this module configure entropy sources. */ #include "psa_crypto_invasive.h" @@ -112,6 +113,59 @@ static void custom_entropy_init(mbedtls_entropy_context *ctx) #endif /* !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG) */ +#if defined MBEDTLS_THREADING_PTHREAD + +typedef struct { + int do_init; +} thread_psa_init_ctx_t; + +static void *thread_psa_init_function(void *ctx) +{ + thread_psa_init_ctx_t *init_context = (thread_psa_init_ctx_t *) ctx; + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; + uint8_t random[10] = { 0 }; + + if (init_context->do_init) { + PSA_ASSERT(psa_crypto_init()); + } + + /* If this is a test only thread, then we can assume PSA is being started + * up on another thread and thus we cannot know whether the following tests + * will be successful or not. These checks are still useful, however even + * without checking the return codes as they may show up race conditions on + * the flags they check under TSAN.*/ + + /* Test getting if drivers are initialised. */ + int can_do = psa_can_do_hash(PSA_ALG_NONE); + + if (init_context->do_init) { + TEST_ASSERT(can_do == 1); + } + +#if !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG) + + /* Test getting global_data.rng_state. */ + status = mbedtls_psa_crypto_configure_entropy_sources(NULL, NULL); + + if (init_context->do_init) { + /* Bad state due to entropy sources already being setup in + * psa_crypto_init() */ + TEST_EQUAL(status, PSA_ERROR_BAD_STATE); + } +#endif /* !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG) */ + + /* Test using the PSA RNG ony if we know PSA is up and running. */ + if (init_context->do_init) { + status = psa_generate_random(random, sizeof(random)); + + TEST_EQUAL(status, PSA_SUCCESS); + } + +exit: + return NULL; +} +#endif /* defined MBEDTLS_THREADING_PTHREAD */ + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -154,6 +208,67 @@ void deinit_without_init(int count) } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_THREADING_PTHREAD */ +void psa_threaded_init(int arg_thread_count) +{ + thread_psa_init_ctx_t init_context; + thread_psa_init_ctx_t init_context_2; + + size_t thread_count = (size_t) arg_thread_count; + mbedtls_test_thread_t *threads = NULL; + + TEST_CALLOC(threads, sizeof(mbedtls_test_thread_t) * thread_count); + + init_context.do_init = 1; + + /* Test initialising PSA and testing certain protected globals on multiple + * threads. */ + for (size_t i = 0; i < thread_count; i++) { + TEST_EQUAL( + mbedtls_test_thread_create(&threads[i], + thread_psa_init_function, + (void *) &init_context), + 0); + } + + for (size_t i = 0; i < thread_count; i++) { + TEST_EQUAL(mbedtls_test_thread_join(&threads[i]), 0); + } + + PSA_DONE(); + + init_context_2.do_init = 0; + + /* Test initialising PSA whilst also testing flags on other threads. */ + for (size_t i = 0; i < thread_count; i++) { + + if (i & 1) { + + TEST_EQUAL( + mbedtls_test_thread_create(&threads[i], + thread_psa_init_function, + (void *) &init_context), + 0); + } else { + TEST_EQUAL( + mbedtls_test_thread_create(&threads[i], + thread_psa_init_function, + (void *) &init_context_2), + 0); + } + } + + for (size_t i = 0; i < thread_count; i++) { + TEST_EQUAL(mbedtls_test_thread_join(&threads[i]), 0); + } +exit: + + PSA_DONE(); + + mbedtls_free(threads); +} +/* END_CASE */ + /* BEGIN_CASE */ void validate_module_init_generate_random(int count) { From 78279962d6e1025d0e1883269b10e271a8de73c9 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 15 Mar 2024 13:34:01 +0000 Subject: [PATCH 09/12] Fix minor style issues Signed-off-by: Paul Elliott --- library/psa_crypto.c | 1 - library/psa_crypto_slot_management.c | 1 - 2 files changed, 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 88842678f5..dd638de92c 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7675,7 +7675,6 @@ static psa_status_t mbedtls_psa_crypto_init_subsystem(mbedtls_psa_crypto_subsyst &mbedtls_threading_psa_rngdata_mutex)); #endif /* defined(MBEDTLS_THREADING_C) */ - break; case PSA_CRYPTO_SUBSYSTEM_TRANSACTION: diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 6a51644027..b184ed08c9 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -36,7 +36,6 @@ static psa_global_data_t global_data; static uint8_t psa_get_key_slots_initialized(void) { - uint8_t initialized; #if defined(MBEDTLS_THREADING_C) From 0db6a9033aed3bd10975c898b850ebf8343b1690 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 15 Mar 2024 13:48:20 +0000 Subject: [PATCH 10/12] Start subsystem IDs at 1 instead of 0 Catch potential invalid calls to init. Signed-off-by: Paul Elliott --- library/psa_crypto.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index dd638de92c..c28937df14 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -93,8 +93,10 @@ static int key_type_is_raw_bytes(psa_key_type_t type) #define RNG_INITIALIZED 1 #define RNG_SEEDED 2 +/* IDs for PSA crypto subsystems. Starts at 1 to catch potential uninitialized + * variables as arguments. */ typedef enum { - PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS = 0, + PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS = 1, PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS, PSA_CRYPTO_SUBSYSTEM_RNG, PSA_CRYPTO_SUBSYSTEM_TRANSACTION, From d35dce6e235dd0c7818949d0954b1bdcaadfea52 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 15 Mar 2024 13:57:16 +0000 Subject: [PATCH 11/12] Add comments about RNG mutex requirements Signed-off-by: Paul Elliott --- library/psa_crypto.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index c28937df14..373918a885 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -7145,6 +7145,9 @@ exit: #endif /** Initialize the PSA random generator. + * + * Note: the mbedtls_threading_psa_rngdata_mutex should be held when calling + * this function if mutexes are enabled. */ static void mbedtls_psa_random_init(mbedtls_psa_random_context_t *rng) { @@ -7177,6 +7180,9 @@ static void mbedtls_psa_random_init(mbedtls_psa_random_context_t *rng) } /** Deinitialize the PSA random generator. + * + * Note: the mbedtls_threading_psa_rngdata_mutex should be held when calling + * this function if mutexes are enabled. */ static void mbedtls_psa_random_free(mbedtls_psa_random_context_t *rng) { From b24e36d07b16e2e4c7f6341f5e033ed7f4ac773f Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 15 Mar 2024 16:25:48 +0000 Subject: [PATCH 12/12] Add explanatory comment for init flags Signed-off-by: Paul Elliott --- library/psa_crypto.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 373918a885..a0a002a088 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -102,6 +102,7 @@ typedef enum { PSA_CRYPTO_SUBSYSTEM_TRANSACTION, } mbedtls_psa_crypto_subsystem; +/* Initialization flags for global_data::initialized */ #define PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED 0x01 #define PSA_CRYPTO_SUBSYSTEM_KEY_SLOTS_INITIALIZED 0x02 #define PSA_CRYPTO_SUBSYSTEM_TRANSACTION_INITIALIZED 0x04