Skip to content

Commit

Permalink
Set MUSL libc to multi-threaded mode
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
anakrish committed Oct 27, 2021
1 parent f34f4b0 commit b872ded
Show file tree
Hide file tree
Showing 8 changed files with 103 additions and 2 deletions.
15 changes: 15 additions & 0 deletions enclave/core/sgx/calls.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
}

/*
**==============================================================================
**
Expand Down Expand Up @@ -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();

Expand Down
2 changes: 2 additions & 0 deletions enclave/link.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <openenclave/debugmalloc.h>
#include <openenclave/enclave.h>
#include <openenclave/internal/crypto/init.h>
#include <openenclave/internal/libc/init.h>
#include <openenclave/internal/malloc.h>
#include "core_t.h"

Expand Down Expand Up @@ -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,
Expand Down
19 changes: 19 additions & 0 deletions include/openenclave/internal/libc/init.h
Original file line number Diff line number Diff line change
@@ -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 <openenclave/internal/defs.h>

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 */
11 changes: 9 additions & 2 deletions libc/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -59,6 +65,7 @@ else ()
${MUSLSRC}/string/memset.c
optee/abort.c
optee/arc4random.c
optee/init.c
optee/trace.c)
endif ()

Expand Down
17 changes: 17 additions & 0 deletions libc/optee/init.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright (c) Open Enclave SDK contributors.
// Licensed under the MIT License.

#include <openenclave/enclave.h>
#include <openenclave/internal/libc/init.h>
#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;
}
35 changes: 35 additions & 0 deletions libc/sgx/init.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (c) Open Enclave SDK contributors.
// Licensed under the MIT License.

#include <openenclave/enclave.h>
#include <openenclave/internal/libc/init.h>
#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);
}
1 change: 1 addition & 0 deletions tests/config_id/config_id.edl
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
5 changes: 5 additions & 0 deletions tests/libc/enc/enc.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <libgen.h>
#include <openenclave/enclave.h>
#include <openenclave/internal/constants_x64.h>
#include <openenclave/internal/libc/init.h>
#include <openenclave/internal/tests.h>
#include <search.h>
#include <stdarg.h>
Expand Down Expand Up @@ -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);
Expand All @@ -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++)
{
Expand Down

0 comments on commit b872ded

Please sign in to comment.