From 024b395f85162594cc683b1e0d65ae77dfaaafac Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Tue, 23 Jan 2024 19:56:32 +0000 Subject: [PATCH 1/5] Make psa_reserve_free_key_slot thread safe Everything needs to be done under the mutex here, we operate directly on FULL/EMPTY slots, and we can't let key_slots_initialized change before we operate on slots. Refactor to use an exit label. Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index dc38662e1..07d7f35fc 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -160,9 +160,13 @@ 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 defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_lock( + &mbedtls_threading_key_slot_mutex)); +#endif if (!global_data.key_slots_initialized) { status = PSA_ERROR_BAD_STATE; - goto error; + goto exit; } selected_slot = unused_persistent_key_slot = NULL; @@ -194,7 +198,7 @@ psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, psa_register_read(selected_slot); status = psa_wipe_key_slot(selected_slot); if (status != PSA_SUCCESS) { - goto error; + goto exit; } } @@ -202,21 +206,27 @@ psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, status = psa_key_slot_state_transition(selected_slot, PSA_SLOT_EMPTY, PSA_SLOT_FILLING); if (status != PSA_SUCCESS) { - goto error; + goto exit; } *volatile_key_id = PSA_KEY_ID_VOLATILE_MIN + ((psa_key_id_t) (selected_slot - global_data.key_slots)); *p_slot = selected_slot; - return PSA_SUCCESS; + goto exit; } status = PSA_ERROR_INSUFFICIENT_MEMORY; -error: - *p_slot = NULL; - *volatile_key_id = 0; +exit: + if (status != PSA_SUCCESS) { + *p_slot = NULL; + *volatile_key_id = 0; + } +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif return status; } From 91ffe5b87187e45a0d58cf0510d773ebe804508d Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Tue, 23 Jan 2024 20:05:42 +0000 Subject: [PATCH 2/5] Make psa_finish_key_creation thread safe Hold mutex for the entirety of the call. We are writing to storage and writing to the slot state here. If we didn't keep the mutex for the whole duration then we may end up with another thread seeing that a persistent key is in storage before our slot is set to FULL; this would be unlinearizable behaviour. Signed-off-by: Ryan Everett --- library/psa_crypto.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 4a0666bb8..d53a09da3 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1799,6 +1799,11 @@ static psa_status_t psa_finish_key_creation( (void) slot; (void) driver; +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_lock( + &mbedtls_threading_key_slot_mutex)); +#endif + #if defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) if (!PSA_KEY_LIFETIME_IS_VOLATILE(slot->attr.lifetime)) { #if defined(MBEDTLS_PSA_CRYPTO_SE_C) @@ -1838,6 +1843,11 @@ static psa_status_t psa_finish_key_creation( status = psa_save_se_persistent_data(driver); if (status != PSA_SUCCESS) { psa_destroy_persistent_key(slot->attr.id); + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif return status; } status = psa_crypto_stop_transaction(); @@ -1853,6 +1863,10 @@ static psa_status_t psa_finish_key_creation( } } +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif return status; } From b71014406c090349e414dec586845c415ed71dd9 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Tue, 23 Jan 2024 20:09:49 +0000 Subject: [PATCH 3/5] Make psa_fail_key_creation thread safe Hold the mutex for the entirety of the call. We need the mutex for the wipe, also hold it for aborting driver transactions as this may have side effects. We can't use the macros here as this function returns void. Signed-off-by: Ryan Everett --- library/psa_crypto.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index d53a09da3..a0e58a271 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1891,6 +1891,10 @@ static void psa_fail_key_creation(psa_key_slot_t *slot, return; } +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_key_slot_mutex); +#endif + #if defined(MBEDTLS_PSA_CRYPTO_SE_C) /* TODO: If the key has already been created in the secure * element, and the failure happened later (when saving metadata @@ -1909,6 +1913,10 @@ static void psa_fail_key_creation(psa_key_slot_t *slot, #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ psa_wipe_key_slot(slot); + +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_key_slot_mutex); +#endif } /** Validate optional attributes during key creation. From 3d8118d9dcae661fe2cc7d958d1a6ec8ee444c5c Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Tue, 30 Jan 2024 16:58:47 +0000 Subject: [PATCH 4/5] Revert psa_reserve_free_key_slot changes, lock in start_key_creation instead This means we can hold the mutex around the call to reserve_free_key_slot in get_and_lock_key_slot, avoiding inefficient rework. (Changes to get_and_lock_key_slot are not in scope in this PR) Signed-off-by: Ryan Everett --- library/psa_crypto.c | 8 ++++++++ library/psa_crypto_slot_management.c | 24 +++++++----------------- library/psa_crypto_slot_management.h | 3 +++ 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index a0e58a271..5300126c3 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1679,7 +1679,15 @@ static psa_status_t psa_start_key_creation( return status; } +#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); +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif if (status != PSA_SUCCESS) { return status; } diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 07d7f35fc..dc38662e1 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -160,13 +160,9 @@ 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 defined(MBEDTLS_THREADING_C) - PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_lock( - &mbedtls_threading_key_slot_mutex)); -#endif if (!global_data.key_slots_initialized) { status = PSA_ERROR_BAD_STATE; - goto exit; + goto error; } selected_slot = unused_persistent_key_slot = NULL; @@ -198,7 +194,7 @@ psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, psa_register_read(selected_slot); status = psa_wipe_key_slot(selected_slot); if (status != PSA_SUCCESS) { - goto exit; + goto error; } } @@ -206,27 +202,21 @@ psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, status = psa_key_slot_state_transition(selected_slot, PSA_SLOT_EMPTY, PSA_SLOT_FILLING); if (status != PSA_SUCCESS) { - goto exit; + goto error; } *volatile_key_id = PSA_KEY_ID_VOLATILE_MIN + ((psa_key_id_t) (selected_slot - global_data.key_slots)); *p_slot = selected_slot; - goto exit; + return PSA_SUCCESS; } status = PSA_ERROR_INSUFFICIENT_MEMORY; -exit: - if (status != PSA_SUCCESS) { - *p_slot = NULL; - *volatile_key_id = 0; - } +error: + *p_slot = NULL; + *volatile_key_id = 0; -#if defined(MBEDTLS_THREADING_C) - PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( - &mbedtls_threading_key_slot_mutex)); -#endif return status; } diff --git a/library/psa_crypto_slot_management.h b/library/psa_crypto_slot_management.h index 18a914496..585de1318 100644 --- a/library/psa_crypto_slot_management.h +++ b/library/psa_crypto_slot_management.h @@ -107,6 +107,9 @@ void psa_wipe_all_key_slots(void); * It is the responsibility of the caller to change the slot's state to * PSA_SLOT_EMPTY/FULL once key creation has finished. * + * 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] p_slot On success, a pointer to the slot. From 73feaf2682b63b7c405c0360c26f3e066c2f465a Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 14 Feb 2024 11:36:41 +0000 Subject: [PATCH 5/5] Comment on locking strategy in psa_fail_key_creation Signed-off-by: Ryan Everett --- library/psa_crypto.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 5300126c3..67f6eac6f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1900,6 +1900,9 @@ static void psa_fail_key_creation(psa_key_slot_t *slot, } #if defined(MBEDTLS_THREADING_C) + /* If the lock operation fails we still wipe the slot. + * Operations will no longer work after a failed lock, + * but we still need to wipe the slot of confidential data. */ mbedtls_mutex_lock(&mbedtls_threading_key_slot_mutex); #endif