From 68acf9d9d4708969e6c68ef7217253c8472d7aa2 Mon Sep 17 00:00:00 2001 From: Ming-Wei Shih Date: Mon, 15 Nov 2021 22:43:15 +0000 Subject: [PATCH] Clear the td states on ECALL return if the state is not RUNNING Signed-off-by: Ming-Wei Shih --- enclave/core/sgx/calls.c | 4 +- enclave/core/sgx/td.c | 62 +++++++++--- enclave/core/sgx/td_basic.c | 6 ++ include/openenclave/internal/sgx/td.h | 18 ++-- tests/VectorException/enc/enc.c | 9 +- tests/VectorException/enc/init.cpp | 6 +- tests/sgx/td_state/enc/enc.c | 95 +++++++++++++++++-- tests/sgx/td_state/host/host.c | 44 +++++++++ tests/sgx/td_state/td_state.edl | 5 + tests/sgx/thread_interrupt/enc/enc.c | 18 ++-- tests/stack_overflow_exception/enc/enc.c | 17 +++- tests/stack_overflow_exception/host/host.c | 5 - .../stack_overflow_exception.edl | 1 - 13 files changed, 236 insertions(+), 54 deletions(-) diff --git a/enclave/core/sgx/calls.c b/enclave/core/sgx/calls.c index ace58631a9..cf98c9e901 100644 --- a/enclave/core/sgx/calls.c +++ b/enclave/core/sgx/calls.c @@ -1223,7 +1223,9 @@ void oe_abort(void) { oe_sgx_td_t* td = oe_sgx_get_td(); - td->state = OE_TD_STATE_ABORTED; + /* only update the state if td is initialized */ + if (td) + td->state = OE_TD_STATE_ABORTED; // Once it starts to crash, the state can only transit forward, not // backward. diff --git a/enclave/core/sgx/td.c b/enclave/core/sgx/td.c index 5f66e1c452..1f45171ad3 100644 --- a/enclave/core/sgx/td.c +++ b/enclave/core/sgx/td.c @@ -172,16 +172,55 @@ oe_sgx_td_t* oe_sgx_get_td() /* **============================================================================== ** -** oe_sgx_set_td_exception_handler_stack() +** oe_sgx_clear_td_states() +** +** Internal API that allows an enclave to clear the td states. +** +**============================================================================== +*/ + +void oe_sgx_td_clear_states(oe_sgx_td_t* td) +{ + /* Mask host signals by default */ + oe_sgx_td_mask_host_signal(td); + + /* Clear exception-related information */ + td->exception_code = 0; + td->exception_flags = 0; + td->exception_address = 0; + td->faulting_address = 0; + td->error_code = 0; + td->last_ssa_rsp = 0; + td->last_ssa_rbp = 0; + + /* Clear states related host signal handling */ + td->exception_nesting_level = 0; + td->is_handling_host_signal = 0; + td->host_signal_bitmask = 0; + td->host_signal = 0; + + /* Clear the states of the state machine */ + td->previous_state = OE_TD_STATE_NULL; + td->state = OE_TD_STATE_RUNNING; // the default state during the runtime +} + +/* +**============================================================================== +** +** oe_sgx_td_set_exception_handler_stack() ** ** Internal API that allows an enclave to setup stack area for ** exception handlers to use. ** **============================================================================== */ -bool oe_sgx_set_td_exception_handler_stack(void* stack, uint64_t size) +bool oe_sgx_td_set_exception_handler_stack( + oe_sgx_td_t* td, + void* stack, + uint64_t size) { - oe_sgx_td_t* td = oe_sgx_get_td(); + if (!td) + return false; /* ensure stack + size is 16-byte aligned */ if (((uint64_t)stack + size) % 16) @@ -204,21 +243,22 @@ bool oe_sgx_set_td_exception_handler_stack(void* stack, uint64_t size) **============================================================================== */ -OE_INLINE void _set_td_host_signal_unmasked(uint64_t value) +OE_INLINE void _set_td_host_signal_unmasked(oe_sgx_td_t* td, uint64_t value) { - oe_sgx_td_t* td = oe_sgx_get_td(); + if (!td) + return; td->host_signal_unmasked = value; } -void oe_sgx_td_mask_host_signal() +void oe_sgx_td_mask_host_signal(oe_sgx_td_t* td) { - _set_td_host_signal_unmasked(0); + _set_td_host_signal_unmasked(td, 0); } -void oe_sgx_td_unmask_host_signal() +void oe_sgx_td_unmask_host_signal(oe_sgx_td_t* td) { - _set_td_host_signal_unmasked(1); + _set_td_host_signal_unmasked(td, 1); } /* @@ -257,12 +297,12 @@ OE_INLINE bool _set_td_host_signal_bitmask( return true; } -bool oe_sgx_register_td_host_signal(oe_sgx_td_t* td, int signal_number) +bool oe_sgx_td_register_host_signal(oe_sgx_td_t* td, int signal_number) { return _set_td_host_signal_bitmask(td, signal_number, 1 /* set */); } -bool oe_sgx_unregister_td_host_signal(oe_sgx_td_t* td, int signal_number) +bool oe_sgx_td_unregister_host_signal(oe_sgx_td_t* td, int signal_number) { return _set_td_host_signal_bitmask(td, signal_number, 0 /* clear */); } diff --git a/enclave/core/sgx/td_basic.c b/enclave/core/sgx/td_basic.c index dde624f340..501421a418 100644 --- a/enclave/core/sgx/td_basic.c +++ b/enclave/core/sgx/td_basic.c @@ -110,6 +110,9 @@ void td_init(oe_sgx_td_t* td) /* List of callsites is initially empty */ td->callsites = NULL; + /* Set the exception handler stack to NULL */ + oe_sgx_td_set_exception_handler_stack(td, NULL, 0); + oe_thread_local_init(td); } } @@ -152,5 +155,8 @@ void td_clear(oe_sgx_td_t* td) /* Clear the magic number */ td->magic = 0; + /* Clear td states */ + oe_sgx_td_clear_states(td); + /* Never clear oe_sgx_td_t.initialized nor host registers */ } diff --git a/include/openenclave/internal/sgx/td.h b/include/openenclave/internal/sgx/td.h index 71543e7748..6486f596a8 100644 --- a/include/openenclave/internal/sgx/td.h +++ b/include/openenclave/internal/sgx/td.h @@ -227,20 +227,20 @@ OE_STATIC_ASSERT( /* Get the thread data object for the current thread */ oe_sgx_td_t* oe_sgx_get_td(void); -/* The following APIs are expected to be used only by the thread itself. */ +void oe_sgx_td_clear_states(oe_sgx_td_t* td); -bool oe_sgx_set_td_exception_handler_stack(void* stack, uint64_t size); +bool oe_sgx_td_set_exception_handler_stack( + oe_sgx_td_t* td, + void* stack, + uint64_t size); -void oe_sgx_td_mask_host_signal(); +void oe_sgx_td_mask_host_signal(oe_sgx_td_t* td); -void oe_sgx_td_unmask_host_signal(); +void oe_sgx_td_unmask_host_signal(oe_sgx_td_t* td); -/* The following APIs are expected to be used by both the thread itself - * and other threads. */ +bool oe_sgx_td_register_host_signal(oe_sgx_td_t* td, int signal_number); -bool oe_sgx_register_td_host_signal(oe_sgx_td_t* td, int signal_number); - -bool oe_sgx_unregister_td_host_signal(oe_sgx_td_t* td, int signal_number); +bool oe_sgx_td_unregister_host_signal(oe_sgx_td_t* td, int signal_number); bool oe_sgx_td_host_signal_registered(oe_sgx_td_t* td, int signal_number); diff --git a/tests/VectorException/enc/enc.c b/tests/VectorException/enc/enc.c index 31f1cfc67b..b87cf3b9ec 100644 --- a/tests/VectorException/enc/enc.c +++ b/tests/VectorException/enc/enc.c @@ -23,18 +23,19 @@ int initialize_exception_handler_stack( uint64_t* stack_size, int use_exception_handler_stack) { + oe_sgx_td_t* td = oe_sgx_get_td(); + if (use_exception_handler_stack) { *stack_size = EXCEPTION_HANDLER_STACK_SIZE; *stack = malloc(*stack_size); if (!*stack) return -1; - if (!oe_sgx_set_td_exception_handler_stack(*stack, *stack_size)) + if (!oe_sgx_td_set_exception_handler_stack(td, *stack, *stack_size)) return -1; } else { - oe_sgx_td_t* td = oe_sgx_get_td(); void* tcs = td_to_tcs(td); *stack_size = STACK_SIZE; *stack = (void*)((uint64_t)tcs - PAGE_SIZE - STACK_SIZE); @@ -48,9 +49,11 @@ void cleaup_exception_handler_stack( uint64_t* stack_size, int use_exception_handler_stack) { + oe_sgx_td_t* td = oe_sgx_get_td(); + if (use_exception_handler_stack) { - oe_sgx_set_td_exception_handler_stack(NULL, 0); + oe_sgx_td_set_exception_handler_stack(td, NULL, 0); free(*stack); } diff --git a/tests/VectorException/enc/init.cpp b/tests/VectorException/enc/init.cpp index 529c8059ad..d03ae5a307 100644 --- a/tests/VectorException/enc/init.cpp +++ b/tests/VectorException/enc/init.cpp @@ -58,12 +58,14 @@ __attribute__((constructor)) void test_cpuid_constructor() void* stack = malloc(EXCEPTION_HANDLER_STACK_SIZE); if (!stack) return; - oe_sgx_set_td_exception_handler_stack(stack, EXCEPTION_HANDLER_STACK_SIZE); + oe_sgx_td_t* td = oe_sgx_get_td(); + oe_sgx_td_set_exception_handler_stack( + td, stack, EXCEPTION_HANDLER_STACK_SIZE); test_cpuid_instruction(500, 1); test_cpuid_instruction(600, 1); test_cpuid_instruction(AESNI_INSTRUCTIONS, 1); - oe_sgx_set_td_exception_handler_stack(NULL, 0); + oe_sgx_td_set_exception_handler_stack(td, NULL, 0); free(stack); } diff --git a/tests/sgx/td_state/enc/enc.c b/tests/sgx/td_state/enc/enc.c index 0741d49a36..e99f589632 100644 --- a/tests/sgx/td_state/enc/enc.c +++ b/tests/sgx/td_state/enc/enc.c @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -11,6 +12,7 @@ #include #include +#include #define OE_EXPECT(a, b) \ do \ @@ -30,11 +32,7 @@ } \ } while (0) -bool oe_sgx_register_target_td_host_signal( - oe_sgx_td_t* target_td, - int signal_number); - -typedef struct _thread_info_nonblocking_t +typedef struct _thread_info_t { int tid; oe_sgx_td_t* td; @@ -44,6 +42,9 @@ static thread_info_t _thread_info; static volatile int _handler_done; static volatile int* _host_lock_state; +static thread_info_t _thread_handler_no_return_info; +static oe_jmpbuf_t jump_buffer; + static void cpuid( unsigned int leaf, unsigned int subleaf, @@ -163,7 +164,7 @@ static uint64_t td_state_handler(oe_exception_record_t* exception_record) OE_TEST(oe_sgx_td_is_handling_host_signal(_thread_info.td)); OE_TEST( - oe_sgx_unregister_td_host_signal(_thread_info.td, SIGUSR1) == true); + oe_sgx_td_unregister_host_signal(_thread_info.td, SIGUSR1) == true); __atomic_store_n(_host_lock_state, 2, __ATOMIC_RELEASE); @@ -221,7 +222,7 @@ void enc_run_thread(int tid) OE_CHECK(oe_add_vectored_exception_handler(false, td_state_handler)); // Invoke the internal API to unmask host signals - oe_sgx_td_unmask_host_signal(); + oe_sgx_td_unmask_host_signal(_thread_info.td); // Ensure the order of setting the lock asm volatile("" ::: "memory"); @@ -273,6 +274,8 @@ void enc_run_thread(int tid) // Expect the state to be persisted after an exception. OE_EXPECT(_thread_info.td->state, OE_TD_STATE_RUNNING); + OE_CHECK(oe_remove_vectored_exception_handler(td_state_handler)); + printf("(tid=%d) thread is exiting...\n", self_tid); done: return; @@ -308,7 +311,7 @@ void enc_td_state(uint64_t lock_state) OE_TEST(_thread_info.tid != 0); host_sleep_msec(30); - OE_TEST(oe_sgx_register_td_host_signal(_thread_info.td, SIGUSR1) == true); + OE_TEST(oe_sgx_td_register_host_signal(_thread_info.td, SIGUSR1) == true); printf( "(tid=%d) Sending interrupt to (td=0x%lx, tid=%d) inside the " @@ -346,6 +349,82 @@ void enc_td_state(uint64_t lock_state) OE_EXPECT(_thread_info.td->state, OE_TD_STATE_EXITED); } +static uint64_t td_state_handler_no_return( + oe_exception_record_t* exception_record) +{ + if (exception_record->code == OE_EXCEPTION_DIVIDE_BY_ZERO) + { + oe_longjmp(&jump_buffer, 1); + } + + return OE_EXCEPTION_ABORT_EXECUTION; +} + +void enc_run_thread_handler_no_return(int tid) +{ + oe_result_t result = OE_OK; + + _thread_handler_no_return_info.tid = tid; + _thread_handler_no_return_info.td = oe_sgx_get_td(); + + printf( + "(tid=%d) thread is created td=0x%lx\n", + _thread_handler_no_return_info.tid, + (uint64_t)_thread_handler_no_return_info.td); + + OE_CHECK( + oe_add_vectored_exception_handler(false, td_state_handler_no_return)); + + if (oe_setjmp(&jump_buffer) == 0) + divide_by_zero_exception_function(); + + // Expect the state is still OE_TD_STATE_SECOND_LEVEL_EXCEPTION_HANDLING + // (the handler does not return) + OE_EXPECT( + _thread_handler_no_return_info.td->state, + OE_TD_STATE_SECOND_LEVEL_EXCEPTION_HANDLING); + +done: + return; +} + +void enc_run_thread_reuse_tcs(int tid) +{ + oe_sgx_td_t* td = oe_sgx_get_td(); + + // Expect the tcs is re-used + OE_EXPECT(td, _thread_handler_no_return_info.td); + + OE_EXPECT(_thread_handler_no_return_info.td->state, OE_TD_STATE_RUNNING); + + printf("(tid=%d) thread is created td=0x%lx\n", tid, (uint64_t)td); +} + +void enc_td_state_handler_no_return() +{ + oe_result_t result; + int tid = 0; + + host_get_tid(&tid); + OE_TEST(tid != 0); + + printf("(tid=%d) Create a thread...\n", tid); + + result = host_create_thread_handler_no_return(); + if (result != OE_OK) + return; + + host_join_thread(); + + printf("(tid=%d) Create a thread...\n", tid); + + result = host_create_thread_reuse_tcs(); + if (result != OE_OK) + return; + + host_join_thread(); +} + OE_SET_ENCLAVE_SGX( 1, /* ProductID */ 1, /* SecurityVersion */ diff --git a/tests/sgx/td_state/host/host.c b/tests/sgx/td_state/host/host.c index 065afcfc01..cc2abfb508 100644 --- a/tests/sgx/td_state/host/host.c +++ b/tests/sgx/td_state/host/host.c @@ -65,6 +65,42 @@ void host_create_thread() pthread_attr_destroy(&attr); } +static void* _thread_function_handler_no_return() +{ + pid_t tid = (pid_t)syscall(SYS_gettid); + + enc_run_thread_handler_no_return(enclave, tid); + + return NULL; +} + +void host_create_thread_handler_no_return() +{ + pthread_attr_t attr; + + pthread_attr_init(&attr); + pthread_create(&_thread, &attr, _thread_function_handler_no_return, NULL); + pthread_attr_destroy(&attr); +} + +static void* _thread_function_reuse_tcs() +{ + pid_t tid = (pid_t)syscall(SYS_gettid); + + enc_run_thread_reuse_tcs(enclave, tid); + + return NULL; +} + +void host_create_thread_reuse_tcs() +{ + pthread_attr_t attr; + + pthread_attr_init(&attr); + pthread_create(&_thread, &attr, _thread_function_reuse_tcs, NULL); + pthread_attr_destroy(&attr); +} + void host_join_thread() { pthread_join(_thread, NULL); @@ -102,10 +138,18 @@ int main(int argc, const char* argv[]) argv[1], OE_ENCLAVE_TYPE_SGX, flags, NULL, 0, &enclave)) != OE_OK) oe_put_err("oe_create_enclave(): result=%u", result); + printf("=== test the td state on a thread\n"); + result = enc_td_state(enclave, (uint64_t)&_lock_state); if (result != OE_OK) oe_put_err("oe_call_enclave() failed: result=%u", result); + printf("=== test the handler no return\n"); + + result = enc_td_state_handler_no_return(enclave); + if (result != OE_OK) + oe_put_err("oe_call_enclave() failed: result=%u", result); + result = oe_terminate_enclave(enclave); OE_TEST(result == OE_OK); diff --git a/tests/sgx/td_state/td_state.edl b/tests/sgx/td_state/td_state.edl index 6efe781fe7..eed119d3f2 100644 --- a/tests/sgx/td_state/td_state.edl +++ b/tests/sgx/td_state/td_state.edl @@ -9,11 +9,16 @@ enclave { trusted { public void enc_td_state(uint64_t lock_state); public void enc_run_thread(int tid); + public void enc_td_state_handler_no_return(); + public void enc_run_thread_handler_no_return(int tid); + public void enc_run_thread_reuse_tcs(int tid); }; untrusted { void host_send_interrupt(int tid, int signal_number); void host_create_thread(); + void host_create_thread_handler_no_return(); + void host_create_thread_reuse_tcs(); void host_join_thread(); void host_spin(); int host_get_tid(); diff --git a/tests/sgx/thread_interrupt/enc/enc.c b/tests/sgx/thread_interrupt/enc/enc.c index 175eda70e7..1077c1f272 100644 --- a/tests/sgx/thread_interrupt/enc/enc.c +++ b/tests/sgx/thread_interrupt/enc/enc.c @@ -76,7 +76,7 @@ void enc_run_thread_nonblocking(int tid) OE_TEST(_thread_info_nonblocking.td->state == OE_TD_STATE_RUNNING); // Invoke the internal API to unmask host signals - oe_sgx_td_unmask_host_signal(); + oe_sgx_td_unmask_host_signal(_thread_info_nonblocking.td); // Ensure the order of setting the lock asm volatile("" ::: "memory"); @@ -104,7 +104,7 @@ void enc_run_thread_nonblocking(int tid) // Test unregistering signals for (size_t i = 0; i < SIGNAL_NUMBER; i++) { - OE_TEST(oe_sgx_unregister_td_host_signal( + OE_TEST(oe_sgx_td_unregister_host_signal( _thread_info_nonblocking.td, _signal_list[i])); } __atomic_store_n(&_thread_info_nonblocking.lock, 4, __ATOMIC_RELEASE); @@ -165,7 +165,7 @@ void enc_thread_interrupt_nonblocking(void) // Signal registration OE_TEST( - oe_sgx_register_td_host_signal( + oe_sgx_td_register_host_signal( _thread_info_nonblocking.td, _current_signal) == true); printf( @@ -205,7 +205,7 @@ void enc_thread_interrupt_nonblocking(void) // Register SIGUSR1 again OE_TEST( - oe_sgx_register_td_host_signal( + oe_sgx_td_register_host_signal( _thread_info_nonblocking.td, _current_signal) == true); // Sending the same signal multiple times @@ -251,7 +251,7 @@ void enc_run_thread_blocking(int tid) OE_TEST(_thread_info_blocking.td->state == OE_TD_STATE_RUNNING); // Mask host signals (the default behavior) - oe_sgx_td_mask_host_signal(); + oe_sgx_td_mask_host_signal(_thread_info_blocking.td); // Ensure the order of setting the lock asm volatile("" ::: "memory"); @@ -296,7 +296,7 @@ void enc_thread_interrupt_blocking(void) while (!_handler_entered) { OE_TEST( - oe_sgx_register_td_host_signal(_thread_info_blocking.td, SIGUSR1) == + oe_sgx_td_register_host_signal(_thread_info_blocking.td, SIGUSR1) == true); printf( @@ -325,9 +325,9 @@ void enc_thread_interrupt_blocking(void) retry = 0; _handler_entered = 0; - // The oe_sgx_td_unmask_host_signal API is only expected to be used - // by the thread itself. Set the flag direclty for the testing purposes. - _thread_info_blocking.td->host_signal_unmasked = 1; + // The oe_sgx_td_unmask_host_signal API is usually expected to be used + // by the thread itself. Use here for testing purposes. + oe_sgx_td_unmask_host_signal(_thread_info_blocking.td); while (!_handler_entered) { diff --git a/tests/stack_overflow_exception/enc/enc.c b/tests/stack_overflow_exception/enc/enc.c index cc89da8ce1..94f6b13237 100644 --- a/tests/stack_overflow_exception/enc/enc.c +++ b/tests/stack_overflow_exception/enc/enc.c @@ -14,7 +14,6 @@ #define EXCEPTION_HANDLER_STACK_SIZE 8192 #define STACK_PAGE_NUMBER 2 #define STACK_SIZE (STACK_PAGE_NUMER * PAGE_SIZE) -bool oe_sgx_set_td_exception_handler_stack(void* stack, uint64_t size); void* td_to_tcs(const oe_sgx_td_t* td); uint8_t exception_handler_stack[EXCEPTION_HANDLER_STACK_SIZE]; @@ -34,12 +33,13 @@ uint64_t test_stack_overflow_handler(oe_exception_record_t* exception_record) return OE_EXCEPTION_ABORT_EXECUTION; } -oe_result_t enc_initialize_exception_handler() +static oe_result_t _initialize_exception_handler() { oe_result_t result = OE_FAILURE; + oe_sgx_td_t* td = oe_sgx_get_td(); - if (!oe_sgx_set_td_exception_handler_stack( - exception_handler_stack, EXCEPTION_HANDLER_STACK_SIZE)) + if (!oe_sgx_td_set_exception_handler_stack( + td, exception_handler_stack, EXCEPTION_HANDLER_STACK_SIZE)) goto done; OE_CHECK( @@ -51,7 +51,7 @@ oe_result_t enc_initialize_exception_handler() return result; } -void enc_stack_overflow_exception() +static void _stack_overflow_exception() { uint8_t data[1024]; @@ -65,6 +65,13 @@ void enc_stack_overflow_exception() : "r8"); } +void enc_stack_overflow_exception() +{ + _initialize_exception_handler(); + + _stack_overflow_exception(); +} + OE_SET_ENCLAVE_SGX2( 1, /* ProductID */ 1, /* SecurityVersion */ diff --git a/tests/stack_overflow_exception/host/host.c b/tests/stack_overflow_exception/host/host.c index 689ad66603..6912cb28e9 100644 --- a/tests/stack_overflow_exception/host/host.c +++ b/tests/stack_overflow_exception/host/host.c @@ -53,11 +53,6 @@ int main(int argc, const char* argv[]) "ECALL.\n"); else if (_is_misc_region_supported()) { - if (enc_initialize_exception_handler(enclave, &result) != OE_OK) - oe_put_err( - "enc_initialize_exception_handler() failed: result=%u", result); - OE_TEST(result == OE_OK); - OE_TEST(enc_stack_overflow_exception(enclave) == OE_ENCLAVE_ABORTING); OE_TEST(enclave_stack_overflowed == true); diff --git a/tests/stack_overflow_exception/stack_overflow_exception.edl b/tests/stack_overflow_exception/stack_overflow_exception.edl index 31cea43dd2..859d069d49 100644 --- a/tests/stack_overflow_exception/stack_overflow_exception.edl +++ b/tests/stack_overflow_exception/stack_overflow_exception.edl @@ -7,7 +7,6 @@ enclave { from "openenclave/edl/sgx/platform.edl" import *; trusted { - public oe_result_t enc_initialize_exception_handler(); public void enc_stack_overflow_exception(); };