Fix data races around g_enclave_state (#889)

Before this commit, the memory access within `do_init_enclave` setting
`g_enclave_state` to `ENCLAVE_INIT_DONE` was entirely unsynchronized.
This could cause the compiler to reorder this access, performing it
earlier than actually written. This would effectively unlock the memory
being initialized for use within other threads before the initialization
is finished.

The issue isn't entirely theoretical, as such an optimization could for
example allow to make one of the calls to `memset_s` a tailcall. The
only thing preventing this is the difficulty of proving that the memset
doesn't alias `g_enclave_state`.

One way to fix this would be to access `g_enclave_state` with C11
atomics of ordering `acq_rel` or stronger. However, the freestanding
environment of the SDK doesn't support C11 atomics. Thus we use the
existing assembly wrappers, which are sufficient as achieving `acq_rel`
semantics in x86 assembly doesn't require any special instructions.

To make it less likely that a similar flaw is reintroduced, we remove
the `extern` declaration of `g_enclave_state` from the header files.

Signed-off-by: Maja Kądziołka <[email protected]>

Author: meithecatte

common/inc/internal/global_data.h

@@ -78,7 +78,6 @@ typedef struct _global_data_t
 extern "C" {
 #endif
 extern SE_DECLSPEC_EXPORT global_data_t const volatile g_global_data;
-extern uint32_t g_enclave_state;
 extern sdk_version_t g_sdk_version;
 extern int EDMM_supported;
 extern uint8_t  __ImageBase;

sdk/trts/init_enclave.cpp

@@ -62,7 +62,10 @@ uint64_t g_enclave_size __attribute__((section(RELRO_SECTION_NAME))) = 0;
 
 const volatile global_data_t g_global_data __attribute__((section(".niprod"))) = {VERSION_UINT, 1, 2, 3, 4, 5, 6, 0, 0, 0,
    {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, {0, 0, 0, 0, 0, 0}, 0}, {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 0, 0, {{{0, 0, 0, 0, 0, 0, 0}}}, 0, 0, 0};
+
+// Make sure to access this with atomics or the {get,set}_enclave_state assembly wrappers.
 uint32_t g_enclave_state __attribute__((section(".nipd"))) = ENCLAVE_INIT_NOT_STARTED;
+
 uint32_t g_cpu_core_num __attribute__((section(RELRO_SECTION_NAME))) = 0;
 
 extern "C" {
@@ -267,7 +270,7 @@ sgx_status_t do_init_enclave(void *ms, void *tcs)
     }
 #endif
 
-    g_enclave_state = ENCLAVE_INIT_DONE;
+    set_enclave_state(ENCLAVE_INIT_DONE);
 
 #ifndef SE_SIM
     // EDMM initialization makes ocalls which requires ENCLAVE_INIT_DONE being set

sdk/trts/init_optimized_lib.cpp

@@ -36,6 +36,7 @@
 #include "sgx_trts.h"
 #include "sgx_attributes.h"
 #include "global_data.h"
+#include "trts_internal.h"
 
 extern "C" int sgx_init_string_lib(uint64_t cpu_feature_indicator);
 extern "C" sgx_status_t sgx_init_crypto_lib(uint64_t cpu_feature_indicator, uint32_t *cpuinfo_table);
@@ -94,7 +95,7 @@ static int set_global_feature_indicator(uint64_t feature_bit_array, uint64_t xfr
 
 extern "C" int init_optimized_libs(const uint64_t feature_bit_array, uint32_t *cpuinfo_table, uint64_t xfrm)
 {
-    if (g_enclave_state != ENCLAVE_INIT_IN_PROGRESS)
+    if (get_enclave_state() != ENCLAVE_INIT_IN_PROGRESS)
     {
         return -1;
     }

sdk/trts/trts_veh.cpp

@@ -351,7 +351,7 @@ extern "C" sgx_status_t trts_handle_exception(void *tcs)
     sp_u = ssa_gpr->REG(sp_u);
     if (!sgx_is_outside_enclave((void *)sp_u, sizeof(sp_u)))
     {
-        g_enclave_state = ENCLAVE_CRASHED;
+        set_enclave_state(ENCLAVE_CRASHED);
         return SGX_ERROR_STACK_OVERRUN;
     }
 
@@ -360,13 +360,13 @@ extern "C" sgx_status_t trts_handle_exception(void *tcs)
     sp = ssa_gpr->REG(sp);
     if (sp_u == sp)
     {
-        g_enclave_state = ENCLAVE_CRASHED;
+        set_enclave_state(ENCLAVE_CRASHED);
         return SGX_ERROR_STACK_OVERRUN;
     }
 
     if(!is_stack_addr((void*)sp, 0))  // check stack overrun only, alignment will be checked after exception handled
     {
-        g_enclave_state = ENCLAVE_CRASHED;
+        set_enclave_state(ENCLAVE_CRASHED);
         return SGX_ERROR_STACK_OVERRUN;
     }
 
@@ -383,7 +383,7 @@ extern "C" sgx_status_t trts_handle_exception(void *tcs)
     // check the decreased sp to make sure it is in the trusted stack range
     if(!is_stack_addr((void *)sp, size))
     {
-        g_enclave_state = ENCLAVE_CRASHED;
+        set_enclave_state(ENCLAVE_CRASHED);
         return SGX_ERROR_STACK_OVERRUN;
     }
 
@@ -393,7 +393,7 @@ extern "C" sgx_status_t trts_handle_exception(void *tcs)
     sp -= size;
     if(!is_stack_addr((void *)sp, size))
     {
-        g_enclave_state = ENCLAVE_CRASHED;
+        set_enclave_state(ENCLAVE_CRASHED);
         return SGX_ERROR_STACK_OVERRUN;
     }
 
@@ -416,7 +416,7 @@ extern "C" sgx_status_t trts_handle_exception(void *tcs)
         }
         else
         {
-            g_enclave_state = ENCLAVE_CRASHED;
+            set_enclave_state(ENCLAVE_CRASHED);
             return SGX_ERROR_STACK_OVERRUN;
         }
     }
@@ -486,6 +486,6 @@ extern "C" sgx_status_t trts_handle_exception(void *tcs)
     return SGX_SUCCESS;
 
 default_handler:
-    g_enclave_state = ENCLAVE_CRASHED;
+    set_enclave_state(ENCLAVE_CRASHED);
     return SGX_ERROR_ENCLAVE_CRASHED;
 }