msan: account for AVX state when unpoison ucontext_t

[dvyukov]

Summary

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.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp

176

undef RELEASE?

215

Weird and unrelated: entire file is #if SANITIZER_LINUX || SANITIZER_MAC, but we have a lot of SANITIZER_FREEBSD and SANITIZER_NETBSD

226

what is about SANITIZER_MAC?

[dvyukov]

fixed issues pointed by comments and rebased

[dvyukov]

PTAL

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp

176

Added explanatory comment.

226

Added if defined(SANITIZER_LINUX)

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp

176

Oh, we should rename our sanitizer_thread_safety, those names are extremely common.
e.g. SANITIZER_ prefix

224

SANITIZER_X64

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h

65

probably relevant only for msvc which should not use this file
so maybe SANITIZER_X64?

compiler-rt/test/msan/Linux/signal_mcontext.cpp

13

SANITIZER_X64

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp

176

I will send a follow up fix.

224

Done, but somehow that's not what we use in the codebase:

$ git grep "if.*SANITIZER_X64" | wc -l
1
$ git grep "if.*defined(x86_64)" | wc -l
123

compiler-rt/test/msan/Linux/signal_mcontext.cpp

13

It does not seem that we define these macros for tests anywhere. At least none of the SANITIZER_* macros are used in tests so far.

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp

176

Sent https://reviews.llvm.org/D116652

[paulkirth]

Hi, we're seeing breakages building LLVM for Fuchsia, that appear to be caused by https://reviews.llvm.org/D116208 when building compiler-rt.

The error message is as follows:

/b/s/w/ir/x/w/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp:227:17: error: no member named '__glibc_reserved1' in '_libc_fpstate'
    if (fpregs->__glibc_reserved1[12] == FP_XSTATE_MAGIC1)
        ~~~~~~  ^
/b/s/w/ir/x/w/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp:229:22: error: no member named '__glibc_reserved1' in '_libc_fpstate'
             fpregs->__glibc_reserved1[13] - static_cast<const char *>(ctx);
             ~~~~~~  ^

I've filled and issue on github. https://github.com/llvm/llvm-project/issues/53014

[paulkirth]

It appears this code assumes glibc >= 2.26.

@mcgrathr suggested this as a potential fix:

--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp      
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp      
@@ -224,9 +224,13 @@ namespace __sanitizer {                                    
 #    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)                     
+    // The member names differ across header versions, but the actual layout   
+    // is always the same.  So avoid using members, just use arithmetic.       
+    const uint32_t *after_xmm =                                                
+        reinterpret_cast<const uint32_t*>(fpregs + 1) - 24;                    
+    if (after_xmm[12] == FP_XSTATE_MAGIC1)                                     
       return reinterpret_cast<const char *>(fpregs) +                          
-             fpregs->__glibc_reserved1[13] - static_cast<const char *>(ctx);   
+             after_xmm[13] - static_cast<const char *>(ctx);                   
 #    endif                                                                     
     return sizeof(ucontext_t);                                                 
   }

[paulkirth]

We've applied that patch to work around this in https://reviews.llvm.org/D116695

[dvyukov]

Hi Paul,

Just got back from holidays. I see you already addressed the problem in D116695. Thanks for that. And sorry for the breakage.

[paulkirth]

It was no problem. Roland had the patch immediately when I asked him about
it, and it seemed easier than reverting three commits.

compiler-rt/lib/msan/msan_interceptors.cpp

static void SignalHandler(int signo) {
   cb(signo);
 }
 
 static void SignalAction(int signo, void *si, void *uc) {
   SignalHandlerScope signal_handler_scope;
   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 =
       (sigaction_cb)atomic_load(&sigactions[signo], memory_order_relaxed);
   cb(signo, si, uc);
 }
 
 static void read_sigaction(const __sanitizer_sigaction *act) {

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_freebsd.h

 extern unsigned struct_tms_sz;
 extern unsigned struct_itimerspec_sz;
 extern unsigned struct_sigevent_sz;
 extern unsigned struct_stack_t_sz;
 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;
 extern unsigned struct_regmatch_sz;
 extern unsigned struct_regex_sz;
 extern unsigned struct_FTS_sz;
 extern unsigned struct_FTSENT_sz;
 extern const int unvis_valid;

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_freebsd.cpp

 unsigned mbstate_t_sz = sizeof(mbstate_t);
 unsigned sigset_t_sz = sizeof(sigset_t);
 unsigned struct_timezone_sz = sizeof(struct timezone);
 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_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);
 unsigned struct_itimerspec_sz = sizeof(struct itimerspec);
 unsigned struct_timeb_sz = sizeof(struct timeb);
 unsigned struct_msqid_ds_sz = sizeof(struct msqid_ds);
 unsigned struct_mq_attr_sz = sizeof(struct mq_attr);
 unsigned struct_statvfs_sz = sizeof(struct statvfs);

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_netbsd.h

 extern unsigned struct_timezone_sz;
 extern unsigned struct_tms_sz;
 extern unsigned struct_itimerspec_sz;
 extern unsigned struct_sigevent_sz;
 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;
 extern unsigned struct_timespec_sz;
 extern unsigned struct_sembuf_sz;
 
 extern unsigned struct_kevent_sz;
 extern unsigned struct_FTS_sz;

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_netbsd.cpp

 unsigned gid_t_sz = sizeof(gid_t);
 unsigned mbstate_t_sz = sizeof(mbstate_t);
 unsigned sigset_t_sz = sizeof(sigset_t);
 unsigned struct_timezone_sz = sizeof(struct timezone);
 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);
 unsigned struct_kevent_sz = sizeof(struct kevent);
 unsigned struct_FTS_sz = sizeof(FTS);
 unsigned struct_FTSENT_sz = sizeof(FTSENT);
 unsigned struct_regex_sz = sizeof(regex_t);
 unsigned struct_regmatch_sz = sizeof(regmatch_t);

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h

 extern unsigned struct_statfs64_sz;
 extern unsigned struct_regex_sz;
 extern unsigned struct_regmatch_sz;
 
 #if !SANITIZER_ANDROID
 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__)
 const unsigned struct_kernel_stat_sz = 64;
 const unsigned struct_kernel_stat64_sz = 96;
 #elif defined(__arm__)
 const unsigned struct_kernel_stat_sz = 64;
 const unsigned struct_kernel_stat64_sz = 104;

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp

 #include <sys/filio.h>
 #include <sys/sockio.h>
 #endif
 
 // Include these after system headers to avoid name clashes and ambiguities.
 #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);
 #if !SANITIZER_IOS && !(SANITIZER_MAC && TARGET_CPU_ARM64)
   unsigned struct_stat64_sz = sizeof(struct stat64);
 #endif // !SANITIZER_IOS && !(SANITIZER_MAC && TARGET_CPU_ARM64)
   unsigned struct_rusage_sz = sizeof(struct rusage);
   unsigned struct_tm_sz = sizeof(struct tm);
#endif // !SANITIZER_IOS && !(SANITIZER_MAC && TARGET_CPU_ARM64)
   unsigned struct_sched_param_sz = sizeof(struct sched_param);
   unsigned struct_regex_sz = sizeof(regex_t);
   unsigned struct_regmatch_sz = sizeof(regmatch_t);
 
 #if (SANITIZER_MAC && !TARGET_CPU_ARM64) && !SANITIZER_IOS
   unsigned struct_statfs64_sz = sizeof(struct statfs64);
 #endif // (SANITIZER_MAC && !TARGET_CPU_ARM64) && !SANITIZER_IOS
 
 #if SANITIZER_GLIBC || SANITIZER_FREEBSD || SANITIZER_NETBSD || SANITIZER_MAC
   unsigned struct_fstab_sz = sizeof(struct fstab);
 #endif  // SANITIZER_GLIBC || SANITIZER_FREEBSD || SANITIZER_NETBSD ||
         // SANITIZER_MAC
 #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
+
+  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
+#  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 =
       sizeof(struct __user_cap_header_struct);
   unsigned __user_cap_data_struct_sz = sizeof(struct __user_cap_data_struct);
   unsigned struct_new_utsname_sz = sizeof(struct new_utsname);
   unsigned struct_old_utsname_sz = sizeof(struct old_utsname);
   unsigned struct_oldold_utsname_sz = sizeof(struct oldold_utsname);

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_solaris.h

 extern unsigned struct_tms_sz;
 extern unsigned struct_itimerspec_sz;
 extern unsigned struct_sigevent_sz;
 extern unsigned struct_stack_t_sz;
 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;
 extern unsigned struct_utimbuf_sz;
 
 struct __sanitizer_sem_t {
   //u64 data[6];
   u32 sem_count;

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_solaris.cpp

namespace __sanitizer {
   unsigned mbstate_t_sz = sizeof(mbstate_t);
   unsigned sigset_t_sz = sizeof(sigset_t);
   unsigned struct_timezone_sz = sizeof(struct timezone);
   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_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);
 #endif
   unsigned struct_statvfs_sz = sizeof(struct statvfs);
 
   const uptr sig_ign = (uptr)SIG_IGN;
   const uptr sig_dfl = (uptr)SIG_DFL;

compiler-rt/lib/sanitizer_common/sanitizer_syscalls_netbsd.inc

POST_SYSCALL(utrace)(long long res, void *label_, void *addr_, long long len_) {
   if (addr_) {
     POST_READ(addr_, len_);
   }
 }
 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)
 (long long res, void *ucp_, long long flags_, void *new_lwp_) {}
 PRE_SYSCALL(_lwp_exit)(void) { /* Nothing to do */ }
 POST_SYSCALL(_lwp_exit)(long long res) { /* Nothing to do */ }
 PRE_SYSCALL(_lwp_self)(void) { /* Nothing to do */ }
 POST_SYSCALL(_lwp_self)(long long res) { /* Nothing to do */ }

compiler-rt/test/msan/Linux/signal_mcontext.cpp

+// 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: