From 2c789788c08e772edabf9765d7113f9c654b30ef Mon Sep 17 00:00:00 2001 From: Fabio Alessandrelli Date: Thu, 8 May 2025 10:43:00 +0200 Subject: [PATCH] mbedTLS: Fix concurrency issues with TLS When we first integrated mbedTLS, we decided not to enable MBEDTLS_THREADING_C (which adds mutex locking to calls modifying the state), and instead to simply create separate contexts ("states") for each connection. This worked fine until recently. Sadly, mbedTLS 3 added a global state for the new PSA crypto functionalities (which are required to support TLSv1.3). This results in TLSv1.3 connections to access and modify the global state concurrently when running in threads. This commit enables MBEDTLS_THREADING_C, and MBEDTLS_THREADING_C_ALT to provide a generic Godot implementation using the engine Mutex class. --- core/io/resource_uid.cpp | 13 ++++-- modules/mbedtls/register_types.cpp | 45 ++++++++++++++++++- thirdparty/README.md | 4 +- .../include/godot_module_mbedtls_config.h | 7 +++ thirdparty/mbedtls/include/threading_alt.h | 3 ++ 5 files changed, 65 insertions(+), 7 deletions(-) create mode 100644 thirdparty/mbedtls/include/threading_alt.h diff --git a/core/io/resource_uid.cpp b/core/io/resource_uid.cpp index ef997f7458c..7dbf52d4c9c 100644 --- a/core/io/resource_uid.cpp +++ b/core/io/resource_uid.cpp @@ -109,6 +109,13 @@ ResourceUID::ID ResourceUID::text_to_id(const String &p_text) const { } ResourceUID::ID ResourceUID::create_id() { + // mbedTLS may not be fully initialized when the ResourceUID is created, so we + // need to lazily instantiate the random number generator. + if (crypto == nullptr) { + crypto = memnew(CryptoCore::RandomGenerator); + ((CryptoCore::RandomGenerator *)crypto)->init(); + } + while (true) { ID id = INVALID_ID; MutexLock lock(mutex); @@ -363,9 +370,9 @@ ResourceUID *ResourceUID::singleton = nullptr; ResourceUID::ResourceUID() { ERR_FAIL_COND(singleton != nullptr); singleton = this; - crypto = memnew(CryptoCore::RandomGenerator); - ((CryptoCore::RandomGenerator *)crypto)->init(); } ResourceUID::~ResourceUID() { - memdelete((CryptoCore::RandomGenerator *)crypto); + if (crypto != nullptr) { + memdelete((CryptoCore::RandomGenerator *)crypto); + } } diff --git a/modules/mbedtls/register_types.cpp b/modules/mbedtls/register_types.cpp index a38e833a7b5..aff1fad3ce4 100644 --- a/modules/mbedtls/register_types.cpp +++ b/modules/mbedtls/register_types.cpp @@ -45,15 +45,52 @@ #include "tests/test_crypto_mbedtls.h" #endif +#ifdef GODOT_MBEDTLS_THREADING_ALT +extern "C" { +void godot_mbedtls_mutex_init(mbedtls_threading_mutex_t *p_mutex) { + ERR_FAIL_NULL(p_mutex); + p_mutex->mutex = memnew(Mutex); +} + +void godot_mbedtls_mutex_free(mbedtls_threading_mutex_t *p_mutex) { + ERR_FAIL_NULL(p_mutex); + ERR_FAIL_NULL(p_mutex->mutex); + memdelete((Mutex *)p_mutex->mutex); +} + +int godot_mbedtls_mutex_lock(mbedtls_threading_mutex_t *p_mutex) { + ERR_FAIL_NULL_V(p_mutex, MBEDTLS_ERR_THREADING_BAD_INPUT_DATA); + ERR_FAIL_NULL_V(p_mutex->mutex, MBEDTLS_ERR_THREADING_BAD_INPUT_DATA); + ((Mutex *)p_mutex->mutex)->lock(); + return 0; +} + +int godot_mbedtls_mutex_unlock(mbedtls_threading_mutex_t *p_mutex) { + ERR_FAIL_NULL_V(p_mutex, MBEDTLS_ERR_THREADING_BAD_INPUT_DATA); + ERR_FAIL_NULL_V(p_mutex->mutex, MBEDTLS_ERR_THREADING_BAD_INPUT_DATA); + ((Mutex *)p_mutex->mutex)->unlock(); + return 0; +} +}; +#endif + static bool godot_mbedtls_initialized = false; void initialize_mbedtls_module(ModuleInitializationLevel p_level) { - if (p_level != MODULE_INITIALIZATION_LEVEL_SCENE) { + if (p_level != MODULE_INITIALIZATION_LEVEL_CORE) { return; } GLOBAL_DEF("network/tls/enable_tls_v1.3", true); +#ifdef GODOT_MBEDTLS_THREADING_ALT + mbedtls_threading_set_alt( + godot_mbedtls_mutex_init, + godot_mbedtls_mutex_free, + godot_mbedtls_mutex_lock, + godot_mbedtls_mutex_unlock); +#endif + #if MBEDTLS_VERSION_MAJOR >= 3 int status = psa_crypto_init(); ERR_FAIL_COND_MSG(status != PSA_SUCCESS, "Failed to initialize psa crypto. The mbedTLS modules will not work."); @@ -74,7 +111,7 @@ void initialize_mbedtls_module(ModuleInitializationLevel p_level) { } void uninitialize_mbedtls_module(ModuleInitializationLevel p_level) { - if (p_level != MODULE_INITIALIZATION_LEVEL_SCENE) { + if (p_level != MODULE_INITIALIZATION_LEVEL_CORE) { return; } @@ -90,4 +127,8 @@ void uninitialize_mbedtls_module(ModuleInitializationLevel p_level) { PacketPeerMbedDTLS::finalize_dtls(); StreamPeerMbedTLS::finalize_tls(); CryptoMbedTLS::finalize_crypto(); + +#ifdef GODOT_MBEDTLS_THREADING_ALT + mbedtls_threading_free_alt(); +#endif } diff --git a/thirdparty/README.md b/thirdparty/README.md index 8e2483a8c66..eda05b53345 100644 --- a/thirdparty/README.md +++ b/thirdparty/README.md @@ -634,8 +634,8 @@ File extracted from upstream release tarball: - The `LICENSE` file (edited to keep only the Apache 2.0 variant) - Added 2 files `godot_core_mbedtls_platform.c` and `godot_core_mbedtls_config.h` providing configuration for light bundling with core -- Added the file `godot_module_mbedtls_config.h` to customize the build - configuration when bundling the full library +- Added 2 files `godot_module_mbedtls_config.h` and `threading_alt.h` + to customize the build configuration when bundling the full library Patches: diff --git a/thirdparty/mbedtls/include/godot_module_mbedtls_config.h b/thirdparty/mbedtls/include/godot_module_mbedtls_config.h index 0228e0fc0a3..c25804979fd 100644 --- a/thirdparty/mbedtls/include/godot_module_mbedtls_config.h +++ b/thirdparty/mbedtls/include/godot_module_mbedtls_config.h @@ -49,6 +49,13 @@ #undef MBEDTLS_DES_C #undef MBEDTLS_DHM_C +#ifdef THREADS_ENABLED +// In mbedTLS 3, the PSA subsystem has an implicit shared context, MBEDTLS_THREADING_C is required to make it thread safe. +#define MBEDTLS_THREADING_C +#define MBEDTLS_THREADING_ALT +#define GODOT_MBEDTLS_THREADING_ALT +#endif + #if !(defined(__linux__) && defined(__aarch64__)) // ARMv8 hardware AES operations. Detection only possible on linux. // May technically be supported on some ARM32 arches but doesn't seem diff --git a/thirdparty/mbedtls/include/threading_alt.h b/thirdparty/mbedtls/include/threading_alt.h new file mode 100644 index 00000000000..dd3006b5305 --- /dev/null +++ b/thirdparty/mbedtls/include/threading_alt.h @@ -0,0 +1,3 @@ +typedef struct { + void *mutex; +} mbedtls_threading_mutex_t;