Make PSA global_data thread safe #8901

paul-elliott-arm · 2024-03-06T18:05:33Z

Description

Make the contents of PSA global_data thread safe. Resolves #8425. Resolves #7945

Notes:

It should now be safe to call psa_crypto_init() on multiple threads, either concurrently or sequentially.
Although some protection has now been done on mbedtls_psa_crypto_free() it is still not entirely thread safe, even if no operations are running at the time. This is annoying as I could have used this to improve testing, however we never claimed it would be thread safe.
The Macro PSA_DONE() is still definitely not thread safe, due to the checks it does.
In this PR we only attempt to make access to global_data.rng_state and global_data.rng thread safe. We are not attempting to make the random number generator thread safe, as that was (mostly) done elsewhere. I have seen an existing issue with the RNG whereby the mutex gets deleted just prior to another thread using it, so again, this is not something that can be tested alongside shutdown (but this unfortunately means we are not testing access to global_data.rng.drbg particularly thoroughly).
psa_crypto_init() had to be split into blocks due to issues with deadlock when the rng startup would end up calling into md.c which would call psa_can_do_hash(), which would attempt to lock the same mutex. The only solution to this as far as I could see was to use multiple mutexes (re-entrant mutexes are going to make cross platform support difficult, one time initialisation did not entirely solve the issue), which necessitated the split. I attempted to do the split as per suggestions in Define the PSA subsystem initialization interface in Mbed TLS #6007 - Note I did not yet make this a public interface, so improvements can be made prior to making this public (work which is not really in the scope of this PR)
The testing in this PR is slightly tortured, admittedly. Any suggestions for testing improvements would be welcomed, but be aware that we are quite restricted in what we can or can not do (see above)

PR checklist

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

changelog ~~provided, or~~ not required (part of the multithreading EPIC)
backport ~~done, or~~ not required (new features)
tests provided ~~, or not required~~

Ryan-Everett-arm

There are a few changes needed, and I have some suggestions and questions

library/psa_crypto.c

Ryan-Everett-arm · 2024-03-07T17:48:28Z

include/mbedtls/threading.h

+ * A mutex used to make the PSA global_data struct members thread safe.
+ *
+ * This mutex must be held when any read or write to a any of the PSA
+ * global_data structure members. */


There are actually two very different psa_global_data_t structs, each relating to a variable called global_data. One is defined in psa_crypto.c, the other in psa_crypto_slot_management.c. This mutex only covers the former struct. To alleviate confusion I think it would help to say "used to make the members of the global_data struct defined in psa_crypto.c thread safe"

Ryan-Everett-arm · 2024-03-07T18:14:56Z

include/mbedtls/threading.h

+ *
+ * This mutex must be held when any read or write to a any of the PSA
+ * global_data structure members. */
+extern mbedtls_threading_mutex_t mbedtls_threading_psa_globaldata_mutex;


Further to my above comment. global_data.key_slots_initialized is not protected by this PR in its current state. This variable is written to in psa_crypto_init and mbedtls_psa_crypo_free under the new mutex. It is read in psa_reserve_free_key_slot and psa_get_and_lock_key_slot.

We can either protect it with your new mutex by making a getter function, or we can protect it with the key slot mutex (key_slot_mutex is currently held in the call to reserve_free_key_slot but not get_and_lock_key_slot). A case could be made for either option:

Code wise, it is slightly cleaner to protect key_slots_initialized with the global data mutex.

Documentation wise, protecting it with the key slot mutex is cleaner as then each mutex protects the entirety of one struct.

Ryan-Everett-arm · 2024-03-07T18:40:46Z

tests/suites/test_suite_psa_crypto_init.function

+    if (init_context->do_shutdown) {
+        /* Note that shutting down PSA crypto is *not* guaranteed to be thread
+         * safe if *any* operations are currently in progress. I am merely doing
+         * this here to test the safety of global flags that are written in
+         * psa_crypto_init() / mbedtls_psa_crypto_free(). */
+        mbedtls_psa_crypto_free();
+    }
+
+    /* Given that another thread may have called mbedtls_psa_crypto_free() by
+     * this point on another thread, we can only guarantee the return codes
+     * from the following tests if we are not also testing shutdown. The tests
+     * are still worth doing though, so that TSAN can catch potential issues. */
+
+    /* Test getting if drivers are initialised. */
+    int can_do = psa_can_do_hash(PSA_ALG_NONE);
+
+    if (!init_context->do_shutdown) {
+        TEST_ASSERT(can_do == 1);
+    }
+
+#if !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG)
+
+    /* Test getting global_data.rng_state. */
+    status = mbedtls_psa_crypto_configure_entropy_sources(NULL, NULL);
+
+    if (!init_context->do_shutdown) {
+        /* Bad state due to entropy sources already being setup in
+         * psa_crypto_init() */
+        TEST_EQUAL(status, PSA_ERROR_BAD_STATE);
+    }
+#endif /* !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG) */
+
+    /* Test using the PSA RNG if not testing shutdown (RNG is not safe to use if
+     * we shut down PSA whilst using it). */
+    if (!init_context->do_shutdown) {
+        status = psa_generate_random(random, sizeof(random));
+
+        TEST_EQUAL(status, PSA_SUCCESS);
+    }


Suggestion: We can shorten this by explicitely splitting the cases, but if you think the current style is cleaner then keep it as is

if (init_context->do_shutdown) { /* Note that shutting down PSA crypto is *not* guaranteed to be thread * safe if *any* operations are currently in progress. I am merely doing * this here to test the safety of global flags that are written in * psa_crypto_init() / mbedtls_psa_crypto_free(). */ mbedtls_psa_crypto_free(); goto exit; } else { /* Test getting if drivers are initialized. * We cannot test this if do_shutdown is true, as another thread * may have already called mbedtls_psa_crypto_free * which uninitializes the drivers. TSAN can catch potential issues * here, so we should still do this test. */ TEST_ASSERT(psa_can_do_hash(PSA_ALG_NONE) == 1); #if !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG) .... #endif .... }

This has kinda changed in the latest iteration. I was, however deliberately running the flag test checks at the same time as doing startup / shutdown of PSA.

Ryan-Everett-arm · 2024-03-07T18:42:33Z

tests/suites/test_suite_psa_crypto_init.data

@@ -10,6 +10,9 @@ deinit_without_init:0
 PSA deinit twice
 deinit_without_init:1

+PSA threaded init checks
+psa_threaded_init:100


Do we need a depends_on:MBEDTLS_THREADING_PTHREAD here?

No, this is done on the test function itself, rather than the data. The test cannot run without threading, regardless of what the data is set to.

Ryan-Everett-arm · 2024-03-07T18:51:16Z

library/psa_crypto.c

+
+    return initialized;
+}
+
 #if !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG)
 mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state =
    &global_data.rng.drbg;


Question: is it okay that the accesses to MBEDTLS_PSA_RANDOM_STATE/mbedtls_psa_random_state are not done under this mutex? I can see a point in the PR description about global_data.rng.drbg, but I'm a bit confused about the state of thread-safety of RNG.

I.e., is it possible that if multiple threads are each doing ecjpake operations one may have non thread-safe rng behaviour at the blinding stage?

Unfortunately, given the way that the state is passed around as a pointer, I don't see how I could protect all the uses. There is already a mutex within mbedtls_ctr_drbg_context which should protect a lot of these uses, but otherwise its going to be extremely difficult to add more protection.

When the PSA RNG is implemented on top of CTR_DRBG and HMAC_DRBG, i.e. when MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG is disabled, the PSA part of the code doesn't need to take any precautions when calling get_random, because the CTR_DRBG and HMAC_DRBG modules have their own mutex guarding the RNG state. The PSA code must not call init/seed/free concurrently with those, but that's only ever done as part of psa_crypto_init and free which needs locking for other reasons anyway.

The external RNG case is its own task, which I think isn't a 3.6.0 objective?

No, its not part of this work, thanks for clarifying that.

tom-cosgrove-arm · 2024-03-12T15:02:00Z

Note: has conflicts

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Unfortunately this requires holding the mutex for the entire psa_crypto_init() function, as calling psa_crypto_free() from another thread should block until init has ended, then run. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Reads and writes of rng_state in psa_crypto_init() and psa_crypto_free() were already covered by mutex. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Writes to this in psa_crypto_init() were again already covered. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Internal only for now, but can be made external with some more work. Break up psa_crypto_init into chunks to prevent deadlocks when initialising RNG, likewise break up mbedtls_crypto_free() to stop having to hold more than one mutex at a time. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Use the global data mutex, as the key slot mutex has to be held in some of the functions where we are testing the flag, and we already hold the global data mutex when calling the functions where the flag is set. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

paul-elliott-arm · 2024-03-13T14:25:16Z

Internal CI failures appear to be spurious (I can't even see what its complaining about, apparently the build failed, but there is no evidence of that)

gilles-peskine-arm · 2024-03-13T14:33:58Z

The internal CI is still running, but https://jenkins-mbedtls.oss.arm.com/blue/organizations/jenkins/mbed-tls-pr-head/detail/PR-8901-head/8/pipeline/1128 (Windows-2013-Debug-Win32-cmake-retarget) failed due to a build error

         CUSTOMBUILD : CMake error : Cannot restore timestamp C:/builds/workspace/mbed-tls-pr-head_PR-8901-head/worktrees/tmp3qwimxf9/cmake_solution/tests/CMakeFiles/generate.stamp [C:\builds\workspace\mbed-tls-pr-head_PR-8901-head\worktrees\tmp3qwimxf9\cmake_solution\tests\test_suite_psa_crypto_low_hash.generated.vcxproj]

I have no idea what it means, but it's clearly not related to this PR, and I doubt that it's reproducible.

paul-elliott-arm · 2024-03-15T14:04:33Z

should psa_generate_random() be taking the rngdata_mutex (or another one) as it accesses global_data.rng?

The internals of the rng should be guarded by their own mutex in the mbedtls_ctr_drbg_context - technically we don't need to hold the global mutex to seed either, but we need to do the RNG init atomically, so we do.

paul-elliott-arm · 2024-03-15T14:05:00Z

All nits addressed, only minor changes.

yanesca

LGTM

tom-cosgrove-arm

LGTM

tom-cosgrove-arm · 2024-03-15T15:58:35Z

library/psa_crypto.c

+#define PSA_CRYPTO_DEF_SUBSYSTEM_TRANSACTION_INITIALIZED        0x04
+
+const uint8_t PSA_CRYPTO_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED =
+    PSA_CRYPTO_DEF_SUBSYSTEM_DRIVER_WRAPPERS_INITIALIZED;


We've now got both #define and const uint8_t, whereas we originally just had only the #defines. IMO it's not worth the extra set of identifiers for the small amount of typing we get, and I'd rather we just rolled back some of a5189c49fbad6e5700e7a0807750a50ff21898b7

Ok, do you mind if I force push?

Catch potential invalid calls to init. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

paul-elliott-arm · 2024-03-15T16:27:44Z

Removed commit to change over to const uint8_ts and the fix for this, added back the comment.

tom-cosgrove-arm

LGTM - thanks

yanesca

LGTM

Stale review

paul-elliott-arm self-assigned this Mar 6, 2024

paul-elliott-arm force-pushed the make_psa_global_data_safe branch 2 times, most recently from b5049d6 to 055fad5 Compare March 7, 2024 14:59

Ryan-Everett-arm self-requested a review March 7, 2024 15:19

paul-elliott-arm force-pushed the make_psa_global_data_safe branch from d4c1a31 to 8997508 Compare March 7, 2024 16:15

Ryan-Everett-arm previously requested changes Mar 7, 2024

View reviewed changes

paul-elliott-arm added 5 commits March 12, 2024 15:36

Add new global mutex for PSA global_data

077fd87

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Add new mutex for PSA global rng data

b8e38e0

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Protect PSA global initialized flag with mutex.

600472b

Unfortunately this requires holding the mutex for the entire psa_crypto_init() function, as calling psa_crypto_free() from another thread should block until init has ended, then run. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Protect PSA global rng data with mutex.

8e15153

Reads and writes of rng_state in psa_crypto_init() and psa_crypto_free() were already covered by mutex. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Protect PSA drivers_initialized with mutex

3581652

Writes to this in psa_crypto_init() were again already covered. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

paul-elliott-arm force-pushed the make_psa_global_data_safe branch 2 times, most recently from f8de5a1 to 9a73a7f Compare March 12, 2024 18:04

paul-elliott-arm added 3 commits March 13, 2024 12:39

Add PSA threaded init tests

0493ab5

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

paul-elliott-arm force-pushed the make_psa_global_data_safe branch from 9a73a7f to 0493ab5 Compare March 13, 2024 12:39

paul-elliott-arm dismissed yanesca’s stale review via 3f6b1cc March 15, 2024 13:59

paul-elliott-arm requested review from tom-cosgrove-arm and yanesca March 15, 2024 14:05

paul-elliott-arm removed the approved Design and code approved - may be waiting for CI or backports label Mar 15, 2024

yanesca previously approved these changes Mar 15, 2024

View reviewed changes

tom-cosgrove-arm previously approved these changes Mar 15, 2024

View reviewed changes

tom-cosgrove-arm added the approved Design and code approved - may be waiting for CI or backports label Mar 15, 2024

paul-elliott-arm dismissed stale reviews from tom-cosgrove-arm and yanesca via 0bebb69 March 15, 2024 15:52

tom-cosgrove-arm reviewed Mar 15, 2024

View reviewed changes

paul-elliott-arm added 3 commits March 15, 2024 16:06

Start subsystem IDs at 1 instead of 0

0db6a90

Catch potential invalid calls to init. Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Add comments about RNG mutex requirements

d35dce6

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

Add explanatory comment for init flags

b24e36d

Signed-off-by: Paul Elliott <paul.elliott@arm.com>

paul-elliott-arm force-pushed the make_psa_global_data_safe branch from 0bebb69 to b24e36d Compare March 15, 2024 16:26

paul-elliott-arm requested review from tom-cosgrove-arm and yanesca March 15, 2024 16:27

tom-cosgrove-arm approved these changes Mar 15, 2024

View reviewed changes

yanesca approved these changes Mar 15, 2024

View reviewed changes

paul-elliott-arm enabled auto-merge March 15, 2024 19:49

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

Merged via the queue into Mbed-TLS:development with commit 78064ac Mar 15, 2024
5 of 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

Make PSA global_data thread safe #8901

Make PSA global_data thread safe #8901

paul-elliott-arm commented Mar 6, 2024 •

edited

Loading

Ryan-Everett-arm left a comment

Ryan-Everett-arm Mar 7, 2024

Ryan-Everett-arm Mar 7, 2024

Ryan-Everett-arm Mar 7, 2024

paul-elliott-arm Mar 12, 2024

Ryan-Everett-arm Mar 7, 2024

paul-elliott-arm Mar 12, 2024

Ryan-Everett-arm Mar 7, 2024

paul-elliott-arm Mar 12, 2024

gilles-peskine-arm Mar 12, 2024 •

edited

Loading

paul-elliott-arm Mar 12, 2024

tom-cosgrove-arm commented Mar 12, 2024

paul-elliott-arm commented Mar 13, 2024

gilles-peskine-arm commented Mar 13, 2024

paul-elliott-arm commented Mar 15, 2024

paul-elliott-arm commented Mar 15, 2024

yanesca left a comment

tom-cosgrove-arm left a comment

tom-cosgrove-arm Mar 15, 2024

paul-elliott-arm Mar 15, 2024

paul-elliott-arm commented Mar 15, 2024

tom-cosgrove-arm left a comment

yanesca left a comment

Make PSA global_data thread safe #8901

Make PSA global_data thread safe #8901

Conversation

paul-elliott-arm commented Mar 6, 2024 • edited Loading

Description

Notes:

PR checklist

Ryan-Everett-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Mar 12, 2024

paul-elliott-arm commented Mar 13, 2024

gilles-peskine-arm commented Mar 13, 2024

paul-elliott-arm commented Mar 15, 2024

paul-elliott-arm commented Mar 15, 2024

yanesca left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-elliott-arm commented Mar 15, 2024

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

yanesca left a comment

Choose a reason for hiding this comment

paul-elliott-arm commented Mar 6, 2024 •

edited

Loading

gilles-peskine-arm Mar 12, 2024 •

edited

Loading