Add testing for concurrently loading/using/destroying the same key #8924

Ryan-Everett-arm · 2024-03-12T18:31:55Z

Description

There are two parts to this PR. The first part is repetitive work which changes the key smoke tests to support the possibility that the key may be destroyed at any time. The second part is adding tests for various key behaviours.

We first add a new parameter key_destroyable to the exercise_X functions. When key_destroyable == 0 the behaviour is the same as before these changes, when key_destroyable == 1 the smoke tests will ignore any valid failures caused by the key suddenly not existing; unexpected failures like corruption will be reported.

The reason we are doing this is that it provides us a framework which we can use to test that we abide by our key access rules set in psa-thread-safety.md:

FULL: the slot contains a key, and any thread is able to use the key after registering as a reader.

PENDING_DELETION: the key within the slot has been destroyed or marked for destruction, but at least one thread is still registered as a reader. No thread can register to read this slot. The slot must not be wiped until the last reader de-registers, wiping the slot by calling psa_wipe_key_slot.

By running mbedtls_test_psa_exercise_key on the key in one thread, and concurrently destroying the key in another, TSan/internal errors will spot if these properties are being violated.

If an operation reads the slot contents without being registered - there will be a read/write conflict and the thread will try to read NULL data, causing an error.
If a thread wipes the slot before everyone has deregistered - the same will occur.

This PR adds testing for the following:

Exactly one thread succeeds when multiple threads attempt to import the same persistent key.
Exactly one thread succeeds when multiple threads attempt to destroy the same key (Fix threading bug when multiple destroy_key calls run on the same key #8912).
The correct error is returned when trying to access a free slot when no slot exists.
Any thread can use a key as soon as it is loaded by another thread.
Threads do not access the slot material when not registered as readers.
Threads do not wipe the contents of the slot while other threads are registered as readers (ensures destruction in use doesn't cause corruption).

Disclaimer: this doesn't test every API call or every code path. Exercise key can do a lot, but it doesn't cover all types of operation or all API calls. We also only do not cover all methods of loading a key, or all types of keys. Future work should add more functionality to exercise key, add more test data to this function, and add more test functions which use the other key management functions.

PR checklist

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

changelog provided
backport not required
tests yes

This will allow us to use this smoke test to ensure that key slot content reads are only performed when we are registered to read a full slot. We will destroy the key on another thread while the key is being exercised, and fail the test if an unexpected error code is hit. Future commits will incrementally implement this new parameter. All current usages of this function have this parameter set to 0, in which case the new behaviour must be the same as the old behaviour Signed-off-by: Ryan Everett <ryan.everett@arm.com>

This function is currently only used in the exercise_key smoke test. Signed-off-by: Ryan Everett <ryan.everett@arm.com>

If the key has been destroyed (and the new parameter is 1) then we test that psa_mac_abort succeeds in this scenario. Signed-off-by: Ryan Everett <ryan.everett@arm.com>

If the key has been destroyed (and the new parameter is 1), we test that psa_cipher_abort succeeds in this scenario. Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Signed-off-by: Ryan Everett <ryan.everett@arm.com>

All current usages have this parameter set to 0 (in this case the behaviour of the test is unchanged) Signed-off-by: Ryan Everett <ryan.everett@arm.com>

All current usages have this parameter set to 0 (meaning the behaviour of these tests hasn't changed). We also now return the actual error code, not GENERIC_ERROR Signed-off-by: Ryan Everett <ryan.everett@arm.com>

All current usages have this parameter set to 0 (this means the tests are unchanged). Remove the GENERIC_ERROR return behaviour, in favour of returning the actual status. Signed-off-by: Ryan Everett <ryan.everett@arm.com>

These are only called from mbedtls_test_psa_exercise_key Signed-off-by: Ryan Everett <ryan.everett@arm.com>

The thread functions can also be used in future tests for other key types and other test scenarios Signed-off-by: Ryan Everett <ryan.everett@arm.com>

There is a 1-1 correlation between these test cases and the test cases for import_and_exercise_key. Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Ryan-Everett-arm · 2024-03-13T14:42:29Z

In its current form this PR does not handle the potential INSUFFICIENT_MEMORY failure which can occur in exercise key when lots of threads attempt to load into a key slot. It shouldn't be merged before this is changed.

valeriosetti

I did a first pass to get more familiar with the PR and it looks almost OK to me. I only found very few comments. I will do a second pass tomorrow.

valeriosetti · 2024-03-14T14:42:43Z

tests/src/psa_exercise_key.c

+        /* The key has been destroyed. */
+        status = PSA_SUCCESS;
+        goto exit;
+    }


I think that PSA_ASSERT(status) is missing after this line in order to keep the same behavior as before in case the key has not been destroyed.

valeriosetti · 2024-03-14T15:25:29Z

tests/suites/test_suite_psa_crypto.function

+    same_key_context *skc = (struct same_key_context *) ctx;
+    psa_key_attributes_t got_attributes = PSA_KEY_ATTRIBUTES_INIT;
+
+    /* Import the key, exactly one thread must succceed. */


Typo: succeed

valeriosetti · 2024-03-14T15:28:09Z

tests/suites/test_suite_psa_crypto.function

+                if (skc->key_loaded) {
+                    mbedtls_mutex_unlock(&skc->key_loaded_mutex);
+                    /* More than one thread has succeeded, report a failure. */
+                    TEST_EQUAL(skc->key_loaded, 0);


Why not directly TEST_FAIL() in this case? I think it would be more explicit, but I might be missing something.
If you accept this change, then there is a very similar case also in thread_use_and_destroy_key() below

I wasn't aware of TEST_FAIL(), good suggestion!

Ryan-Everett-arm · 2024-03-14T17:37:38Z

In its current form this PR does not handle the potential INSUFFICIENT_MEMORY failure which can occur in exercise key when lots of threads attempt to load into a key slot. It shouldn't be merged before this is changed.

Upon revisiting this PR I realize that I was mistaken here. My concern was of two (what I thought where) possible scenarios:

exercise_key_derivation_key calls key_derivation_output_key - It doesn't, it only ever calls key_derivation_output_bytes.
A scenario could arise within a set of calls to exercise_key(k) where multiple slots are reserved for the same persistent key. - The only key ever used in this situation is k, and our locking policy in psa_get_and_lock_key_slot means that there can never be a situation where two threads calling psa_get_and_lock_key_slot_X attempt to load the same persistent key into multiple slots concurrently.

To summarize: no extra resources are ever required when calling psa_exercise_key concurrently on the same key, so there is no risk of INSUFFICIENT_MEMORY occuring in any sensible test. I will add a comment to the definition of exercise_key which documents that this function cannot be called concurrently with more unique persistent keys than there are free key slots.

Signed-off-by: Ryan Everett <ryan.everett@arm.com>

paul-elliott-arm

LGTM

valeriosetti

Thanks for addressing my comments. LGTM :)

Ryan-Everett-arm added 9 commits March 12, 2024 17:01

Add key_destroyable parameter to check_key_attributes_sanity

f08a93f

This function is currently only used in the exercise_key smoke test. Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Add key_destroyable parameter to exercise_mac_key

7763550

If the key has been destroyed (and the new parameter is 1) then we test that psa_mac_abort succeeds in this scenario. Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Add key_destroyable parameter to psa_exercise_cipher_key

70691f3

If the key has been destroyed (and the new parameter is 1), we test that psa_cipher_abort succeeds in this scenario. Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Add key_destroyable parameter to exercise_aead_key

fbe703d

Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Add key_destroyable parameter to exercise_signature_key

6edd408

Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Add key_destroyable parameter to exercise_asymmetric_encryption_key

d48fc10

Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Add key_destroyable parameter to key derivation smoke tests

c1cc668

All current usages have this parameter set to 0 (in this case the behaviour of the test is unchanged) Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Add key_destroyable parameter to raw key agreement smoke tests

8163028

All current usages have this parameter set to 0 (meaning the behaviour of these tests hasn't changed). We also now return the actual error code, not GENERIC_ERROR Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Ryan-Everett-arm self-assigned this Mar 12, 2024

Ryan-Everett-arm mentioned this pull request Mar 12, 2024

Fix threading bug when multiple destroy_key calls run on the same key #8912

Merged

3 tasks

paul-elliott-arm self-requested a review March 13, 2024 12:47

Ryan-Everett-arm added 4 commits March 13, 2024 14:31

Add key_destroyable parameter to key export smoke tests

fbf815d

These are only called from mbedtls_test_psa_exercise_key Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Add test function for concurrently using the same persistent key

5061999

The thread functions can also be used in future tests for other key types and other test scenarios Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Add test cases for concurrently_use_same_persistent_key

f111f35

There is a 1-1 correlation between these test cases and the test cases for import_and_exercise_key. Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Ryan-Everett-arm force-pushed the threading-same-key-tests branch from d4d5808 to f111f35 Compare March 13, 2024 14:34

Ryan-Everett-arm added the needs-work label Mar 13, 2024

valeriosetti self-requested a review March 14, 2024 10:54

valeriosetti reviewed Mar 14, 2024

View reviewed changes

Ryan-Everett-arm added 2 commits March 14, 2024 17:49

Fix typo in thread_import_key

6c48870

Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Use TEST_FAIL in threaded tests

3de040f

Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Ryan-Everett-arm added 2 commits March 14, 2024 17:50

Add missing PSA_ASSERT in mbedtls_test_psa_raw_key_agreement_with_self

6de38ac

Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Document unsupported concurrency scenario in psa_exercise_key

e1b50f3

Signed-off-by: Ryan Everett <ryan.everett@arm.com>

Ryan-Everett-arm requested a review from valeriosetti March 14, 2024 17:56

Ryan-Everett-arm removed the needs-work label Mar 14, 2024

paul-elliott-arm approved these changes Mar 15, 2024

View reviewed changes

Ryan-Everett-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Mar 15, 2024

valeriosetti approved these changes Mar 15, 2024

View reviewed changes

paul-elliott-arm added this pull request to the merge queue Mar 15, 2024

Merged via the queue into Mbed-TLS:development with commit 44ccc87 Mar 15, 2024
6 checks passed

paul-elliott-arm mentioned this pull request Mar 18, 2024

Add multi-threaded unit tests for PSA Crypto #8420

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add testing for concurrently loading/using/destroying the same key #8924

Add testing for concurrently loading/using/destroying the same key #8924

Ryan-Everett-arm commented Mar 12, 2024 •

edited

Loading

Ryan-Everett-arm commented Mar 13, 2024

valeriosetti left a comment

valeriosetti Mar 14, 2024

valeriosetti Mar 14, 2024

valeriosetti Mar 14, 2024

Ryan-Everett-arm Mar 14, 2024

Ryan-Everett-arm commented Mar 14, 2024

paul-elliott-arm left a comment

valeriosetti left a comment

Add testing for concurrently loading/using/destroying the same key #8924

Add testing for concurrently loading/using/destroying the same key #8924

Conversation

Ryan-Everett-arm commented Mar 12, 2024 • edited Loading

Description

PR checklist

Ryan-Everett-arm commented Mar 13, 2024

valeriosetti left a comment

Choose a reason for hiding this comment

valeriosetti Mar 14, 2024

Choose a reason for hiding this comment

valeriosetti Mar 14, 2024

Choose a reason for hiding this comment

valeriosetti Mar 14, 2024

Choose a reason for hiding this comment

Ryan-Everett-arm Mar 14, 2024

Choose a reason for hiding this comment

Ryan-Everett-arm commented Mar 14, 2024

paul-elliott-arm left a comment

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

Ryan-Everett-arm commented Mar 12, 2024 •

edited

Loading