From f23336e0406e719fe5d736e2b464e22468ca0f3b Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 24 Jan 2024 11:39:21 +0000 Subject: [PATCH 1/9] Make psa_close_key thread safe There are two mutex locks here, the one performed in get_and_lock.. and the one performed outside. Linearizes at the final unlock. (This function is deprecated) Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 47ace359d..3bb2691c6 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -536,11 +536,22 @@ psa_status_t psa_close_key(psa_key_handle_t handle) return status; } + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_lock( + &mbedtls_threading_key_slot_mutex)); +#endif if (slot->registered_readers == 1) { - return psa_wipe_key_slot(slot); + status = psa_wipe_key_slot(slot); } else { - return psa_unregister_read(slot); + status = psa_unregister_read(slot); } +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif + + return status; } psa_status_t psa_purge_key(mbedtls_svc_key_id_t key) From b0821959ae4a742de79d834bd71bc3cd1952fb86 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 24 Jan 2024 11:42:32 +0000 Subject: [PATCH 2/9] Make psa_purge_key thread safe Relies on get_and_lock_X being thread safe. There are two mutex locks here, one in psa_get_and_lock... Linearization point is the final unlock (or first lock on failure). Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 3bb2691c6..e8813b901 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -564,12 +564,22 @@ psa_status_t psa_purge_key(mbedtls_svc_key_id_t key) return status; } +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_lock( + &mbedtls_threading_key_slot_mutex)); +#endif if ((!PSA_KEY_LIFETIME_IS_VOLATILE(slot->attr.lifetime)) && (slot->registered_readers == 1)) { - return psa_wipe_key_slot(slot); + status = psa_wipe_key_slot(slot); } else { - return psa_unregister_read(slot); + status = psa_unregister_read(slot); } +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif + + return status; } void mbedtls_psa_get_stats(mbedtls_psa_stats_t *stats) From 16abd59a62522852423a35c2b96a087676e6a7ad Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 24 Jan 2024 17:37:46 +0000 Subject: [PATCH 3/9] Update psa_wipe_all_key_slots and document non-thread safety This function, and mbedtls_psa_crypto_free, are not thread safe as they wipe slots regardless of state. They are not part of the PSA Crypto API, untrusted applications cannot call these functions in a crypto service. In a service intergration, mbedtls_psa_crypto_free on the client cuts the communication with the crypto service. Signed-off-by: Ryan Everett --- include/psa/crypto_extra.h | 2 ++ library/psa_crypto_slot_management.c | 6 ++++++ library/psa_crypto_slot_management.h | 2 ++ 3 files changed, 10 insertions(+) diff --git a/include/psa/crypto_extra.h b/include/psa/crypto_extra.h index fc9bf4f0f..18dccae0a 100644 --- a/include/psa/crypto_extra.h +++ b/include/psa/crypto_extra.h @@ -198,6 +198,8 @@ psa_status_t mbedtls_psa_register_se_key( * * This function clears all data associated with the PSA layer, * including the whole key store. + * 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. * * This is an Mbed TLS extension. */ diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index e8813b901..599cc363b 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -144,6 +144,9 @@ void psa_wipe_all_key_slots(void) { size_t slot_idx; +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_lock(&mbedtls_threading_key_slot_mutex); +#endif 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; @@ -151,6 +154,9 @@ void psa_wipe_all_key_slots(void) (void) psa_wipe_key_slot(slot); } global_data.key_slots_initialized = 0; +#if defined(MBEDTLS_THREADING_C) + mbedtls_mutex_unlock(&mbedtls_threading_key_slot_mutex); +#endif } psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, diff --git a/library/psa_crypto_slot_management.h b/library/psa_crypto_slot_management.h index 002429b93..18a914496 100644 --- a/library/psa_crypto_slot_management.h +++ b/library/psa_crypto_slot_management.h @@ -92,6 +92,8 @@ psa_status_t psa_get_and_lock_key_slot(mbedtls_svc_key_id_t key, psa_status_t psa_initialize_key_slots(void); /** 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. * * This does not affect persistent storage. */ void psa_wipe_all_key_slots(void); From c053d968f2b90926a6c69b079fc35d25713005fb Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Thu, 25 Jan 2024 17:56:32 +0000 Subject: [PATCH 4/9] Make psa_destroy_key threadsafe We do not require linearizability in the case of destroying a key in use. Using a key and destroying it simultaneously will not cause any issues as the user will only use the copy of the key in the slot. Two simulatenous deletion calls to one key cannot interfere, the first caller sets the slot's state to PENDING_DELETION, the second caller will back off. Remove outdated comment about one key being in multiple slots, psa_open_key does not put the key into a new slot. Signed-off-by: Ryan Everett --- library/psa_crypto.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e6d3851ba..c81666818 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1071,6 +1071,10 @@ psa_status_t psa_destroy_key(mbedtls_svc_key_id_t key) return status; } +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_lock( + &mbedtls_threading_key_slot_mutex)); +#endif /* Set the key slot containing the key description's state to * PENDING_DELETION. This stops new operations from registering * to read the slot. Current readers can safely continue to access @@ -1079,7 +1083,12 @@ psa_status_t psa_destroy_key(mbedtls_svc_key_id_t key) * If the key is persistent, we can now delete the copy of the key * from memory. If the key is opaque, we require the driver to * deal with the deletion. */ - slot->state = PSA_SLOT_PENDING_DELETION; + status = psa_key_slot_state_transition(slot, PSA_SLOT_FULL, + PSA_SLOT_PENDING_DELETION); + + if (status != PSA_SUCCESS) { + goto exit; + } if (PSA_KEY_LIFETIME_IS_READ_ONLY(slot->attr.lifetime)) { /* Refuse the destruction of a read-only key (which may or may not work @@ -1134,11 +1143,6 @@ psa_status_t psa_destroy_key(mbedtls_svc_key_id_t key) if (overall_status == PSA_SUCCESS) { overall_status = status; } - - /* TODO: other slots may have a copy of the same key. We should - * invalidate them. - * https://github.com/ARMmbed/mbed-crypto/issues/214 - */ } #endif /* defined(MBEDTLS_PSA_CRYPTO_STORAGE_C) */ @@ -1159,8 +1163,14 @@ exit: /* Unregister from reading the slot. If we are the last active reader * then this will wipe the slot. */ status = psa_unregister_read(slot); - /* Prioritize CORRUPTION_DETECTED from unregistering over - * a storage error. */ + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif + + /* Prioritize CORRUPTION_DETECTED from unregistering or + * SERVICE_FAILURE from unlocking over a storage error. */ if (status != PSA_SUCCESS) { overall_status = status; } From 763971f32ec317c5c8c6248f39d2f30cee3a93b5 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Mon, 29 Jan 2024 17:13:36 +0000 Subject: [PATCH 5/9] Comment on locking strategy in psa_destroy_key Signed-off-by: Ryan Everett --- library/psa_crypto.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index c81666818..9d7b72f87 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1072,6 +1072,10 @@ psa_status_t psa_destroy_key(mbedtls_svc_key_id_t key) } #if defined(MBEDTLS_THREADING_C) + /* We cannot unlock between setting the state to PENDING_DELETION + * and destroying the key in storage, as otherwise another thread + * could load the key into a new slot and the key will not be + * fully destroyed. */ PSA_THREADING_CHK_GOTO_EXIT(mbedtls_mutex_lock( &mbedtls_threading_key_slot_mutex)); #endif From 3af9bc18f3be7a3e3cc52aa5f08dd4c7b92ddcb6 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Tue, 30 Jan 2024 17:21:57 +0000 Subject: [PATCH 6/9] Wrap get_and_lock_key_slot_in_memory calls in mutex It is useful to do this for the call in get_and_lock_key_slot. Documenting that get_and_lock_key_slot_in_memory requires the mutex is not part of this PR Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 31 +++++++++++++++++----------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 599cc363b..f4c6ee005 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -527,26 +527,29 @@ psa_status_t psa_open_key(mbedtls_svc_key_id_t key, psa_key_handle_t *handle) psa_status_t psa_close_key(psa_key_handle_t handle) { - psa_status_t status; + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_key_slot_t *slot; if (psa_key_handle_is_null(handle)) { return PSA_SUCCESS; } +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_lock( + &mbedtls_threading_key_slot_mutex)); +#endif status = psa_get_and_lock_key_slot_in_memory(handle, &slot); if (status != PSA_SUCCESS) { if (status == PSA_ERROR_DOES_NOT_EXIST) { status = PSA_ERROR_INVALID_HANDLE; } - +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif return status; } -#if defined(MBEDTLS_THREADING_C) - PSA_THREADING_CHK_RET(mbedtls_mutex_lock( - &mbedtls_threading_key_slot_mutex)); -#endif if (slot->registered_readers == 1) { status = psa_wipe_key_slot(slot); } else { @@ -562,18 +565,22 @@ psa_status_t psa_close_key(psa_key_handle_t handle) psa_status_t psa_purge_key(mbedtls_svc_key_id_t key) { - psa_status_t status; + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; psa_key_slot_t *slot; - status = psa_get_and_lock_key_slot_in_memory(key, &slot); - if (status != PSA_SUCCESS) { - return status; - } - #if defined(MBEDTLS_THREADING_C) PSA_THREADING_CHK_RET(mbedtls_mutex_lock( &mbedtls_threading_key_slot_mutex)); #endif + status = psa_get_and_lock_key_slot_in_memory(key, &slot); + if (status != PSA_SUCCESS) { +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif + return status; + } + if ((!PSA_KEY_LIFETIME_IS_VOLATILE(slot->attr.lifetime)) && (slot->registered_readers == 1)) { status = psa_wipe_key_slot(slot); From a76a0011aba1b192df04b710ae876f4395381439 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Tue, 6 Feb 2024 16:45:54 +0000 Subject: [PATCH 7/9] Remove mutex calls in psa_wipe_all_key_slots Code size and code style improvement, these calls aren't needed. Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index f4c6ee005..9890de622 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -144,9 +144,6 @@ void psa_wipe_all_key_slots(void) { size_t slot_idx; -#if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_lock(&mbedtls_threading_key_slot_mutex); -#endif 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; @@ -154,9 +151,6 @@ void psa_wipe_all_key_slots(void) (void) psa_wipe_key_slot(slot); } global_data.key_slots_initialized = 0; -#if defined(MBEDTLS_THREADING_C) - mbedtls_mutex_unlock(&mbedtls_threading_key_slot_mutex); -#endif } psa_status_t psa_reserve_free_key_slot(psa_key_id_t *volatile_key_id, From 7fee4f731895aa13a11dd353ead4ee9e9e260e9e Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Fri, 9 Feb 2024 14:11:27 +0000 Subject: [PATCH 8/9] Fix mutex unlock error handling in psa_destroy_key Signed-off-by: Ryan Everett --- library/psa_crypto.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 9d7b72f87..27ea3b84c 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1167,17 +1167,19 @@ exit: /* Unregister from reading the slot. If we are the last active reader * then this will wipe the slot. */ status = psa_unregister_read(slot); + /* Prioritize CORRUPTION_DETECTED from unregistering over + * a storage error. */ + if (status != PSA_SUCCESS) { + overall_status = status; + } #if defined(MBEDTLS_THREADING_C) + /* Don't overwrite existing errors if the unlock fails. */ + status = overall_status; PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( &mbedtls_threading_key_slot_mutex)); #endif - /* Prioritize CORRUPTION_DETECTED from unregistering or - * SERVICE_FAILURE from unlocking over a storage error. */ - if (status != PSA_SUCCESS) { - overall_status = status; - } return overall_status; } From 9dc076b4f49ceedb2bfae13c74ae58c3251d1a95 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Fri, 9 Feb 2024 14:20:09 +0000 Subject: [PATCH 9/9] Fix issue with lock failures returning CORRUPTION_DETECTED Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 9890de622..dc38662e1 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -529,6 +529,9 @@ psa_status_t psa_close_key(psa_key_handle_t handle) } #if defined(MBEDTLS_THREADING_C) + /* We need to set status as success, otherwise CORRUPTION_DETECTED + * would be returned if the lock fails. */ + status = PSA_SUCCESS; PSA_THREADING_CHK_RET(mbedtls_mutex_lock( &mbedtls_threading_key_slot_mutex)); #endif @@ -563,6 +566,9 @@ psa_status_t psa_purge_key(mbedtls_svc_key_id_t key) psa_key_slot_t *slot; #if defined(MBEDTLS_THREADING_C) + /* We need to set status as success, otherwise CORRUPTION_DETECTED + * would be returned if the lock fails. */ + status = PSA_SUCCESS; PSA_THREADING_CHK_RET(mbedtls_mutex_lock( &mbedtls_threading_key_slot_mutex)); #endif