diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 67f6eac6f0..6cd6557d9e 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, @@ -954,7 +959,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; } @@ -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, @@ -986,7 +996,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; } @@ -1319,7 +1329,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; } @@ -1415,7 +1425,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; } @@ -1529,7 +1539,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; } @@ -2234,7 +2244,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; } @@ -2741,7 +2751,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; } @@ -2885,7 +2895,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; } @@ -2933,7 +2943,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; @@ -3200,7 +3210,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; } @@ -3252,7 +3262,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; } @@ -4323,7 +4333,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; } @@ -4384,7 +4394,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; } @@ -4510,7 +4520,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; } @@ -4565,7 +4575,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; } @@ -7269,7 +7279,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; } 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. * diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index dc38662e1b..f7b7fbe238 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; @@ -248,11 +249,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 +258,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 +330,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); @@ -358,12 +348,27 @@ 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) + /* 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 + * 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; } @@ -374,6 +379,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; } @@ -407,10 +416,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) @@ -447,6 +461,24 @@ 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) + /* 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 + 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 585de13184..bcfc9d8adc 100644 --- a/library/psa_crypto_slot_management.h +++ b/library/psa_crypto_slot_management.h @@ -205,6 +205,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.