[dfsan] Clean TLS after sigaction callbacks

[stephan.yichao.zhao]

Summary

DFSan uses TLS to pass metadata of arguments and return values. When an
instrumented function accesses the TLS, if a signal callback happens, and
the callback calls other instrumented functions with updating the same TLS,
the TLS is in an inconsistent state after the callback ends. This may cause
either under-tainting or over-tainting.

This fix follows MSan's workaround.
  https://github.com/llvm/llvm-project/commit/cb22c67a21e4b5e1ade65141117a70be318be072
It simply resets TLS at restore. This prevents from over-tainting. Although
under-tainting may still happen, a taint flow can be found eventually if we
run a DFSan-instrumented program multiple times. The alternative option is
saving the entire TLS. However the TLS storage takes 2k bytes, and signal calls
could be nested. So it does not seem worth.

This diff fixes sigaction. A following diff will be fixing signal.

[stephan.yichao.zhao]

@browneee, @gbalats, FYI

[morehouse]

compiler-rt/lib/dfsan/dfsan.h

79

96

RAII seems like overkill since Backup() does nothing and we don't have any early returns in the functions we use it from. How about a simple function call instead?

compiler-rt/lib/dfsan/dfsan_custom.cpp

840

853

Can these params actually be labeled? Usually labels come in via TLS or extra args. (But I am not sure how signals work with DFSan)

868

sizeof(*si)?

899–932

I think we need to block signals before taking this lock. sigaction needs to be async-signal safe.

906

Any reason not to simply use SA_SIGINFO (and other standard SIG_IGN, SIG_DFL constants below)?

910

Is this cast necessary?

916

Is this cast necessary?

927

I think we need logic here to differentiate sa_sigaction and sa_handler instead of blindly casting to sa_sigaction.

compiler-rt/test/dfsan/sigaction.c

9

Is it worth testing legacy mode at all? I think the relevant code paths are the same.

33

38

Can remove this line if we do psa = {}; above.

41

We should also test that the correct oldact is returned.

compiler-rt/test/dfsan/sigaction_stress_test.c

3

Is it worth testing legacy mode at all? I think the relevant code paths are the same.

41–42

[browneee]

compiler-rt/lib/dfsan/dfsan.h

78

Typo: headlers -> handlers

The typo morehouse raised also appears in change description: accesse -> accesses

[stephan.yichao.zhao]

compiler-rt/lib/dfsan/dfsan_custom.cpp

853

Thank you for catching this. Right. This works only for *si and *uc. For others, their TLS need to be set. Will be changing.

899–932

Yeah. Such a case seems possible. Does blocking signals work like this?

sigset_t original;                                                         
sigset_t blocked;                                                          
sigemptyset(&blocked);                                           // is it fine to block all signals or needs to allow some, for example, SIGPROF and SIGALRM?  
sigprocmask(SIG_BLOCK, &blocked, &original);                              
{ lock, ///
}
sigprocmask(SIG_SETMASK, &original, nullptr);

MSan's code is likely to have the similar issue. @eugenis: Does MSan's case need a similar update?

[morehouse]

compiler-rt/lib/dfsan/dfsan_custom.cpp

899–932

s/sigemptyset/sigfillset

I would also recommend creating an RAII SignalSpinMutexLock that blocks signals prior to locking.

[stephan.yichao.zhao]

addressed comments

[stephan.yichao.zhao]

addressed comments

compiler-rt/lib/dfsan/dfsan.h

78

Thanks

96

I feel this one is more about making the code easy to read, and in case we need to make SignalAction and SignalHandler with multiple return points. it still works.

compiler-rt/lib/dfsan/dfsan_custom.cpp

840

rephrased after using a RAII async-safe lock.

899–932

introduced SignalSpinLocker. Named it as SignalSpinLocker but not SignalSpinMutexLock because mutex is moved to the class's private static member, and after that the class looks like a lock.

906

Using them avoids casting from handle_t to uptr. But since this function does not use __sanitizer::* consistently. replaced.

910

removed. they were requred if pnew_act was declared as __sanitizer::action_type.

927

Thank you for catching this. It worked before because the undefined behavior in linux: both sa_sigaction and sa_handler are defined as a union, and at runtime I guess linux always applies the 3 arguments that sa_sigaction needs, so it works for both sa_sigaction and sa_handler because sa_handler can safely use the first argument...

compiler-rt/test/dfsan/sigaction.c

41

This test case is for testing callbacks work by SIGHUP since that custom.cpp test cannot be kiiled.
I added some test cases about oldact in custom.cpp.

[morehouse]

compiler-rt/lib/dfsan/dfsan.h

90

Since this is only used in dfsan_custom.cpp, should we move it there?

96

Then let's remove Backup since it's unused. And also rename it to something more accurate, e.g. ClearThreadLocalState

compiler-rt/lib/dfsan/dfsan_custom.cpp

839

I don't see any atomics in the implementation.

885–887

925

I think these uptr casts are unnecessary.

compiler-rt/test/dfsan/custom.cpp

817–818

[stephan.yichao.zhao]

updated based on comments.

[stephan.yichao.zhao]

formatted.

[stephan.yichao.zhao]

compiler-rt/lib/dfsan/dfsan.h

90

defined dfsan_clear_thread_local_state in dfsan.h, and moved ScopedThreadLocalStateBackup to dfsan_custom.cpp.

96

renamed to ScopedClearThreadLocalState and moved to dfsan_custom.cpp.

compiler-rt/lib/dfsan/dfsan_custom.cpp

885–887

Thanks.

[morehouse]

LGTM

compiler-rt/lib/dfsan/dfsan.h

struct dfsan_label_info {
   void *userdata;
 };
 
 extern "C" {
 void dfsan_add_label(dfsan_label label, void *addr, uptr size);
 void dfsan_set_label(dfsan_label label, void *addr, uptr size);
 dfsan_label dfsan_read_label(const void *addr, uptr size);
 dfsan_label dfsan_union(dfsan_label l1, dfsan_label l2);
+// Zero out [offset, offset+size) from __dfsan_arg_tls.
+void dfsan_clear_arg_tls(uptr offset, uptr size);
+// Zero out the TLS storage.
+void dfsan_clear_thread_local_state();
 }  // extern "C"
 
 template <typename T>
 void dfsan_set_label(dfsan_label label, T &data) {  // NOLINT
   dfsan_set_label(label, (void *)&data, sizeof(T));
 }
 
 namespace __dfsan {

 
 extern Flags flags_data;
 inline Flags &flags() {
   return flags_data;
 }
 
 }  // namespace __dfsan
 
 #endif  // DFSAN_H

compiler-rt/lib/dfsan/dfsan.cpp

static void InitializeFlags() {
   RegisterCommonFlags(&parser);
   RegisterDfsanFlags(&parser, &flags());
   parser.ParseStringFromEnv("DFSAN_OPTIONS");
   InitializeCommonFlags();
   if (Verbosity()) ReportUnrecognizedFlags();
   if (common_flags()->help) parser.PrintFlagDescriptions();
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
+void dfsan_clear_arg_tls(uptr offset, uptr size) {
+  internal_memset((void *)((uptr)__dfsan_arg_tls + offset), 0, size);
+}
+
+SANITIZER_INTERFACE_ATTRIBUTE
+void dfsan_clear_thread_local_state() {
+  internal_memset(__dfsan_arg_tls, 0, sizeof(__dfsan_arg_tls));
+  internal_memset(__dfsan_retval_tls, 0, sizeof(__dfsan_retval_tls));
+}
+
 static void InitializePlatformEarly() {
   AvoidCVE_2016_2143();
 #ifdef DFSAN_RUNTIME_VMA
   __dfsan::vmaSize =
     (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1);
   if (__dfsan::vmaSize == 39 || __dfsan::vmaSize == 42 ||
       __dfsan::vmaSize == 48) {
     __dfsan_shadow_ptr_mask = ShadowMask();

compiler-rt/lib/dfsan/dfsan_custom.cpp

 #define CALL_WEAK_INTERCEPTOR_HOOK(f, ...)                                     \
   do {                                                                         \
     if (f)                                                                     \
       f(__VA_ARGS__);                                                          \
   } while (false)
 #define DECLARE_WEAK_INTERCEPTOR_HOOK(f, ...) \
 SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE void f(__VA_ARGS__);
 
+// Async-safe, non-reentrant spin lock.
+class SignalSpinLocker {
+ public:
+  SignalSpinLocker() {
+    sigset_t all_set;
+    sigfillset(&all_set);
+    pthread_sigmask(SIG_SETMASK, &all_set, &saved_thread_mask_);
+    sigactions_mu.Lock();
+  }
+  ~SignalSpinLocker() {
+    sigactions_mu.Unlock();
+    pthread_sigmask(SIG_SETMASK, &saved_thread_mask_, nullptr);
+  }
+
+ private:
+  static StaticSpinMutex sigactions_mu;
+  sigset_t saved_thread_mask_;
+
+  SignalSpinLocker(const SignalSpinLocker &) = delete;
+  SignalSpinLocker &operator=(const SignalSpinLocker &) = delete;
+};
+
+StaticSpinMutex SignalSpinLocker::sigactions_mu;
+
 extern "C" {
 SANITIZER_INTERFACE_ATTRIBUTE int
 __dfsw_stat(const char *path, struct stat *buf, dfsan_label path_label,
             dfsan_label buf_label, dfsan_label *ret_label) {
   int ret = stat(path, buf);
   if (ret == 0)
     dfsan_set_label(0, buf, sizeof(struct stat));
   *ret_label = 0;

 SANITIZER_INTERFACE_ATTRIBUTE
 int __dfsw_sigemptyset(sigset_t *set, dfsan_label set_label,
                        dfsan_label *ret_label) {
   int ret = sigemptyset(set);
   dfsan_set_label(0, set, sizeof(sigset_t));
   return ret;
 }
 
+// Clear DFSan runtime TLS state at the end of a scope.
+//
+// Implementation must be async-signal-safe and use small data size, because
+// instances of this class may live on the signal handler stack.
+//
+// DFSan uses TLS to pass metadata of arguments and return values. When an
+// instrumented function accesses the TLS, if a signal callback happens, and the
+// callback calls other instrumented functions with updating the same TLS, the
+// TLS is in an inconsistent state after the callback ends. This may cause
+// either under-tainting or over-tainting.
+//
+// The current implementation simply resets TLS at restore. This prevents from
+// over-tainting. Although under-tainting may still happen, a taint flow can be
+// found eventually if we run a DFSan-instrumented program multiple times. The
+// alternative option is saving the entire TLS. However the TLS storage takes
+// 2k bytes, and signal calls could be nested. So it does not seem worth.
+class ScopedClearThreadLocalState {
+ public:
+  ScopedClearThreadLocalState() {}
+  ~ScopedClearThreadLocalState() { dfsan_clear_thread_local_state(); }
+};
+
+// SignalSpinLocker::sigactions_mu guarantees atomicity of sigaction() calls.
+const int kMaxSignals = 1024;
+static atomic_uintptr_t sigactions[kMaxSignals];
+
+static void SignalHandler(int signo) {
+  ScopedClearThreadLocalState stlsb;
+
+  // Clear shadows for all inputs provided by system. This is why DFSan
+  // instrumentation generates a trampoline function to each function pointer,
+  // and uses the trampoline to clear shadows. However sigaction does not use
+  // a function pointer directly, so we have to do this manually.
+  dfsan_clear_arg_tls(0, sizeof(dfsan_label));
+
+  typedef void (*signal_cb)(int x);
+  signal_cb cb =
+      (signal_cb)atomic_load(&sigactions[signo], memory_order_relaxed);
+  cb(signo);
+}
+
+static void SignalAction(int signo, siginfo_t *si, void *uc) {
+  ScopedClearThreadLocalState stlsb;
+
+  // Clear shadows for all inputs provided by system. Similar to SignalHandler.
+  dfsan_clear_arg_tls(0, 3 * sizeof(dfsan_label));
+  dfsan_set_label(0, si, sizeof(*si));
+  dfsan_set_label(0, uc, sizeof(ucontext_t));
+
+  typedef void (*sigaction_cb)(int, siginfo_t *, void *);
+  sigaction_cb cb =
+      (sigaction_cb)atomic_load(&sigactions[signo], memory_order_relaxed);
+  cb(signo, si, uc);
+}
+
 SANITIZER_INTERFACE_ATTRIBUTE
 int __dfsw_sigaction(int signum, const struct sigaction *act,
                      struct sigaction *oldact, dfsan_label signum_label,
                      dfsan_label act_label, dfsan_label oldact_label,
                      dfsan_label *ret_label) {
-  int ret = sigaction(signum, act, oldact);
+  CHECK_LT(signum, kMaxSignals);
+  SignalSpinLocker lock;
+  uptr old_cb = atomic_load(&sigactions[signum], memory_order_relaxed);
+  struct sigaction new_act;
+  struct sigaction *pnew_act = act ? &new_act : nullptr;
+  if (act) {
+    internal_memcpy(pnew_act, act, sizeof(struct sigaction));
+    if (pnew_act->sa_flags & SA_SIGINFO) {
+      uptr cb = (uptr)(pnew_act->sa_sigaction);
+      if (cb != (uptr)SIG_IGN && cb != (uptr)SIG_DFL) {
+        atomic_store(&sigactions[signum], cb, memory_order_relaxed);
+        pnew_act->sa_sigaction = SignalAction;
+      }
+    } else {
+      uptr cb = (uptr)(pnew_act->sa_handler);
+      if (cb != (uptr)SIG_IGN && cb != (uptr)SIG_DFL) {
+        atomic_store(&sigactions[signum], cb, memory_order_relaxed);
+        pnew_act->sa_handler = SignalHandler;
+      }
+    }
+  }
+
+  int ret = sigaction(signum, pnew_act, oldact);
+
+  if (ret == 0 && oldact) {
+    if (oldact->sa_flags & SA_SIGINFO) {
+      if (oldact->sa_sigaction == SignalAction)
+        oldact->sa_sigaction = (decltype(oldact->sa_sigaction))old_cb;
+    } else {
+      if (oldact->sa_handler == SignalHandler)
+        oldact->sa_handler = (decltype(oldact->sa_handler))old_cb;
+    }
+  }
+
   if (oldact) {
     dfsan_set_label(0, oldact, sizeof(struct sigaction));
   }
   *ret_label = 0;
   return ret;
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE

compiler-rt/test/dfsan/custom.cpp

 void test_sigemptyset() {
   sigset_t set;
   dfsan_set_label(j_label, &set, 1);
   int ret = sigemptyset(&set);
   assert(ret == 0);
   ASSERT_READ_ZERO_LABEL(&set, sizeof(set));
 }
 
+static void SignalHandler(int signo) {}
+
+static void SignalAction(int signo, siginfo_t *si, void *uc) {}
+
 void test_sigaction() {
-  struct sigaction oldact;
-  dfsan_set_label(j_label, &oldact, 1);
-  int ret = sigaction(SIGUSR1, NULL, &oldact);
+  struct sigaction newact_with_sigaction = {};
+  newact_with_sigaction.sa_flags = SA_SIGINFO;
+  newact_with_sigaction.sa_sigaction = SignalAction;
+
+  // Set sigaction to be SignalAction, save the last one into origin_act
+  struct sigaction origin_act;
+  dfsan_set_label(j_label, &origin_act, 1);
+  int ret = sigaction(SIGUSR1, &newact_with_sigaction, &origin_act);
   assert(ret == 0);
-  ASSERT_READ_ZERO_LABEL(&oldact, sizeof(oldact));
+  ASSERT_ZERO_LABEL(ret);
+  ASSERT_READ_ZERO_LABEL(&origin_act, sizeof(origin_act));
+
+  struct sigaction newact_with_sighandler = {};
+  newact_with_sighandler.sa_handler = SignalHandler;
+
+  // Set sigaction to be SignalHandler, check the last one is SignalAction
+  struct sigaction oldact;
+  assert(0 == sigaction(SIGUSR1, &newact_with_sighandler, &oldact));
+  assert(oldact.sa_sigaction == SignalAction);
+
+  // Restore sigaction to the orginal setting, check the last one is SignalHandler
+  assert(0 == sigaction(SIGUSR1, &origin_act, &oldact));
+  assert(oldact.sa_handler == SignalHandler);
 }
 
 void test_sigaltstack() {
   stack_t old_altstack = {};
   dfsan_set_label(j_label, &old_altstack, sizeof(old_altstack));
   int ret = sigaltstack(NULL, &old_altstack);
   assert(ret == 0);
   ASSERT_ZERO_LABEL(ret);

compiler-rt/test/dfsan/sigaction.c

+// RUN: %clang_dfsan -DUSE_SIGNAL_ACTION -mllvm -dfsan-fast-16-labels=true %s -o %t && \
+// RUN:     %run %t
+// RUN: %clang_dfsan -mllvm -dfsan-fast-16-labels=true %s -o %t && %run %t
+
+#include <sanitizer/dfsan_interface.h>
+
+#include <assert.h>
+#include <signal.h>
+#include <string.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+volatile int x;
+volatile int z = 1;
+
+void SignalHandler(int signo) {
+  assert(dfsan_get_label(signo) == 0);
+  x = z;
+}
+
+void SignalAction(int signo, siginfo_t *si, void *uc) {
+  assert(dfsan_get_label(signo) == 0);
+  assert(dfsan_get_label(si) == 0);
+  assert(dfsan_get_label(uc) == 0);
+  assert(0 == dfsan_read_label(si, sizeof(*si)));
+  assert(0 == dfsan_read_label(uc, sizeof(ucontext_t)));
+  x = z;
+}
+
+int main(int argc, char *argv[]) {
+  dfsan_set_label(8, (void *)&z, sizeof(z));
+
+  struct sigaction sa = {};
+#ifdef USE_SIGNAL_ACTION
+  sa.sa_flags = SA_SIGINFO;
+  sa.sa_sigaction = SignalAction;
+#else
+  sa.sa_handler = SignalHandler;
+#endif
+  int r = sigaction(SIGHUP, &sa, NULL);
+  assert(dfsan_get_label(r) == 0);
+
+  kill(getpid(), SIGHUP);
+  signal(SIGHUP, SIG_DFL);
+
+  assert(x == 1);
+
+  return 0;
+}

compiler-rt/test/dfsan/sigaction_stress_test.c

+// RUN: %clangxx_dfsan -mllvm -dfsan-fast-16-labels=true -O0 %s -o %t && %run %t
+//
+// Test that the state of shadows from a sigaction handler are consistent.
+
+#include <signal.h>
+#include <stdarg.h>
+#include <sanitizer/dfsan_interface.h>
+#include <assert.h>
+#include <sys/time.h>
+#include <stdio.h>
+
+const int kSigCnt = 200;
+int x;
+
+__attribute__((noinline))
+int f(int a) {
+  return a;
+}
+
+__attribute__((noinline))
+void g() {
+  int r = f(x);
+  const dfsan_label r_label = dfsan_get_label(r);
+  assert(r_label == 8 || r_label == 0);
+  return;
+}
+
+int sigcnt;
+
+void SignalHandler(int signo) {
+  assert(signo == SIGPROF);
+  int a = 0;
+  dfsan_set_label(4, &a, sizeof(a));
+  (void)f(a);
+  ++sigcnt;
+}
+
+int main() {
+  struct sigaction psa = {};
+  psa.sa_handler = SignalHandler;
+  int r = sigaction(SIGPROF, &psa, NULL);
+
+  itimerval itv;
+  itv.it_interval.tv_sec = 0;
+  itv.it_interval.tv_usec = 100;
+  itv.it_value.tv_sec = 0;
+  itv.it_value.tv_usec = 100;
+  setitimer(ITIMER_PROF, &itv, NULL);
+
+  dfsan_set_label(8, &x, sizeof(x));
+  do {
+    g();
+  } while (sigcnt < kSigCnt);
+
+  itv.it_interval.tv_sec = 0;
+  itv.it_interval.tv_usec = 0;
+  itv.it_value.tv_sec = 0;
+  itv.it_value.tv_usec = 0;
+  setitimer(ITIMER_PROF, &itv, NULL);
+
+  signal(SIGPROF, SIG_DFL);
+  return 0;
+}