[zircon][debugger] Don't write reserved part of mxcsr register. This CL fixes a kernel panic in zircon, in which the `zx_thread_write_state` syscall could overwrite the entire `mxcsr`, including the reserved bits. According to section "10.2.3 MXCSR Control and Status Register" in Intel's Software Developer Manual Volume 1: Basic Architecture, writing a non-zero value to the reserved bits causes a general protection fault. The fix reads the mxcsr_mask from the `x86_xsave_legacy_area` and checks whether the function is trying to write into a non-set bit. Note that it's possible for the mxcsr_mask to be zero, and in that case, the mask is hard-coded to the value 0x0000ffbf, as specified by the intel and amd manuals[0][1]. If the mxcsr has invalid bits, the function returns `ZX_ERR_INVALID_ARGS`, not copying anything to the fxsave area. I added a unit test to test this behavior: writing all ones to the register would trigger a general protection fault if this fix is not included. To run the test: ``` $ fx set bringup.x64 \ --with-base //garnet/packages/tests:zircon \ --with-base //bundles/bringup:tests ``` And run it with `runtests -t threads-test` BUG=50632 This vulnerability was reported by: Quarkslab [0]: Intel 64 and IA-32 Architectures Software Developer’s Manual Volume 1: Basic Architecture Section: 11.6.6 Guidelines for Writing to the MXCSR Register [1]: AMD64 Architecture Programmer’s Manual Volume 2: System Programming Section: 11.5.9 MXCSR State Management Change-Id: If1794258a086e20beb24591e63d8da87b74b828a Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/383146 Commit-Queue: Marco Vanotti <mvanotti@google.com> Reviewed-by: Gianfranco Valentino <gevalentino@google.com> Reviewed-by: Venkatesh Srinivas <venkateshs@google.com> Reviewed-by: Brett Wilson <brettw@google.com> Testability-Review: Gianfranco Valentino <gevalentino@google.com> Testability-Review: Brett Wilson <brettw@google.com>

diff --git a/zircon/kernel/arch/x86/debugger.cc b/zircon/kernel/arch/x86/debugger.cc
index 6d20d23..ca42445 100644
--- a/zircon/kernel/arch/x86/debugger.cc
+++ b/zircon/kernel/arch/x86/debugger.cc

@@ -87,6 +87,28 @@
 // Whether an operation gets thread state or sets it.
 enum class RegAccess { kGet, kSet };
 
+// Checks whether the mxcsr register has unsupported bits.
+// The processor specifies which flags of the mxcsr are supported via the
+// mxcsr_mask obtained with the fxsave instruction.
+//
+// The manuals mention that it is possible for the mask to be 0, and specify
+// 0x000ffbf as the default value.
+//
+// For details see:
+//   Intel 64 and IA-32 Architectures Software Developer’s Manual
+//     Volume 1: Basic Architecture
+//     Section: 11.6.6 Guidelines for Writing to the MXCSR Register
+//   AMD64 Architecture Programmer’s Manual
+//     Volume 2: System Programming
+//     Section: 11.5.9  MXCSR State Management
+static inline bool mxcsr_is_valid(uint32_t mxcsr, uint32_t mxcsr_mask) {
+  if (mxcsr_mask == 0x0) {
+    mxcsr_mask = 0x0000ffbf;
+  }
+
+  return mxcsr & ~mxcsr_mask;
+}
+
 // Backend for arch_get_vector_regs and arch_set_vector_regs. This does a read or write of the
 // thread to or from the regs structure.
 zx_status_t x86_get_set_vector_regs(Thread* thread, zx_thread_state_vector_regs* regs,
@@ -121,6 +143,14 @@
           thread->arch_.extended_register_state, X86_XSAVE_STATE_INDEX_SSE, mark_present,
           &comp_size));
   DEBUG_ASSERT(save);  // Legacy getter should always succeed.
+
+  // fxbug.dev/50632: Overwriting the reserved bits of the mxcsr register
+  // causes a #GP Fault. We need to check against the mxcsr_mask to see if the
+  // proposed mxcsr is valid.
+  if (access == RegAccess::kSet && mxcsr_is_valid(regs->mxcsr, save->mxcsr_mask)) {
+    return ZX_ERR_INVALID_ARGS;
+  }
+
   for (int i = 0; i < kNumSSERegs; i++) {
     get_set_memcpy(&regs->zmm[i].v[0], &save->xmm[i], kXmmRegSize);
   }
@@ -244,8 +274,8 @@
       x86_fill_in_syscall_from_gregs(thread->arch_.suspended_general_regs.syscall, in);
       break;
     }
-  default:
-    ASSERT(false);
+    default:
+      ASSERT(false);
   }
 
   thread->arch_.fs_base = in->fs_base;

diff --git a/zircon/system/utest/core/threads/threads.cc b/zircon/system/utest/core/threads/threads.cc
index 40d6593..db3ecb7 100644
--- a/zircon/system/utest/core/threads/threads.cc
+++ b/zircon/system/utest/core/threads/threads.cc

@@ -152,9 +152,7 @@
     }
   }
 
-  void GrowStackVmo() {
-    ASSERT_EQ(stack_handle_.set_size(kStackSize), ZX_OK);
-  }
+  void GrowStackVmo() { ASSERT_EQ(stack_handle_.set_size(kStackSize), ZX_OK); }
 
   bool StartThread(zxr_thread_entry_t entry, void* arg) {
     return zxr_thread_start(thread_, stack_, kStackSize, entry, arg) == ZX_OK;
@@ -917,9 +915,7 @@
 }
 
 // Test signal delivery of suspended threads via single async wait.
-TEST(Threads, SuspendSingleWaitAsyncSignalDelivery) {
-  TestSuspendWaitAsyncSignalDeliveryWorker();
-}
+TEST(Threads, SuspendSingleWaitAsyncSignalDelivery) { TestSuspendWaitAsyncSignalDeliveryWorker(); }
 
 // Test signal delivery of suspended threads via repeating async wait.
 TEST(Threads, SuspendRepeatingWaitAsyncSignalDelivery) {
@@ -1199,6 +1195,31 @@
   EXPECT_TRUE(vector_regs_expect_eq(regs_to_set, regs));
 }
 
+// Test for fxbug.dev/50632: Make sure zx_thread_write_state doesn't overwrite
+// reserved bits in mxcsr (x64 only).
+TEST(Threads, WriteThreadStateWithInvalidMxcsrIsInvalidArgs) {
+#if defined(__x86_64__)
+  RegisterWriteSetup<zx_thread_state_vector_regs_t> setup;
+  setup.Init();
+
+  zx_thread_state_vector_regs_t start_values;
+  ASSERT_OK(zx_thread_read_state(setup.thread_handle(), ZX_THREAD_STATE_VECTOR_REGS, &start_values,
+                                 sizeof(start_values)));
+
+  zx_thread_state_vector_regs_t regs_to_set;
+  vector_regs_fill_test_values(&regs_to_set);
+  regs_to_set.mxcsr = 0xffffffff;
+
+  EXPECT_EQ(ZX_ERR_INVALID_ARGS,
+            zx_thread_write_state(setup.thread_handle(), ZX_THREAD_STATE_VECTOR_REGS, &regs_to_set,
+                                  sizeof(regs_to_set)));
+
+  zx_thread_state_vector_regs_t end_values;
+  setup.DoSave(&save_vector_regs_and_exit_thread, &end_values);
+  EXPECT_TRUE(vector_regs_expect_eq(start_values, end_values));
+#endif  // defined(__x86_64__)
+}
+
 // This test starts a thread which reads and writes from TLS.
 TEST(Threads, ThreadLocalRegisterState) {
   RegisterWriteSetup<struct thread_local_regs> setup;