From b872deded7f63a5eb0872e3261653815735e0b4b Mon Sep 17 00:00:00 2001 From: Anand Krishnamoorthi Date: Tue, 26 Oct 2021 08:51:42 -0700 Subject: [PATCH] Set MUSL libc to multi-threaded mode Fix bug where writing to stdout, stderr from multiple threads could cause memory corruption/crash. MUSL, by default, sets locks of stdout, stderr to -1 indicating that locking is not needed. libc.threaded is also set to 1. When reads and writes are performed from the standard streams, no locking is performed since there is only the main thread. The first time a program creates another thread, the pthread_create function sets the locks of stdout, stderr etc to 0 to indicate that locking ought to be done since the program now has more than one thread. Similarly, libc.threaded is also set to 1. For enclaves, threads are allocated before-hand via the enclave configuration. Each TCS corresponds to an enclave thread. These threads are not created via pthread_creat; they are pre-allocated at enclave creation time. Since no call to pthread_create is made, MUSL continues to think that there is only one thread. This results in memory corruption. The fix is to set state variables in MUSL during enclave initialization so that MUSL operates in multi-threaded mode. Signed-off-by: Anand Krishnamoorthi --- enclave/core/sgx/calls.c | 15 ++++++++++ enclave/link.c | 2 ++ include/openenclave/internal/libc/init.h | 19 +++++++++++++ libc/CMakeLists.txt | 11 ++++++-- libc/optee/init.c | 17 ++++++++++++ libc/sgx/init.c | 35 ++++++++++++++++++++++++ tests/config_id/config_id.edl | 1 + tests/libc/enc/enc.c | 5 ++++ 8 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 include/openenclave/internal/libc/init.h create mode 100644 libc/optee/init.c create mode 100644 libc/sgx/init.c diff --git a/enclave/core/sgx/calls.c b/enclave/core/sgx/calls.c index 17e5380730..6f8ee30f1b 100644 --- a/enclave/core/sgx/calls.c +++ b/enclave/core/sgx/calls.c @@ -135,6 +135,18 @@ uint8_t __oe_initialized = 0; **============================================================================== */ +/* +**============================================================================== +** oe_libc_initialize() +** +** Weak implementation of libc initialization function. +** +**============================================================================== +*/ +OE_WEAK void oe_libc_initialize(void) +{ +} + /* **============================================================================== ** @@ -175,6 +187,9 @@ static oe_result_t _handle_init_enclave(uint64_t arg_in) * Depends on TD and sgx_create_report, so can't happen earlier */ OE_CHECK(oe_set_is_xsave_supported()); + /* Initialize libc */ + oe_libc_initialize(); + /* Initialize the OE crypto library. */ oe_crypto_initialize(); diff --git a/enclave/link.c b/enclave/link.c index 01fd6b3368..932e4139e1 100644 --- a/enclave/link.c +++ b/enclave/link.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include "core_t.h" @@ -34,6 +35,7 @@ const void* oe_link_enclave(void) oe_allocator_malloc, oe_debug_malloc_tracking_start, oe_crypto_initialize, + oe_libc_initialize, SC_OSSL_ENGINE_Initialize, #if defined(OE_USE_DEBUG_MALLOC) oe_debug_malloc_check, diff --git a/include/openenclave/internal/libc/init.h b/include/openenclave/internal/libc/init.h new file mode 100644 index 0000000000..2c861ff41f --- /dev/null +++ b/include/openenclave/internal/libc/init.h @@ -0,0 +1,19 @@ +// Copyright (c) Open Enclave SDK contributors. +// Licensed under the MIT License. + +#ifndef _OE_INTERNAL_LIBC_INIT_H +#define _OE_INTERNAL_LIBC_INIT_H + +#include + +OE_EXTERNC_BEGIN + +/* Callback for initializing libc */ +void oe_libc_initialize(void); + +/* Test utility to check whether libc has been initialized */ +bool oe_test_libc_is_initialized(void); + +OE_EXTERNC_END + +#endif /* _OE_INTERNAL_LIBC_INIT_H */ diff --git a/libc/CMakeLists.txt b/libc/CMakeLists.txt index 932eea32a0..b70d4b45ff 100644 --- a/libc/CMakeLists.txt +++ b/libc/CMakeLists.txt @@ -47,8 +47,14 @@ endif () set(PLATFORM_SRC "") if (OE_SGX) - list(APPEND PLATFORM_SRC sgx/arc4random.c ${MUSLSRC}/math/x86_64/sqrt.c - ${MUSLSRC}/math/x86_64/sqrtl.c ${MUSLSRC}/math/x86_64/sqrtf.c) + list( + APPEND + PLATFORM_SRC + sgx/arc4random.c + ${MUSLSRC}/math/x86_64/sqrt.c + ${MUSLSRC}/math/x86_64/sqrtl.c + ${MUSLSRC}/math/x86_64/sqrtf.c + sgx/init.c) else () list( APPEND @@ -59,6 +65,7 @@ else () ${MUSLSRC}/string/memset.c optee/abort.c optee/arc4random.c + optee/init.c optee/trace.c) endif () diff --git a/libc/optee/init.c b/libc/optee/init.c new file mode 100644 index 0000000000..289a05648c --- /dev/null +++ b/libc/optee/init.c @@ -0,0 +1,17 @@ +// Copyright (c) Open Enclave SDK contributors. +// Licensed under the MIT License. + +#include +#include +#include "libc.h" +#include "stdio_impl.h" + +void oe_libc_initialize(void) +{ + // No multi-threaded initialization needed for OP-TEE. +} + +bool oe_test_libc_is_initialized(void) +{ + return true; +} diff --git a/libc/sgx/init.c b/libc/sgx/init.c new file mode 100644 index 0000000000..419ac23ac0 --- /dev/null +++ b/libc/sgx/init.c @@ -0,0 +1,35 @@ +// Copyright (c) Open Enclave SDK contributors. +// Licensed under the MIT License. + +#include +#include +#include "libc.h" +#include "stdio_impl.h" + +void oe_libc_initialize(void) +{ + /* In MUSL, locks are initialized for standard streams the first time a + thread is created, in pthread_create.c. + Since OE has pre-allocated number of thread, pthread_create is too late + to mark libc as multi threaded. Therefore, libc initialization is done + here instead. This also allows oecore to control exactly when libc is + initialized. + */ + libc.threaded = 1; + // MUSL also maintains libc.threads_minus_1 variable to keep track of + // number of running threads. That variable is used to decide whether + // locks are needed. OE, instead, always locks since it is not possible + // to easily manage that variable from within oecore. + libc.need_locks = 1; + stdin->lock = 0; + stdout->lock = 0; + stderr->lock = 0; +} + +bool oe_test_libc_is_initialized(void) +{ + return (libc.threaded == 1) && (libc.need_locks == 1) && + (__atomic_load_n(&stdin->lock, __ATOMIC_SEQ_CST) >= 0) && + (__atomic_load_n(&stdout->lock, __ATOMIC_SEQ_CST) >= 0) && + (__atomic_load_n(&stderr->lock, __ATOMIC_SEQ_CST) >= 0); +} diff --git a/tests/config_id/config_id.edl b/tests/config_id/config_id.edl index ef70f20103..3261dc6a46 100644 --- a/tests/config_id/config_id.edl +++ b/tests/config_id/config_id.edl @@ -4,6 +4,7 @@ enclave { from "openenclave/edl/logging.edl" import *; from "openenclave/edl/sgx/platform.edl" import *; + from "openenclave/edl/fcntl.edl" import *; trusted { public oe_result_t enclave_test_config_id_non_kss(); public oe_result_t enclave_test_config_id(); diff --git a/tests/libc/enc/enc.c b/tests/libc/enc/enc.c index 1070b44ffc..4e8ea39538 100644 --- a/tests/libc/enc/enc.c +++ b/tests/libc/enc/enc.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -148,6 +149,8 @@ int run_test(const char* test_name) device_init(); libc_test_function_t test = get_test_case(test_name); + OE_TEST(oe_test_libc_is_initialized()); + if (test) { return run_test_helper(test_name, test); @@ -163,6 +166,8 @@ int run_all_tests() device_init(); + OE_TEST(oe_test_libc_is_initialized()); + ret = 0; for (int i = 0; i < sizeof(libc_tests) / sizeof(libc_test_entry_t); i++) {