Security fix for AC flag poisoning and MCDT

mingweishih · radhikaj · commit 441499e49f52 · 2023-07-17T09:42:02.000-07:00
This PR does the following
- Ensuring the AC flag along with other system/control flags are always cleared upon enclave enter
- Update the inital MXCSR value to 0x1FBF and put lfence after MXCSR load (via LDMXCSR, XRSTOR, or FXRSTOR)
  for the MCDT mitigation

Signed-off-by: Ming-Wei Shih &lt;mishih@microsoft.com&gt;
diff --git a/enclave/core/sgx/asmcommon.inc b/enclave/core/sgx/asmcommon.inc
@@ -11,7 +11,7 @@
 //
 //  - The legacy SSE state:
 //    - Initializes the FPU control word to ABI-specified value 0x037F
-//    - Initializes MXCSR to ABI-specified value 0x1F80 and MXCSR_MASK to 0xFFFF
+//    - Initializes MXCSR to ABI-specified value 0x1FBF and MXCSR_MASK to 0xFFFF
 //    - Clears FPU Status, Tag, OpCode, and FIP words
 //    - Clears all FDP bits
 //    - Clears all MMX/FPU registers
@@ -46,6 +46,9 @@
     // Restore initial enclave legacy SSE state
     fxrstor64 OE_XSAVE_INITIAL_STATE(%rip)
 2:
+    // Put a lfence after changing MXCSR for MXCSR Configuration Dependent
+    // Timing (MCDT) mitigation
+    lfence
     // Restore the registers
     mov %r8, %rax
     mov %r9, %rdx
@@ -80,17 +83,36 @@
     xor %r14, %r14
     xor %r15, %r15
 
-    // Zero out the RFLAGS except for system flags and
-    // reserved bits that are not writable by the enclave
+    // Zero out the status flags (CF, PF, AF, SF, OF) that could leak
+    // information about instructions executed by the enclave without using stack.
+    // Doing so avoiding using untrusted host stack when cleaning up registers
+    // during enclave enter and exit routines.
+    // To clear system and control flags, see oe_cleanup_flags_on_enclave_stack
+    // for more detail.
     mov %rax, %r8
     xor %rax, %rax
     test %al, %al // Clear OF
-    cld // Clear DF
     sahf // Clear SF, ZF AF, PF, and CF
     mov %r8, %rax
 
     // No need to clear r8, which equals to rax (return value to the host)
 
 .endm
 
+//==============================================================================
+//
+// This macro is used to clean up the enclave FLAGS register, including not only
+// status but also system and control flags.
+//
+// The macro does not reserve r15 and require consuming stack. Please make sure
+// using the macro within the enclave stack. Currently, the macro is only used
+// during oe_enter.
+//
+//==============================================================================
+.macro oe_cleanup_flags_on_enclave_stack
+    xor %r15, %r15
+    pushq %r15
+    popfq
+.endm
+
 #endif
diff --git a/enclave/core/sgx/asmdefs.c b/enclave/core/sgx/asmdefs.c
@@ -16,9 +16,9 @@ OE_XSAVE_INITIAL_STATE[OE_MINIMAL_XSTATE_AREA_SIZE/sizeof(uint32_t)] = {
      * clear Status, Tag, OpCode, FIP words */
     0x037F, 0, 0, 0,
 
-    /* Clear FDP bits, set MXCSR to ABI init value of 0x1F80
+    /* Clear FDP bits, set MXCSR to ABI init value of 0x1FBF
      * and MXCSR_MASK to all bits (0XFFFF) */
-    0, 0, 0x1F80, 0xFFFF,
+    0, 0, 0x1FBF, 0xFFFF,
 
     /* Clear ST/MM0-7 */
     0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
@@ -35,10 +35,12 @@ OE_XSAVE_INITIAL_STATE[OE_MINIMAL_XSTATE_AREA_SIZE/sizeof(uint32_t)] = {
     0, 0, 0, 0, 0, 0, 0, 0,
 
     /* XSAVE Header */
-    /* Clear XSTATE_BV. Note that this means we don't support
-     * compaction mode (XCOMP_BV[63]) to accommodate running
-     * the same code in simulation mode on older CPUs. */
-    0, 0, 0, 0,
+    /* Set XSTATE_BV[1] to 1 (SSE state) */
+    2, 0,
+    /* Set XCOMP_BV[1] to 1 (SSE state), allowing non-default
+     * MXCSR value to be restored.
+     * Also, set XCOMP_BV[63] to 1 for compaction mode */
+    2, 0x80000000,
 
     /* Reserved XSAVE header bits */
     0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
diff --git a/enclave/core/sgx/calls.c b/enclave/core/sgx/calls.c
@@ -641,15 +641,17 @@ OE_INLINE void _handle_oret(
     td->oret_arg = arg;
 
     /* Restore the FXSTATE and flags */
-    asm volatile("pushq %[rflags] \n\t" // Restore flags.
-                 "popfq \n\t"
-                 "fldcw %[fcw] \n\t"     // Restore x87 control word
-                 "ldmxcsr %[mxcsr] \n\t" // Restore MXCSR
-                 : [mxcsr] "=m"(callsite->mxcsr),
-                   [fcw] "=m"(callsite->fcw),
-                   [rflags] "=m"(callsite->rflags)
-                 :
-                 : "cc");
+    asm volatile(
+        "pushq %[rflags] \n\t" // Restore flags.
+        "popfq \n\t"
+        "fldcw %[fcw] \n\t"     // Restore x87 control word
+        "ldmxcsr %[mxcsr] \n\t" // Restore MXCSR
+        "lfence \n\t" // MXCSR Configuration Dependent Timing (MCDT) mitigation
+        : [mxcsr] "=m"(callsite->mxcsr),
+          [fcw] "=m"(callsite->fcw),
+          [rflags] "=m"(callsite->rflags)
+        :
+        : "cc");
 
     oe_longjmp(&callsite->jmpbuf, 1);
 }
diff --git a/enclave/core/sgx/context.inc b/enclave/core/sgx/context.inc
@@ -106,6 +106,9 @@ oe_continue_execution:
     // oe_snap_current_context.
     ldmxcsr OE_CONTEXT_MXCSR(%rdi)
 
+    // For MXCSR Configuration Dependent Timing (MCDT) mitigation
+    lfence
+
     // Restore general registers.
     // Restore rax, rbx, rcx, rdx.
     movq    OE_CONTEXT_RAX(%rdi), %rax
diff --git a/enclave/core/sgx/enter.S b/enclave/core/sgx/enter.S
@@ -314,6 +314,13 @@ oe_enter:
     xor %r11, %r11
     oe_cleanup_registers
 
+    // Clear the flags not covered by oe_cleanup_registers (i.e., control and system
+    // flags) that requires using (enclave) stack. Given that these flags are not
+    // affected instructions and persistent throughout the enclave lifetime, we only
+    // clear them once during the enter routine, which prevents poisoning attack from
+    // the host.
+    oe_cleanup_flags_on_enclave_stack
+
     // Call __oe_handle_main(ARG1=RDI, ARG2=RSI, CSSA=RDX, TCS=RCX, OUTPUTARG1=R8, OUTPUTARG2=R9)
     mov %rax, %rdx
     mov %rbx, %rcx
diff --git a/tests/abi/abi_utils.h b/tests/abi/abi_utils.h
@@ -24,8 +24,8 @@ OE_EXTERNC_END
 // Set FTZ, Truncation RC and DAZ, clear all exception masks
 #define TEST_MXCSR 0xE040
 
-// Initial MXCSR value as defined by Linux/Window ABIs
-#define INIT_MXCSR 0x1F80
+// Default MXCSR value used by OE for MCDT mitigation
+#define INIT_MXCSR 0x1FBF
 
 // Set RC to RNE, PC to SP, clear all exception masks (11 00 01 000000)
 #define TEST_FCW 0xC40
diff --git a/tests/abi/enc/enc.cpp b/tests/abi/enc/enc.cpp
@@ -76,9 +76,25 @@ double enclave_check_abi()
          .deepcopy_out_buffer_size = 0,
          ._retval = 0}};
 
+    flat_ocall_args_t* args = NULL;
+
+    /* Get the FLAGS */
+    uint64_t flags = 0xffffffff;
+    asm volatile("pushfq\n\t"
+                 "popq %0"
+                 : "=r"(flags)
+                 :
+                 : "r8");
+
+    /* Validate bit 10 (DF) and 16 - 31 bits are cleared by the
+     * oe_enter routine.
+     * Skip checks against other bits given that they could be impacted
+     * by branch or memory access executed until this point */
+    if (flags & 0xffff0400)
+        goto done;
+
     /* Alloc and initialize host_check_abi OCALL args buffer */
-    flat_ocall_args_t* args =
-        (flat_ocall_args_t*)oe_allocate_ocall_buffer(sizeof(args_template));
+    args = (flat_ocall_args_t*)oe_allocate_ocall_buffer(sizeof(args_template));
     if (!args)
         goto done;
 
diff --git a/tests/abi/host/host.cpp b/tests/abi/host/host.cpp
@@ -145,6 +145,23 @@ oe_result_t test_abi_roundtrip(oe_enclave_t* enclave)
         set_test_abi_state();
         read_abi_state(&before_ecall_state);
 
+#ifndef _WIN32
+        /* Explicitly poison FLAGS bits 10 (DF) and 15-31 while reserving the
+         * others
+         */
+        uint64_t flags = 0xffff0400;
+#else
+        /* Do not set DF on Windows, which will affect the behavior of
+         * memset_repmovs that is invoked in the critical path on the host */
+        uint64_t flags = 0xffff0000;
+#endif
+        asm volatile("pushfq\n\t"
+                     "or %0, (%%rsp)\n\t"
+                     "popfq\n\t"
+                     :
+                     : "r"(flags)
+                     : "r8");
+
         /* Invoke oe_enter directly to test the ocall transition ABI handling */
         oe_enter(tcs, OE_AEP_ADDRESS, arg1, arg2, &arg3, &arg4, enclave);
 
diff --git a/tests/libc/enc/enc.c b/tests/libc/enc/enc.c
@@ -32,25 +32,25 @@
 void _reset_fxsave_state()
 {
     /* Initialize the FXSAVE state values to Linux x86-64 ABI defined values:
-     * FCW = 0x037F, MXCSR = 0x1F80, MXCSR mask = 0xFFFF */
+     * FCW = 0x037F, MXCSR = 0x1FBF, MXCSR mask = 0xFFFF */
     static OE_ALIGNED(OE_FXSAVE_ALIGNMENT) const uint64_t
         _initial_fxstate[OE_FXSAVE_AREA_SIZE / sizeof(uint64_t)] = {
-            0x037F, 0, 0, 0xFFFF00001F80,
-            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, 0, 0,
-            0,      0, 0, 0,
-            0,      0, 0, 0,
-            0,      0, 0, 0,
+            0x037F, 0,          0, 0xFFFF00001FBF,
+            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,          0, 0,
+            0,      0,          0, 0,
+            2,      0x80000002, 0, 0,
+            0,      0,          0, 0,
         };
 
     asm volatile("fxrstor %[fx_state] \n\t"
