This is an archive of the discontinued LLVM Phabricator instance.

Hardware-assisted AddressSanitizer (compiler-rt)
ClosedPublic

Authored by eugenis on Dec 6 2017, 5:23 PM.

Details

Summary

Runtime library for HWASan, initial commit.
Does not randomize tags yet, does not handle stack or globals.

Diff Detail

Event Timeline

eugenis created this revision.Dec 6 2017, 5:23 PM
krytarowski added inline comments.Dec 6 2017, 5:36 PM
compiler-rt/lib/hwasan/hwasan_linux.cc
86 ↗(On Diff #125857)

I have a local draft MSan patch adjusting similar code for NetBSD. It's right no incomplete as I need to add proper definition of INTERCEPT_FUNCTION(_lwp_exit); instead of calling it for each thread.

diff --git a/lib/msan/msan_linux.cc b/lib/msan/msan_linux.cc
index 4e6321fcb..d68509672 100644
--- a/lib/msan/msan_linux.cc
+++ b/lib/msan/msan_linux.cc
@@ -30,6 +30,7 @@
 #include <sys/time.h>
 #include <sys/resource.h>
 
+#include "interception/interception.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_procmaps.h"
 
@@ -174,13 +175,28 @@ void InstallAtExitHandler() {
 
 // ---------------------- TSD ---------------- {{{1
 
+#if !SANITIZER_NETBSD
 static pthread_key_t tsd_key;
+#endif
+
 static bool tsd_key_inited = false;
 
+#if SANITIZER_NETBSD
+INTERCEPTOR(void, _lwp_exit) {
+  CHECK(tsd_key_inited);
+  MsanTSDDtor(GetCurrentThread());
+  REAL(_lwp_exit)();
+}
+#endif
+
 void MsanTSDInit(void (*destructor)(void *tsd)) {
   CHECK(!tsd_key_inited);
   tsd_key_inited = true;
+#if SANITIZER_NETBSD
+  INTERCEPT_FUNCTION(_lwp_exit);
+#else
   CHECK_EQ(0, pthread_key_create(&tsd_key, destructor));
+#endif
 }
 
 static THREADLOCAL MsanThread* msan_current_thread;
@@ -195,16 +211,20 @@ void SetCurrentThread(MsanThread *t) {
   msan_current_thread = t;
   // Make sure that MsanTSDDtor gets called at the end.
   CHECK(tsd_key_inited);
+#if !SANITIZER_NETBSD
   pthread_setspecific(tsd_key, (void *)t);
+#endif
 }
 
 void MsanTSDDtor(void *tsd) {
+#if !SANITIZER_NETBSD
   MsanThread *t = (MsanThread*)tsd;
   if (t->destructor_iterations_ > 1) {
     t->destructor_iterations_--;
     CHECK_EQ(0, pthread_setspecific(tsd_key, tsd));
     return;
   }
+#endif
   msan_current_thread = nullptr;
   // Make sure that signal handler can not see a stale current thread pointer.
   atomic_signal_fence(memory_order_seq_cst);

Note that hwasan stores current thread pointer in the TSD slot, because it needs to run on Android where thread-local (i.e. __thread) variables don't really work.
We plan to do the same with MSan in the future, but I don't know when.

kcc added inline comments.Dec 6 2017, 7:24 PM
compiler-rt/lib/hwasan/hwasan_interceptors.cc
137 ↗(On Diff #125857)

Do we expect any of this code to (ever? any time soon?) work on FREEBSD? NETBSD?
Do those OSes exist for aarch64?

eugenis added inline comments.Dec 7 2017, 4:00 PM
compiler-rt/lib/hwasan/hwasan_interceptors.cc
137 ↗(On Diff #125857)

At least FreeBSD does.

krytarowski added inline comments.Dec 7 2017, 4:03 PM
compiler-rt/lib/hwasan/hwasan_interceptors.cc
137 ↗(On Diff #125857)
eugenis updated this revision to Diff 126072.Dec 7 2017, 4:09 PM

switch to 1-to-16 shadow

kcc edited edge metadata.Dec 8 2017, 10:37 AM

My top level comment: can we delete all non-aarch64 code?
The arch owners can reinstate it if needed, but they will only need it if/when they have the TBI feature in HW.

compiler-rt/lib/hwasan/hwasan.cc
60 ↗(On Diff #126072)

seems to be unused (nothings sets it to non zero), please remove.

99 ↗(On Diff #126072)

can we not have any of these #ifdefs?

181 ↗(On Diff #126072)

Is this one relevant?

194 ↗(On Diff #126072)

do we need this?
For asan we did, but probably not for hwasan

compiler-rt/lib/hwasan/hwasan_allocator.cc
72 ↗(On Diff #126072)

oy! Please remove all of this non-aarch64 stuff.
We won't need non-aarch64 any time soon.

compiler-rt/lib/hwasan/hwasan_flags.inc
24 ↗(On Diff #126072)

seems unused, remove

compiler-rt/lib/hwasan/hwasan_interceptors.cc
478 ↗(On Diff #126072)

mips?

compiler-rt/lib/hwasan/hwasan_linux.cc
38 ↗(On Diff #126072)

Note for future, feel free to ignore now: we should try to have the zero shadow base

106 ↗(On Diff #126072)

What's the use case for #else?

eugenis updated this revision to Diff 126213.Dec 8 2017, 2:39 PM
eugenis marked 4 inline comments as done.

Addressed review comments.
Implemented halt_on_error=1.
Fixed tests to pass on unmodified kernels (see disable_allocator_tagging flag).

eugenis added inline comments.Dec 8 2017, 2:39 PM
compiler-rt/lib/hwasan/hwasan.cc
60 ↗(On Diff #126072)

I've implemented halt_on_error=0

99 ↗(On Diff #126072)

don't we want hwasan + ubsan and hwasan + lsan?

194 ↗(On Diff #126072)

we don't

alekseyshl accepted this revision.Dec 8 2017, 3:45 PM
This revision is now accepted and ready to land.Dec 8 2017, 3:45 PM
kcc accepted this revision.Dec 8 2017, 3:58 PM

LGTM, let's iterate from here.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.