From dd90507dc679284d1c99f3e18bafeb51e539e221 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Mon, 29 Apr 2024 18:24:58 +0100 Subject: [PATCH 1/4] Fix potential non-NULL slot return on failure If psa_get_and_lock_key_slot fails, the slot must be wiped. This fixes a bug where a pointer to some valid key slot can be incorrectly returned Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index b184ed08c9..fbcb26ebc8 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -440,6 +440,9 @@ psa_status_t psa_get_and_lock_key_slot(mbedtls_svc_key_id_t key, status = PSA_ERROR_INVALID_HANDLE; #endif /* MBEDTLS_PSA_CRYPTO_STORAGE_C || MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS */ + if (status != PSA_SUCCESS) { + *p_slot = NULL; + } #if defined(MBEDTLS_THREADING_C) PSA_THREADING_CHK_RET(mbedtls_mutex_unlock( &mbedtls_threading_key_slot_mutex)); From 04e2b04f7fd11eb70f686feab62d4daefb6c5a9c Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Mon, 29 Apr 2024 18:26:19 +0100 Subject: [PATCH 2/4] Explicitly document return behaviour A bug existed previously where this guarantee was not met, causing some issues in multi-threaded code. Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/psa_crypto_slot_management.h b/library/psa_crypto_slot_management.h index bcfc9d8adc..a84be7d837 100644 --- a/library/psa_crypto_slot_management.h +++ b/library/psa_crypto_slot_management.h @@ -58,6 +58,9 @@ static inline int psa_key_id_is_volatile(psa_key_id_t key_id) * It is the responsibility of the caller to call psa_unregister_read(slot) * when they have finished reading the contents of the slot. * + * On failure, `*p_slot` is set to NULL. This ensures that it is always valid + * to call psa_unregister_read on the returned slot. + * * \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 From 925b2d76f4b680776044e9a3dfbdb8a6f7107335 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Mon, 29 Apr 2024 18:29:48 +0100 Subject: [PATCH 3/4] Clarify psa_get_and_lock_key_slot return behaviour Signed-off-by: Ryan Everett --- library/psa_crypto_slot_management.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index fbcb26ebc8..9986a44969 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -424,6 +424,8 @@ psa_status_t psa_get_and_lock_key_slot(mbedtls_svc_key_id_t key, if (status != PSA_SUCCESS) { psa_wipe_key_slot(*p_slot); + /* If the key does not exist, we need to return + * PSA_ERROR_INVALID_HANDLE. */ if (status == PSA_ERROR_DOES_NOT_EXIST) { status = PSA_ERROR_INVALID_HANDLE; } From c51e94837032ea71b13c5b5d225b5b18880d6455 Mon Sep 17 00:00:00 2001 From: Ryan Everett Date: Tue, 30 Apr 2024 14:04:17 +0100 Subject: [PATCH 4/4] Add changelog Signed-off-by: Ryan Everett --- ChangeLog.d/fix-concurrently-loading-non-existent-keys.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/fix-concurrently-loading-non-existent-keys.txt diff --git a/ChangeLog.d/fix-concurrently-loading-non-existent-keys.txt b/ChangeLog.d/fix-concurrently-loading-non-existent-keys.txt new file mode 100644 index 0000000000..8a406a12e8 --- /dev/null +++ b/ChangeLog.d/fix-concurrently-loading-non-existent-keys.txt @@ -0,0 +1,4 @@ +Bugfix + * Fix rare concurrent access bug where attempting to operate on a + non-existent key while concurrently creating a new key could potentially + corrupt the key store.