mbedtls icon indicating copy to clipboard operation
mbedtls copied to clipboard

Make key loading thread safe

Open Ryan-Everett-arm opened this issue 1 year ago • 3 comments

Description

Make functions which transition the state of key slots from PSA_SLOT_EMPTY to another state thread safe. Make our implementation of the PSA key creation API thread safe. Resolves #8675.

The work of this PR and the issues #8676, #8677 and #8412 is to ensure that API calls are linearizable and correct. To do this we rely on some properties, these properties need to be enforced. The relevant properties are:

  • All calls to psa_register_read, psa_unregister_read, psa_key_slot_state_transition, psa_wipe_key_slot and psa_key_slot_has_readers are performed under the global mutex.
  • Every API call has at most one point in time where effects are made visible to other threads. By effects here we mean something like a key being created and now existing in the eyes of other threads, or a key being destroyed. We do not need to consider resource constraints here.
  • Any function which iterates through key slots or finds a key slot containing X ignores all slots in the FILLING or PENDING_DELETION state. Whoever sets a slot to FILLING directly or via psa_reserve_free_key_slot has exclusive access to the slot. A slot in PENDING_DELETION can only be read by the registered readers, and must be wiped upon the final reader unregistering.
  • If a slot in the FULL state has more than 1 registered reader then it cannot be wiped (with the exception of a call to mbedtls_psa_cypto_free, a non-threadsafe function).
  • No key can be in multiple FULL slots at the same time, each FULL slot must hold a unique key ID.
  • Other threads running other API calls mustn't interfere in a destructive way.

(See the thread safety document for more details.) This PR must make the key creation functions enforce these properties.

Note for reviewers

This pr only directly changes three functions, but the scope of this issue requires checking that these changes ensure that the following functions are thread safe under the assumption that functions not in this list enforce the above properties, here is the list along with my reasoning; please check each function:

  • psa_start_key_creation: This reserves a slot, sets some metadata and optionally does some driver work. Please check carefully that the driver work doesn't break the above properties, see the thread safety document for our approach to thread safe drivers.
  • psa_import_key, psa_key_derivation_output_key, psa_generate_key, mbedtls_psa_register_se_key: These set an unused slot's state to FILLING, operates on it without the mutex, and then makes the slot FULL/fails and makes it EMPTY. This is a standard pattern. The linearization point is the unlock of the finish/fail call.
  • psa_copy_key: The call to get_and_lock_X is made thread safe as part of #8676. We ensure the source slot can be read from (by registering as a reader) and the target slot can be written to (by marking it as FILLING), then this follows the standard pattern. Linearizes at finish/fail.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • [x] changelog not required for individual tasks on this epic
  • [x] backport not required
  • [x] tests To be added in #8420

Ryan-Everett-arm avatar Jan 24 '24 09:01 Ryan-Everett-arm

There are some inefficiencies in the locking here. We hold the mutex for some operations which we do not strictly need to hold it for when loading persistent keys (see psa_finish_key_creation). But while we do not have testing I thought it best to keep things simple, I can make an issue to track these inefficiencies - or refactor now if that is preferred.

Ryan-Everett-arm avatar Jan 25 '24 17:01 Ryan-Everett-arm

Just to clarify. Do not review the first 4 commits, these are part of a separate PR which has been approved.

Ryan-Everett-arm avatar Jan 29 '24 13:01 Ryan-Everett-arm

I am reverting the changes to psa_reserve_free_key_slot and instead will wrap the call in a LOCK; call; UNLOCK; style. This is a minor change for this PR (since this function holds the mutex at nearly all time already) which slightly increases time spent under the lock. But it makes #8676 more efficient as it means we can avoid looping through the array of all key slots twice in specific cases.

Ryan-Everett-arm avatar Jan 30 '24 16:01 Ryan-Everett-arm