From 5c5210f7e1be8b26cf85eb5fc624692e1d24e647 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Tue, 30 Jan 2024 18:27:16 +0000 Subject: [PATCH 1/8] Remove state transitions in psa_load_X_key_into_slot These calls otherwise don't abide by our assertions about changing the slot. psa_get_and_lock_key_slot, the only caller to these, handles the state transitions already. It also changes the contents of the slot after these calls. Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 47ace359d7..93586195e4 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -248,11 +248,6 @@ static psa_status_t psa_load_persistent_key_into_slot(psa_key_slot_t *slot) data = (psa_se_key_data_storage_t *) key_data; status = psa_copy_key_material_into_slot( slot, data->slot_number, sizeof(data->slot_number)); - - if (status == PSA_SUCCESS) { - status = psa_key_slot_state_transition(slot, PSA_SLOT_FILLING, - PSA_SLOT_FULL); - } goto exit; } #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ @@ -262,9 +257,6 @@ static psa_status_t psa_load_persistent_key_into_slot(psa_key_slot_t *slot) goto exit; } - status = psa_key_slot_state_transition(slot, PSA_SLOT_FILLING, - PSA_SLOT_FULL); - exit: psa_free_persistent_key_data(key_data, key_data_length); return status; @@ -337,9 +329,6 @@ static psa_status_t psa_load_builtin_key_into_slot(psa_key_slot_t *slot) /* Copy actual key length and core attributes into the slot on success */ slot->key.bytes = key_buffer_length; slot->attr = attributes.core; - - status = psa_key_slot_state_transition(slot, PSA_SLOT_FILLING, - PSA_SLOT_FULL); exit: if (status != PSA_SUCCESS) { psa_remove_key_data_from_memory(slot); From 6ad1fd133f257429d0b6a2876c29ea0daf9b2893 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 31 Jan 2024 13:21:33 +0000 Subject: [PATCH 2/8] Update psa_get_and_lock_key_slot_in_memory Document that callers must hold the key slot mutex. Change the volatile key checking behaviour so that it doesn't read the contents of non-FULL slots. Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 93586195e4..2a73c0af15 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -70,6 +70,9 @@ int psa_is_valid_key_id(mbedtls_svc_key_id_t key, int vendor_ok) * On success, the function locks the key slot. It is the responsibility of * the caller to unlock the key slot when it does not access it anymore. * + * If multi-threading is enabled, the caller must hold the + * global key slot mutex. + * * \param key Key identifier to query. * \param[out] p_slot On success, `*p_slot` contains a pointer to the * key slot containing the description of the key @@ -94,16 +97,14 @@ static psa_status_t psa_get_and_lock_key_slot_in_memory( if (psa_key_id_is_volatile(key_id)) { slot = &global_data.key_slots[key_id - PSA_KEY_ID_VOLATILE_MIN]; - /* - * Check if both the PSA key identifier key_id and the owner - * identifier of key match those of the key slot. - * - * Note that, if the key slot is not occupied, its PSA key identifier - * is equal to zero. This is an invalid value for a PSA key identifier - * and thus cannot be equal to the valid PSA key identifier key_id. - */ - status = mbedtls_svc_key_id_equal(key, slot->attr.id) ? - PSA_SUCCESS : PSA_ERROR_DOES_NOT_EXIST; + /* 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))) { + status = PSA_SUCCESS; + } else { + status = PSA_ERROR_DOES_NOT_EXIST; + } } else { if (!psa_is_valid_key_id(key, 1)) { return PSA_ERROR_INVALID_HANDLE; From 2f1f17201dbf9242c07cfb4ac781e637a0e314af Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 31 Jan 2024 13:31:00 +0000 Subject: [PATCH 3/8] Make psa_get_and_lock_key_slot threadsafe Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 2a73c0af15..fd33ad9a6b 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -348,12 +348,24 @@ psa_status_t psa_get_and_lock_key_slot(mbedtls_svc_key_id_t key, return PSA_ERROR_BAD_STATE; } +#if defined(MBEDTLS_THREADING_C) + /* If the key is persistent and not loaded, we cannot unlock the mutex + * between checking if the key is loaded and setting the slot as FULL, + * as otherwise another thread may load and then destroy the key + * in the meantime. */ + PSA_THREADING_CHK_RET(mbedtls_mutex_lock( + &mbedtls_threading_key_slot_mutex)); +#endif /* * On success, the pointer to the slot is passed directly to the caller * thus no need to unlock the key slot here. */ status = psa_get_and_lock_key_slot_in_memory(key, p_slot); if (status != PSA_ERROR_DOES_NOT_EXIST) { +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif return status; } @@ -364,6 +376,10 @@ psa_status_t psa_get_and_lock_key_slot(mbedtls_svc_key_id_t key, status = psa_reserve_free_key_slot(&volatile_key_id, p_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; } @@ -397,10 +413,15 @@ psa_status_t psa_get_and_lock_key_slot(mbedtls_svc_key_id_t key, status = psa_register_read(*p_slot); } - return status; #else /* MBEDTLS_PSA_CRYPTO_STORAGE_C || MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS */ - return PSA_ERROR_INVALID_HANDLE; + status = PSA_ERROR_INVALID_HANDLE; #endif /* MBEDTLS_PSA_CRYPTO_STORAGE_C || MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS */ + +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( + &mbedtls_threading_key_slot_mutex)); +#endif + return status; } psa_status_t psa_unregister_read(psa_key_slot_t *slot) From eb1722a2b9ab4660992ef306566be1bc7fc5af2a Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 31 Jan 2024 13:36:39 +0000 Subject: [PATCH 4/8] Add a wrapper function for psa_unregister_read There are at least 20 occurences in the current code where we will need this pattern of code, so I thought it best to put this in a function Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 15 +++++++++++++++ library/psa_crypto_slot_management.h | 21 +++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index fd33ad9a6b..53ebf3177d 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -458,6 +458,21 @@ psa_status_t psa_unregister_read(psa_key_slot_t *slot) return PSA_ERROR_CORRUPTION_DETECTED; } +psa_status_t psa_unregister_read_under_mutex(psa_key_slot_t *slot) +{ + psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; +#if defined(MBEDTLS_THREADING_C) + PSA_THREADING_CHK_RET(mbedtls_mutex_lock( + &mbedtls_threading_key_slot_mutex)); +#endif + 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_validate_key_location(psa_key_lifetime_t lifetime, psa_se_drv_table_entry_t **p_drv) { diff --git a/library/psa_crypto_slot_management.h b/library/psa_crypto_slot_management.h index 002429b933..c6ba68b23f 100644 --- a/library/psa_crypto_slot_management.h +++ b/library/psa_crypto_slot_management.h @@ -200,6 +200,27 @@ static inline psa_status_t psa_register_read(psa_key_slot_t *slot) */ psa_status_t psa_unregister_read(psa_key_slot_t *slot); +/** Wrap a call to psa_unregister_read in the global key slot mutex. + * + * If threading is disabled, this simply calls psa_unregister_read. + * + * \note To ease the handling of errors in retrieving a key slot + * a NULL input pointer is valid, and the function returns + * successfully without doing anything in that case. + * + * \param[in] slot The key slot. + * \retval #PSA_SUCCESS + * \p slot is NULL or the key slot reader counter has been + * decremented (and potentially wiped) successfully. + * \retval #PSA_ERROR_CORRUPTION_DETECTED + * The slot's state was neither PSA_SLOT_FULL nor + * PSA_SLOT_PENDING_DELETION. + * Or a wipe was attempted and the slot's state was not + * PSA_SLOT_PENDING_DELETION. + * Or registered_readers was equal to 0. + */ +psa_status_t psa_unregister_read_under_mutex(psa_key_slot_t *slot); + /** Test whether a lifetime designates a key in an external cryptoprocessor. * * \param lifetime The lifetime to test. From fb792cad31f843c5924869cb99bc3cef05edd6ca Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 31 Jan 2024 13:40:05 +0000 Subject: [PATCH 5/8] Make psa_get_and_lock_X_with_policy threadsafe Between the call to psa_get_and_lock_key_slot and psa_unregister_read we only read the contents of a slot which we are registered to read, so no extra mutex taking is needed. Signed-off-by: Ryan Everett --- library/psa_crypto.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e6d3851ba8..e3b2be6fe4 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -936,7 +936,7 @@ static psa_status_t psa_get_and_lock_key_slot_with_policy( error: *p_slot = NULL; - psa_unregister_read(slot); + psa_unregister_read_under_mutex(slot); return status; } @@ -968,7 +968,7 @@ static psa_status_t psa_get_and_lock_transparent_key_slot_with_policy( } if (psa_key_lifetime_is_external((*p_slot)->attr.lifetime)) { - psa_unregister_read(*p_slot); + psa_unregister_read_under_mutex(*p_slot); *p_slot = NULL; return PSA_ERROR_NOT_SUPPORTED; } From a103ec9ad45465579ebf082c9b460920ed3cf78d Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Wed, 31 Jan 2024 13:59:57 +0000 Subject: [PATCH 6/8] Make one shot operations thread safe These all follow a pattern of locking some key slot, reading its contents, and then unregistering from reading the slot. psa_copy_key also writes to another slot, but calls the functions needed to be threadsafe. Signed-off-by: Ryan Everett --- library/psa_crypto.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e3b2be6fe4..5e35fac4b3 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1285,7 +1285,7 @@ psa_status_t psa_get_key_attributes(mbedtls_svc_key_id_t key, psa_reset_key_attributes(attributes); } - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); return (status == PSA_SUCCESS) ? unlock_status : status; } @@ -1381,7 +1381,7 @@ psa_status_t psa_export_key(mbedtls_svc_key_id_t key, slot->key.data, slot->key.bytes, data, data_size, data_length); - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); return (status == PSA_SUCCESS) ? unlock_status : status; } @@ -1495,7 +1495,7 @@ psa_status_t psa_export_public_key(mbedtls_svc_key_id_t key, data, data_size, data_length); exit: - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); return (status == PSA_SUCCESS) ? unlock_status : status; } @@ -2167,7 +2167,7 @@ exit: psa_fail_key_creation(target_slot, driver); } - unlock_status = psa_unregister_read(source_slot); + unlock_status = psa_unregister_read_under_mutex(source_slot); return (status == PSA_SUCCESS) ? unlock_status : status; } @@ -2674,7 +2674,7 @@ exit: psa_wipe_tag_output_buffer(mac, status, mac_size, *mac_length); - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); return (status == PSA_SUCCESS) ? unlock_status : status; } @@ -2818,7 +2818,7 @@ exit: psa_wipe_tag_output_buffer(signature, status, signature_size, *signature_length); - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); return (status == PSA_SUCCESS) ? unlock_status : status; } @@ -2866,7 +2866,7 @@ static psa_status_t psa_verify_internal(mbedtls_svc_key_id_t key, signature, signature_length); } - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); return (status == PSA_SUCCESS) ? unlock_status : status; @@ -3133,7 +3133,7 @@ psa_status_t psa_asymmetric_encrypt(mbedtls_svc_key_id_t key, alg, input, input_length, salt, salt_length, output, output_size, output_length); exit: - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); return (status == PSA_SUCCESS) ? unlock_status : status; } @@ -3185,7 +3185,7 @@ psa_status_t psa_asymmetric_decrypt(mbedtls_svc_key_id_t key, output, output_size, output_length); exit: - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); return (status == PSA_SUCCESS) ? unlock_status : status; } @@ -4256,7 +4256,7 @@ psa_status_t psa_cipher_encrypt(mbedtls_svc_key_id_t key, output_size - default_iv_length, output_length); exit: - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); if (status == PSA_SUCCESS) { status = unlock_status; } @@ -4317,7 +4317,7 @@ psa_status_t psa_cipher_decrypt(mbedtls_svc_key_id_t key, output, output_size, output_length); exit: - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); if (status == PSA_SUCCESS) { status = unlock_status; } @@ -4443,7 +4443,7 @@ psa_status_t psa_aead_encrypt(mbedtls_svc_key_id_t key, } exit: - psa_unregister_read(slot); + psa_unregister_read_under_mutex(slot); return status; } @@ -4498,7 +4498,7 @@ psa_status_t psa_aead_decrypt(mbedtls_svc_key_id_t key, } exit: - psa_unregister_read(slot); + psa_unregister_read_under_mutex(slot); return status; } @@ -7151,7 +7151,7 @@ exit: *output_length = output_size; } - unlock_status = psa_unregister_read(slot); + unlock_status = psa_unregister_read_under_mutex(slot); return (status == PSA_SUCCESS) ? unlock_status : status; } From 91ce79225381c83d4f9c04e09ebaeaa7504e8659 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Mon, 12 Feb 2024 12:17:28 +0000 Subject: [PATCH 7/8] Fix return code error when locking mutex 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 53ebf3177d..b0744bd4ec 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -349,6 +349,9 @@ psa_status_t psa_get_and_lock_key_slot(mbedtls_svc_key_id_t key, } #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; /* If the key is persistent and not loaded, we cannot unlock the mutex * between checking if the key is loaded and setting the slot as FULL, * as otherwise another thread may load and then destroy the key @@ -462,6 +465,9 @@ psa_status_t psa_unregister_read_under_mutex(psa_key_slot_t *slot) { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; #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 From 93cea578b9a44740d11a3571ecc736f14746c4a0 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Tue, 20 Feb 2024 18:01:29 +0000 Subject: [PATCH 8/8] Clarify which unregister operation needs to be used Signed-off-by: Ryan Everett --- library/psa_crypto.c | 18 ++++++++++++++---- library/psa_crypto_core.h | 4 +++- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 43b597c389..07edd0a0c0 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -909,8 +909,13 @@ static psa_status_t psa_restrict_key_policy( * into a key slot if not already done. * * On success, the returned key slot has been registered for reading. - * It is the responsibility of the caller to call psa_unregister_read(slot) - * when they have finished reading the contents of the slot. + * It is the responsibility of the caller to then unregister + * once they have finished reading the contents of the slot. + * The caller unregisters by calling psa_unregister_read() or + * psa_unregister_read_under_mutex(). psa_unregister_read() must be called + * if and only if the caller already holds the global key slot mutex + * (when mutexes are enabled). psa_unregister_read_under_mutex() encapsulates + * the unregister with mutex lock and unlock operations. */ static psa_status_t psa_get_and_lock_key_slot_with_policy( mbedtls_svc_key_id_t key, @@ -970,8 +975,13 @@ error: * for a cryptographic operation. * * On success, the returned key slot has been registered for reading. - * It is the responsibility of the caller to call psa_unregister_read(slot) - * when they have finished reading the contents of the slot. + * It is the responsibility of the caller to then unregister + * once they have finished reading the contents of the slot. + * The caller unregisters by calling psa_unregister_read() or + * psa_unregister_read_under_mutex(). psa_unregister_read() must be called + * if and only if the caller already holds the global key slot mutex + * (when mutexes are enabled). psa_unregister_read_under_mutex() encapsulates + * psa_unregister_read() with mutex lock and unlock operations. */ static psa_status_t psa_get_and_lock_transparent_key_slot_with_policy( mbedtls_svc_key_id_t key, diff --git a/library/psa_crypto_core.h b/library/psa_crypto_core.h index dc376d7ebf..0d7322ce5c 100644 --- a/library/psa_crypto_core.h +++ b/library/psa_crypto_core.h @@ -89,7 +89,9 @@ typedef struct { * 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. + * 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. *