Merge openenclave#2388

2388: Mark oe_setjmp and setjmp as returns_twice r=jhand2 a=jhand2 The function setjmp in libc has a behavior known as "returns twice". This means that setjmp returns from normal control flow, but can also return via another method. The `longjmp` function resets a saved register state and jumps to the instruction immediately after a call to `setjmp`, which is functionally equivalent to a return from `setjmp`. clang 8 and 9 have a feature called Speculative Load Hardening, which is designed to mitigate some vulnerabilities in speculative execution. One such mitigation is to check for a return address in the "red zone" of the stack (a range of the stack below %rsp) to ensure proper control flow. You can read more [here](https://llvm.org/docs/SpeculativeLoadHardening.html#indirect-call-branch-and-return-predicates). Clang cannot apply these mitigations to functions with nonstandard control flow (like returns_twice) but in OE clang does not know that setjmp and oe_setjmp have this havavior. This PR adds annotations to these functions. Because it updates a 3rdparty library (musl) a .patch file is also included that can be applied to future versions of musl. It also changes oe_setjmp and oe_longjmp to use straight assembly rather than inline assembly in C files. This ensures that the stack is not improperly modified by compiler generated instructions. It also removes the need to compile setjmp and longjmp with specific optimization. Fixes openenclave#2386 Co-authored-by: Jordan Hand <[email protected]> Co-authored-by: Jordan Hand <[email protected]>
lsds · Jan 14, 2020 · 8911dfd · 8911dfd
2 parents 5ff92bd + a19accf
commit 8911dfd
Show file tree

Hide file tree

Showing 9 changed files with 81 additions and 113 deletions.
diff --git a/3rdparty/musl/CMakeLists.txt b/3rdparty/musl/CMakeLists.txt
@@ -55,6 +55,9 @@ ExternalProject_Add(musl_includes
     COMMAND ${CMAKE_COMMAND} -E copy
       ${PATCHES_DIR}/pthread_${ARCH}.h
       ${MUSL_DIR}/arch/${ARCH}/pthread_arch.h
+    COMMAND ${CMAKE_COMMAND} -E copy
+      ${PATCHES_DIR}/setjmp.h
+      ${MUSL_DIR}/include/setjmp.h
   CONFIGURE_COMMAND
     ${CMAKE_COMMAND} -E chdir ${MUSL_DIR}
     ${OE_BASH} -x ./configure

diff --git a/3rdparty/musl/patches/setjmp.h b/3rdparty/musl/patches/setjmp.h
@@ -0,0 +1,44 @@
+// Copyright (c) Open Enclave SDK contributors.
+// Licensed under the MIT License.
+
+#ifndef	_SETJMP_H
+#define	_SETJMP_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <features.h>
+
+#include <bits/setjmp.h>
+
+typedef struct __jmp_buf_tag {
+	__jmp_buf __jb;
+	unsigned long __fl;
+	unsigned long __ss[128/sizeof(long)];
+} jmp_buf[1];
+
+#if defined(_POSIX_SOURCE) || defined(_POSIX_C_SOURCE) \
+ || defined(_XOPEN_SOURCE) || defined(_GNU_SOURCE) \
+ || defined(_BSD_SOURCE)
+typedef jmp_buf sigjmp_buf;
+int sigsetjmp (sigjmp_buf, int) __attribute__((returns_twice));
+_Noreturn void siglongjmp (sigjmp_buf, int);
+#endif
+
+#if defined(_XOPEN_SOURCE) || defined(_GNU_SOURCE) \
+ || defined(_BSD_SOURCE)
+int _setjmp (jmp_buf) __attribute__((returns_twice));
+_Noreturn void _longjmp (jmp_buf, int);
+#endif
+
+int setjmp (jmp_buf) __attribute__((returns_twice));
+_Noreturn void longjmp (jmp_buf, int);
+
+#define setjmp setjmp
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
diff --git a/enclave/core/CMakeLists.txt b/enclave/core/CMakeLists.txt
@@ -60,7 +60,6 @@ if (OE_SGX)
         sgx/hostcalls.c
         sgx/init.c
         sgx/sgx_t_wrapper.c
-        sgx/jump.c
         sgx/keys.c
         sgx/memory.c
         sgx/properties.c
@@ -192,9 +191,6 @@ set_source_files_properties(malloc.c
     PROPERTIES COMPILE_FLAGS "-Wno-conversion -Wno-null-pointer-arithmetic")
 
 if (OE_SGX)
-    # jump.s must be optimized for the correct call-frame.
-    set_source_files_properties(sgx/jump.c PROPERTIES COMPILE_FLAGS -O2)
-
     set_source_files_properties(sgx/keys.c PROPERTIES COMPILE_FLAGS -Wno-type-limits)
 
     # -m64 is an x86_64 specific flag

diff --git a/enclave/core/sgx/jump.c b/enclave/core/sgx/jump.c
diff --git a/enclave/core/sgx/longjmp.S b/enclave/core/sgx/longjmp.S
@@ -3,7 +3,7 @@
 
 //==============================================================================
 //
-// void oe_longjmp(oe_jmp_buf env, int val)
+// void oe_longjmp(oe_jmp_buf* env, int val)
 //
 //     Implementation of standard longjmp() function.
 //
@@ -12,7 +12,7 @@
 //
 //==============================================================================
 
-/* ATTN: WINPORT */
+// Modified from musl-libc root/src/setjmp/x86_64/longjmp.s
 
 .globl oe_longjmp
 .type oe_longjmp, @function
@@ -22,16 +22,15 @@ oe_longjmp:
     // if (val == 0) then set val to 1.
     cmp $0, %rax
     jne val_is_nonzero
-    mov $1, %rax
+    mov $1, %rax # Return value (int)
+
 val_is_nonzero:
-    mov (%rdi), %rbx
-    mov 8(%rdi), %rbp
-    mov 16(%rdi), %r12
-    mov 24(%rdi), %r13
-    mov 32(%rdi), %r14
-    mov 40(%rdi), %r15
-    mov 48(%rdi), %rdx
-    mov %rdx, %rsp
-    mov 56(%rdi), %rdx
-    jmp *%rdx
+    mov (%rdi),%rsp
+    mov 8(%rdi),%rbp
+    mov 24(%rdi),%rbx
+    mov 32(%rdi),%r12
+    mov 40(%rdi),%r13
+    mov 48(%rdi),%r14
+    mov 56(%rdi),%r15
+    jmp *16(%rdi)
 .cfi_endproc
diff --git a/enclave/core/sgx/setjmp.S b/enclave/core/sgx/setjmp.S
@@ -3,30 +3,30 @@
 
 //==============================================================================
 //
-// void oe_setjmp(oe_jmp_buf env)
+// void oe_setjmp(oe_jmp_buf* env)
 //
 //     Implementation of standard setjmp() function.
 //
 //     %rdi := env
 //
 //==============================================================================
 
-/* ATTN: WINPORT */
+// Modified from musl-libc root/src/setjmp/x86_64/setjmp.s
 
 .globl oe_setjmp
 .type oe_setjmp,@function
 oe_setjmp:
 .cfi_startproc
-    mov %rbx, (%rdi)
-    mov %rbp, 8(%rdi)
-    mov %r12, 16(%rdi)
-    mov %r13, 24(%rdi)
-    mov %r14, 32(%rdi)
-    mov %r15, 40(%rdi)
-    lea 8(%rsp), %rdx
-    mov %rdx, 48(%rdi)
-    mov (%rsp), %rdx
-    mov %rdx, 56(%rdi)
-    xor %rax, %rax
+    lea  8(%rsp), %rdx # this is our rsp WITHOUT current ret addr
+    mov  %rdx, (%rdi)
+    mov  %rbp, 8(%rdi)
+    mov  (%rsp), %rdx # save return addr ptr for new rip
+    mov  %rdx, 16(%rdi)
+    mov  %rbx, 24(%rdi)
+    mov  %r12, 32(%rdi)
+    mov  %r13, 40(%rdi)
+    mov  %r14, 48(%rdi)
+    mov  %r15, 56(%rdi)
+    xorl %eax, %eax # Set return value
     ret
 .cfi_endproc
diff --git a/include/openenclave/bits/defs.h b/include/openenclave/bits/defs.h
@@ -51,6 +51,12 @@
 #define OE_INLINE static __inline__
 #endif
 
+#if defined(__GNUC__) || defined(__clang__)
+#define OE_RETURNS_TWICE __attribute__((returns_twice))
+#else
+#define OE_RETURNS_TWICE
+#endif
+
 #ifdef _MSC_VER
 #define OE_NO_OPTIMIZE_BEGIN __pragma(optimize("", off))
 #define OE_NO_OPTIMIZE_END __pragma(optimize("", on))

diff --git a/include/openenclave/corelibc/setjmp.h b/include/openenclave/corelibc/setjmp.h
@@ -6,6 +6,7 @@
 
 #include <openenclave/bits/defs.h>
 #include <openenclave/bits/types.h>
+#include <openenclave/corelibc/bits/defs.h>
 
 /*
 **==============================================================================
@@ -21,7 +22,7 @@
 #undef ___OE_JMP_BUF
 #undef __OE_JMP_BUF
 
-int oe_setjmp(oe_jmp_buf* env);
+int oe_setjmp(oe_jmp_buf* env) OE_RETURNS_TWICE;
 
 void oe_longjmp(oe_jmp_buf* env, int val);
 

diff --git a/include/openenclave/internal/jump.h b/include/openenclave/internal/jump.h
@@ -26,7 +26,7 @@ typedef struct _oe_jmpbuf
     uint64_t r15;
 } oe_jmpbuf_t;
 
-int oe_setjmp(oe_jmpbuf_t* env);
+int oe_setjmp(oe_jmpbuf_t* env) OE_RETURNS_TWICE;
 
 void oe_longjmp(oe_jmpbuf_t* env, int val);