Enforce slot lock/unlock pairing robustness through documentation and testing

[gilles-peskine-arm]

Functions in psa_crypto.c that access a key slot call psa_lock_key_slot (through one of its wrapper functions such as psa_get_and_lock_key_slot, psa_get_and_lock_key_slot_with_policy, etc.) before accessing the slot, and psa_unlock_key_slot on exit. Each lock call must be paired with an unlock call.

If the locking fail, the function's cleanup code must not call psa_unlock_key_slot: this would decrement the lock count if nonzero, i.e. it would release another user's lock. And when the lock count reaches 0, this asserts when MBEDTLS_CHECK_PARAMS is defined. If the locking fails, psa_unlock_key_slot is called with slot == NULL which is safe. We should make sure that this property is documented and validated so that we don't accidentally break it in a refactoring.

Goals of this task:

• Define an implement a way to detect in a systematic way calls to lock without unlock or to unlock without lock. This may be done by hooking into the lock and unlock functions to count calls and possibly lock failure.
• Possibly change the lock/unlock interface to make it easier to use.
• Note in the documentation of the lock functions that calling unlock is always safe.
• Fix any bug revealed by the tests.

We may want to fix bugs early if we find them by review. 6a8f5f7#r621153108 fixed one such case in psa_mac_setup. By code inspection, I find unlock without lock in the following functions: psa_copy_key, psa_sign_hash, psa_cipher_setup, psa_raw_key_agreement. I didn't find any lock without unlock.

[ronald-cron-arm]

Looking at the change in 6a8f5f7, I don't think there was a bug as when psa_get_and_lock_key_slot_with_policy returns in error slot is set to NULL and thus calling psa_unlock_key_slot does not have any effect. This property of psa_get_and_lock_key_slot_with_policy and psa_get_and_lock_key_slot does not seem to be documented enough though.

[gilles-peskine-arm]

Ah, right, good point. In fact I guess you took this into account in #3547. The documentation of psa_unlock_key_slot even states that “To ease the handling of errors in retrieving a key slot a NULL input pointer is valid”. But it would indeed be worth noting in the documentation of the lock functions that it's always correct to call unlock in the standard pattern, whether they succeed or not.