mbedtls
mbedtls copied to clipboard
Make key loading thread safe
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
andpsa_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 thefinish
/fail
call. -
psa_copy_key
: The call toget_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
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.
Just to clarify. Do not review the first 4 commits, these are part of a separate PR which has been approved.
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.