Merge pull request #8773 from Ryan-Everett-arm/threadsafe-key-locking

Make key locking and one-shot operations thread safe

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)
+ * It is the responsibility of the caller to then unregister
- * when they have finished reading the contents of the slot.
+ * 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)
+ * It is the responsibility of the caller to then unregister
- * when they have finished reading the contents of the slot.
+ * 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;
 }

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.
      *

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
-         * Check if both the PSA key identifier key_id and the owner
+         * identifier of key match those of the key slot. */
-         * identifier of key match those of the key slot.
+        if ((slot->state == PSA_SLOT_FULL) &&
-         *
+            (mbedtls_svc_key_id_equal(key, slot->attr.id))) {
-         * Note that, if the key slot is not occupied, its PSA key identifier
+            status = PSA_SUCCESS;
-         * is equal to zero. This is an invalid value for a PSA key identifier
+        } else {
-         * and thus cannot be equal to the valid PSA key identifier key_id.
+            status = PSA_ERROR_DOES_NOT_EXIST;
-         */
+        }
-        status = mbedtls_svc_key_id_equal(key, slot->attr.id) ?
-                 PSA_SUCCESS : 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)
 {

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.