msan: account for AVX state when unpoison ucontext_t

dvyukov · dvyukov · commit 1298273e8206 · 2022-01-05T13:20:40.000+01:00
ucontext_t can be larger than its static size if it contains AVX state and YMM/ZMM registers. Currently a signal handler that tries to access that state can produce false positives with random origins on stack. Account for the additional ucontext_t state. Reviewed By: vitalybuka Differential Revision: https://reviews.llvm.org/D116208
diff --git a/compiler-rt/lib/msan/msan_interceptors.cpp b/compiler-rt/lib/msan/msan_interceptors.cpp
@@ -990,7 +990,7 @@ static void SignalAction(int signo, void *si, void *uc) {
   ScopedThreadLocalStateBackup stlsb;
   UnpoisonParam(3);
   __msan_unpoison(si, sizeof(__sanitizer_sigaction));
-  __msan_unpoison(uc, __sanitizer::ucontext_t_sz);
+  __msan_unpoison(uc, ucontext_t_sz(uc));
 
   typedef void (*sigaction_cb)(int, void *, void *);
   sigaction_cb cb =
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_freebsd.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_freebsd.cpp
@@ -130,7 +130,7 @@ unsigned struct_sigevent_sz = sizeof(struct sigevent);
 unsigned struct_sched_param_sz = sizeof(struct sched_param);
 unsigned struct_statfs_sz = sizeof(struct statfs);
 unsigned struct_sockaddr_sz = sizeof(struct sockaddr);
-unsigned ucontext_t_sz = sizeof(ucontext_t);
+unsigned ucontext_t_sz(void *ctx) { return sizeof(ucontext_t); }
 unsigned struct_rlimit_sz = sizeof(struct rlimit);
 unsigned struct_timespec_sz = sizeof(struct timespec);
 unsigned struct_utimbuf_sz = sizeof(struct utimbuf);
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_freebsd.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_freebsd.h
@@ -57,7 +57,7 @@ extern unsigned struct_sched_param_sz;
 extern unsigned struct_statfs64_sz;
 extern unsigned struct_statfs_sz;
 extern unsigned struct_sockaddr_sz;
-extern unsigned ucontext_t_sz;
+unsigned ucontext_t_sz(void *ctx);
 extern unsigned struct_rlimit_sz;
 extern unsigned struct_utimbuf_sz;
 extern unsigned struct_timespec_sz;
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_netbsd.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_netbsd.cpp
@@ -554,7 +554,7 @@ unsigned struct_tms_sz = sizeof(struct tms);
 unsigned struct_sigevent_sz = sizeof(struct sigevent);
 unsigned struct_sched_param_sz = sizeof(struct sched_param);
 unsigned struct_sockaddr_sz = sizeof(struct sockaddr);
-unsigned ucontext_t_sz = sizeof(ucontext_t);
+unsigned ucontext_t_sz(void *ctx) { return sizeof(ucontext_t); }
 unsigned struct_rlimit_sz = sizeof(struct rlimit);
 unsigned struct_timespec_sz = sizeof(struct timespec);
 unsigned struct_sembuf_sz = sizeof(struct sembuf);
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_netbsd.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_netbsd.h
@@ -45,7 +45,7 @@ extern unsigned struct_stack_t_sz;
 extern unsigned struct_sched_param_sz;
 extern unsigned struct_statfs_sz;
 extern unsigned struct_sockaddr_sz;
-extern unsigned ucontext_t_sz;
+unsigned ucontext_t_sz(void *ctx);
 
 extern unsigned struct_rlimit_sz;
 extern unsigned struct_utimbuf_sz;
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
@@ -173,6 +173,11 @@ typedef struct user_fpregs elf_fpregset_t;
 #include "sanitizer_internal_defs.h"
 #include "sanitizer_platform_limits_posix.h"
 
+// To prevent macro redefinition warning between our sanitizer_thread_safety.h
+// and system's scsi.h.
+#  undef RELEASE
+#  include "sanitizer_common.h"
+
 namespace __sanitizer {
   unsigned struct_utsname_sz = sizeof(struct utsname);
   unsigned struct_stat_sz = sizeof(struct stat);
@@ -214,10 +219,20 @@ namespace __sanitizer {
 #if !SANITIZER_ANDROID
   unsigned struct_statfs_sz = sizeof(struct statfs);
   unsigned struct_sockaddr_sz = sizeof(struct sockaddr);
-  unsigned ucontext_t_sz = sizeof(ucontext_t);
-#endif // !SANITIZER_ANDROID
 
-#if SANITIZER_LINUX
+  unsigned ucontext_t_sz(void *ctx) {
+#    if SANITIZER_LINUX && SANITIZER_X64
+    // See kernel arch/x86/kernel/fpu/signal.c for details.
+    const auto *fpregs = static_cast<ucontext_t *>(ctx)->uc_mcontext.fpregs;
+    if (fpregs->__glibc_reserved1[12] == FP_XSTATE_MAGIC1)
+      return reinterpret_cast<const char *>(fpregs) +
+             fpregs->__glibc_reserved1[13] - static_cast<const char *>(ctx);
+#    endif
+    return sizeof(ucontext_t);
+  }
+#  endif  // !SANITIZER_ANDROID
+
+#  if SANITIZER_LINUX
   unsigned struct_epoll_event_sz = sizeof(struct epoll_event);
   unsigned struct_sysinfo_sz = sizeof(struct sysinfo);
   unsigned __user_cap_header_struct_sz =
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
@@ -57,12 +57,12 @@ extern unsigned struct_regmatch_sz;
 extern unsigned struct_fstab_sz;
 extern unsigned struct_statfs_sz;
 extern unsigned struct_sockaddr_sz;
-extern unsigned ucontext_t_sz;
-#endif // !SANITIZER_ANDROID
+unsigned ucontext_t_sz(void *uctx);
+#  endif  // !SANITIZER_ANDROID
 
-#if SANITIZER_LINUX
+#  if SANITIZER_LINUX
 
-#if defined(__x86_64__)
+#    if defined(__x86_64__)
 const unsigned struct_kernel_stat_sz = 144;
 const unsigned struct_kernel_stat64_sz = 0;
 #elif defined(__i386__)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_solaris.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_solaris.cpp
@@ -89,7 +89,7 @@ namespace __sanitizer {
   unsigned struct_sched_param_sz = sizeof(struct sched_param);
   unsigned struct_statfs_sz = sizeof(struct statfs);
   unsigned struct_sockaddr_sz = sizeof(struct sockaddr);
-  unsigned ucontext_t_sz = sizeof(ucontext_t);
+  unsigned ucontext_t_sz(void *ctx) { return sizeof(ucontext_t); }
   unsigned struct_timespec_sz = sizeof(struct timespec);
 #if SANITIZER_SOLARIS32
   unsigned struct_statvfs64_sz = sizeof(struct statvfs64);
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_solaris.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_solaris.h
@@ -43,7 +43,7 @@ extern unsigned struct_sched_param_sz;
 extern unsigned struct_statfs64_sz;
 extern unsigned struct_statfs_sz;
 extern unsigned struct_sockaddr_sz;
-extern unsigned ucontext_t_sz;
+unsigned ucontext_t_sz(void *ctx);
 
 extern unsigned struct_timespec_sz;
 extern unsigned struct_rlimit_sz;
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_syscalls_netbsd.inc b/compiler-rt/lib/sanitizer_common/sanitizer_syscalls_netbsd.inc
@@ -2255,13 +2255,13 @@ PRE_SYSCALL(getcontext)(void *ucp_) { /* Nothing to do */ }
 POST_SYSCALL(getcontext)(long long res, void *ucp_) { /* Nothing to do */ }
 PRE_SYSCALL(setcontext)(void *ucp_) {
   if (ucp_) {
-    PRE_READ(ucp_, ucontext_t_sz);
+    PRE_READ(ucp_, ucontext_t_sz(ucp_));
   }
 }
 POST_SYSCALL(setcontext)(long long res, void *ucp_) {}
 PRE_SYSCALL(_lwp_create)(void *ucp_, long long flags_, void *new_lwp_) {
   if (ucp_) {
-    PRE_READ(ucp_, ucontext_t_sz);
+    PRE_READ(ucp_, ucontext_t_sz(ucp_));
   }
 }
 POST_SYSCALL(_lwp_create)
diff --git a/compiler-rt/test/msan/Linux/signal_mcontext.cpp b/compiler-rt/test/msan/Linux/signal_mcontext.cpp
@@ -0,0 +1,38 @@
+// RUN: %clangxx_msan -fsanitize-memory-track-origins=2 -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+
+#include <pthread.h>
+#include <sanitizer/msan_interface.h>
+#include <signal.h>
+#include <stdio.h>
+#include <ucontext.h>
+#include <unistd.h>
+
+void handler(int sig, siginfo_t *info, void *uctx) {
+  __msan_check_mem_is_initialized(uctx, sizeof(ucontext_t));
+#if defined(__x86_64__)
+  auto *mctx = &static_cast<ucontext_t *>(uctx)->uc_mcontext;
+  if (auto *fpregs = mctx->fpregs) {
+    if (fpregs->__glibc_reserved1[12] == FP_XSTATE_MAGIC1) {
+      auto *xstate = reinterpret_cast<_xstate *>(mctx->fpregs);
+      __msan_check_mem_is_initialized(xstate, sizeof(*xstate));
+    }
+  }
+#endif
+}
+
+__attribute__((noinline)) void poison_stack() {
+  char buf[64 << 10];
+  printf("buf: %p-%p\n", buf, buf + sizeof(buf));
+}
+
+int main(int argc, char **argv) {
+  struct sigaction act = {};
+  act.sa_sigaction = handler;
+  act.sa_flags = SA_SIGINFO;
+  sigaction(SIGPROF, &act, 0);
+  poison_stack();
+  pthread_kill(pthread_self(), SIGPROF);
+  return 0;
+}
+
+// CHECK-NOT: WARNING: MemorySanitizer:

Original file line number	Diff line number	Diff line change
`@@ -2255,13 +2255,13 @@ PRE_SYSCALL(getcontext)(void ucp_) { / Nothing to do */ }`
`2255`	`2255`	`POST_SYSCALL(getcontext)(long long res, void ucp_) { / Nothing to do */ }`
`2256`	`2256`	`PRE_SYSCALL(setcontext)(void *ucp_) {`
`2257`	`2257`	`if (ucp_) {`
`2258`		`- PRE_READ(ucp_, ucontext_t_sz);`
	`2258`	`+ PRE_READ(ucp_, ucontext_t_sz(ucp_));`
`2259`	`2259`	`}`
`2260`	`2260`	`}`
`2261`	`2261`	`POST_SYSCALL(setcontext)(long long res, void *ucp_) {}`
`2262`	`2262`	`PRE_SYSCALL(_lwp_create)(void ucp_, long long flags_, void new_lwp_) {`
`2263`	`2263`	`if (ucp_) {`
`2264`		`- PRE_READ(ucp_, ucontext_t_sz);`
	`2264`	`+ PRE_READ(ucp_, ucontext_t_sz(ucp_));`
`2265`	`2265`	`}`
`2266`	`2266`	`}`
`2267`	`2267`	`POST_SYSCALL(_lwp_create)`