[compiler-rt][hwasan] Add fiber switch for HwASan #153822
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
compiler-rt
compiler-rt:hwasan
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Tomahawkd (Tomahawkd) ChangesCurrently HwASan has no fiber switch interface for coroutines. This PR adds fiber switch interfaces similar to ASan which helps to pass sp check correctly on unwinding. The only difference is HwASan does not need a fake stack since tags can do the same thing (e.g., detect UAR). Fake stack parameters (named Also adds unit test which is similar to ASan with minor adjustment:
The testcase is tested on x86 with alias mode enabled. Full diff: https://github.com/llvm/llvm-project/pull/153822.diff 4 Files Affected:
diff --git a/compiler-rt/lib/hwasan/hwasan_interface_internal.h b/compiler-rt/lib/hwasan/hwasan_interface_internal.h
index 8f2f77dad917d..2c79f0bc8cb99 100644
--- a/compiler-rt/lib/hwasan/hwasan_interface_internal.h
+++ b/compiler-rt/lib/hwasan/hwasan_interface_internal.h
@@ -247,6 +247,14 @@ void *__hwasan_memmove_match_all(void *dest, const void *src, uptr n, u8);
SANITIZER_INTERFACE_ATTRIBUTE
void __hwasan_set_error_report_callback(void (*callback)(const char *));
+
+// hwasan does not need fake stack, so we leave it unused here.
+SANITIZER_INTERFACE_ATTRIBUTE
+void __sanitizer_start_switch_fiber(void **unused, const void *bottom,
+ uptr size);
+SANITIZER_INTERFACE_ATTRIBUTE
+void __sanitizer_finish_switch_fiber(void *unused, const void **bottom_old,
+ uptr *size_old);
} // extern "C"
#endif // HWASAN_INTERFACE_INTERNAL_H
diff --git a/compiler-rt/lib/hwasan/hwasan_thread.cpp b/compiler-rt/lib/hwasan/hwasan_thread.cpp
index 5c07522d42796..4facb1eeb2e73 100644
--- a/compiler-rt/lib/hwasan/hwasan_thread.cpp
+++ b/compiler-rt/lib/hwasan/hwasan_thread.cpp
@@ -119,6 +119,60 @@ void Thread::Destroy() {
*GetCurrentThreadLongPtr() = 0;
}
+void Thread::StartSwitchFiber(uptr bottom, uptr size) {
+ if (atomic_load(&stack_switching_, memory_order_relaxed)) {
+ Report("ERROR: starting fiber switch while in fiber switch\n");
+ Die();
+ }
+
+ next_stack_bottom_ = bottom;
+ next_stack_top_ = bottom + size;
+ atomic_store(&stack_switching_, 1, memory_order_release);
+}
+
+void Thread::FinishSwitchFiber(uptr *bottom_old, uptr *size_old) {
+ if (!atomic_load(&stack_switching_, memory_order_relaxed)) {
+ Report("ERROR: finishing a fiber switch that has not started\n");
+ Die();
+ }
+
+ if (bottom_old)
+ *bottom_old = stack_bottom_;
+ if (size_old)
+ *size_old = stack_top_ - stack_bottom_;
+ stack_bottom_ = next_stack_bottom_;
+ stack_top_ = next_stack_top_;
+ atomic_store(&stack_switching_, 0, memory_order_release);
+ next_stack_top_ = 0;
+ next_stack_bottom_ = 0;
+}
+
+inline Thread::StackBounds Thread::GetStackBounds() const {
+ if (!atomic_load(&stack_switching_, memory_order_acquire)) {
+ // Make sure the stack bounds are fully initialized.
+ if (stack_bottom_ >= stack_top_)
+ return {0, 0};
+ return {stack_bottom_, stack_top_};
+ }
+ char local;
+ const uptr cur_stack = (uptr)&local;
+ // Note: need to check next stack first, because FinishSwitchFiber
+ // may be in process of overwriting stack_top_/bottom_. But in such case
+ // we are already on the next stack.
+ if (cur_stack >= next_stack_bottom_ && cur_stack < next_stack_top_)
+ return {next_stack_bottom_, next_stack_top_};
+ return {stack_bottom_, stack_top_};
+}
+
+uptr Thread::stack_top() { return GetStackBounds().top; }
+
+uptr Thread::stack_bottom() { return GetStackBounds().bottom; }
+
+uptr Thread::stack_size() {
+ const auto bounds = GetStackBounds();
+ return bounds.top - bounds.bottom;
+}
+
void Thread::Print(const char *Prefix) {
Printf("%sT%zd %p stack: [%p,%p) sz: %zd tls: [%p,%p)\n", Prefix, unique_id_,
(void *)this, stack_bottom(), stack_top(),
@@ -226,3 +280,34 @@ void PrintThreads() {
}
} // namespace __lsan
+
+// ---------------------- Interface ---------------- {{{1
+using namespace __hwasan;
+
+extern "C" {
+SANITIZER_INTERFACE_ATTRIBUTE
+void __sanitizer_start_switch_fiber(void **unused, const void *bottom,
+ uptr size) {
+ // this is just a placeholder which make the interface same as ASan
+ (void)unused;
+ auto *t = GetCurrentThread();
+ if (!t) {
+ VReport(1, "__hwasan_start_switch_fiber called from unknown thread\n");
+ return;
+ }
+ t->StartSwitchFiber((uptr)bottom, size);
+}
+
+SANITIZER_INTERFACE_ATTRIBUTE
+void __sanitizer_finish_switch_fiber(void *unused, const void **bottom_old,
+ uptr *size_old) {
+ // this is just a placeholder which make the interface same as ASan
+ (void)unused;
+ auto *t = GetCurrentThread();
+ if (!t) {
+ VReport(1, "__hwasan_finish_switch_fiber called from unknown thread\n");
+ return;
+ }
+ t->FinishSwitchFiber((uptr *)bottom_old, size_old);
+}
+}
\ No newline at end of file
diff --git a/compiler-rt/lib/hwasan/hwasan_thread.h b/compiler-rt/lib/hwasan/hwasan_thread.h
index 62d6157f98b87..8ef282fd7b10f 100644
--- a/compiler-rt/lib/hwasan/hwasan_thread.h
+++ b/compiler-rt/lib/hwasan/hwasan_thread.h
@@ -41,9 +41,9 @@ class Thread {
void Destroy();
- uptr stack_top() { return stack_top_; }
- uptr stack_bottom() { return stack_bottom_; }
- uptr stack_size() { return stack_top() - stack_bottom(); }
+ uptr stack_top();
+ uptr stack_bottom();
+ uptr stack_size();
uptr tls_begin() { return tls_begin_; }
uptr tls_end() { return tls_end_; }
DTLS *dtls() { return dtls_; }
@@ -53,6 +53,9 @@ class Thread {
return addr >= stack_bottom_ && addr < stack_top_;
}
+ void StartSwitchFiber(uptr bottom, uptr size);
+ void FinishSwitchFiber(uptr *bottom_old, uptr *size_old);
+
AllocatorCache *allocator_cache() { return &allocator_cache_; }
HeapAllocationsRingBuffer *heap_allocations() { return heap_allocations_; }
StackAllocationsRingBuffer *stack_allocations() { return stack_allocations_; }
@@ -80,9 +83,22 @@ class Thread {
void ClearShadowForThreadStackAndTLS();
void Print(const char *prefix);
void InitRandomState();
+
+ struct StackBounds {
+ uptr bottom;
+ uptr top;
+ };
+ StackBounds GetStackBounds() const;
+
uptr vfork_spill_;
uptr stack_top_;
uptr stack_bottom_;
+ // these variables are used when the thread is about to switch stack
+ uptr next_stack_top_;
+ uptr next_stack_bottom_;
+ // true if switching is in progress
+ atomic_uint8_t stack_switching_;
+
uptr tls_begin_;
uptr tls_end_;
DTLS *dtls_;
diff --git a/compiler-rt/test/hwasan/TestCases/Linux/swapcontext_annotation.cpp b/compiler-rt/test/hwasan/TestCases/Linux/swapcontext_annotation.cpp
new file mode 100644
index 0000000000000..a2cea7717d0f9
--- /dev/null
+++ b/compiler-rt/test/hwasan/TestCases/Linux/swapcontext_annotation.cpp
@@ -0,0 +1,202 @@
+// Check that HwASan plays well with annotated makecontext/swapcontext.
+
+// RUN: %clangxx_hwasan -std=c++11 -lpthread -O0 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_hwasan -std=c++11 -lpthread -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_hwasan -std=c++11 -lpthread -O2 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_hwasan -std=c++11 -lpthread -O3 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: seq 60 | xargs -i -- grep LOOPCHECK %s > %t.checks
+// RUN: %clangxx_hwasan -std=c++11 -lpthread -O0 %s -o %t && %run %t 2>&1 | FileCheck %t.checks --check-prefix LOOPCHECK
+// RUN: %clangxx_hwasan -std=c++11 -lpthread -O1 %s -o %t && %run %t 2>&1 | FileCheck %t.checks --check-prefix LOOPCHECK
+// RUN: %clangxx_hwasan -std=c++11 -lpthread -O2 %s -o %t && %run %t 2>&1 | FileCheck %t.checks --check-prefix LOOPCHECK
+// RUN: %clangxx_hwasan -std=c++11 -lpthread -O3 %s -o %t && %run %t 2>&1 | FileCheck %t.checks --check-prefix LOOPCHECK
+
+//
+// This test is too subtle to try on non-x86 arch for now.
+// Android and musl do not support swapcontext.
+// REQUIRES: x86-target-arch && glibc-2.27
+
+#include <pthread.h>
+#include <setjmp.h>
+#include <signal.h>
+#include <stdio.h>
+#include <sys/time.h>
+#include <ucontext.h>
+#include <unistd.h>
+
+#include <sanitizer/common_interface_defs.h>
+
+ucontext_t orig_context;
+ucontext_t child_context;
+ucontext_t next_child_context;
+
+char *next_child_stack;
+
+const int kStackSize = 1 << 20;
+
+const void *main_thread_stack;
+size_t main_thread_stacksize;
+
+const void *from_stack;
+size_t from_stacksize;
+
+__attribute__((noinline, noreturn)) void LongJump(jmp_buf env) {
+ longjmp(env, 1);
+ _exit(1);
+}
+
+// Simulate __asan_handle_no_return().
+__attribute__((noinline)) void CallNoReturn() {
+ jmp_buf env;
+ if (setjmp(env) != 0)
+ return;
+
+ LongJump(env);
+ _exit(1);
+}
+
+void NextChild() {
+ CallNoReturn();
+ __sanitizer_finish_switch_fiber(nullptr, &from_stack, &from_stacksize);
+
+ printf("NextChild from: %p %zu\n", from_stack, from_stacksize);
+
+ char x[32] = {0}; // Stack gets poisoned.
+ printf("NextChild: %p\n", x);
+
+ CallNoReturn();
+
+ __sanitizer_start_switch_fiber(nullptr, main_thread_stack,
+ main_thread_stacksize);
+ CallNoReturn();
+ if (swapcontext(&next_child_context, &orig_context) < 0) {
+ perror("swapcontext");
+ _exit(1);
+ }
+}
+
+void Child(int mode) {
+ CallNoReturn();
+ __sanitizer_finish_switch_fiber(nullptr, &main_thread_stack,
+ &main_thread_stacksize);
+ char x[32] = {0}; // Stack gets poisoned.
+ printf("Child: %p\n", x);
+ CallNoReturn();
+ // (a) Do nothing, just return to parent function.
+ // (b) Jump into the original function. Stack remains poisoned unless we do
+ // something.
+ // (c) Jump to another function which will then jump back to the main function
+ if (mode == 0) {
+ __sanitizer_start_switch_fiber(nullptr, main_thread_stack,
+ main_thread_stacksize);
+ CallNoReturn();
+ } else if (mode == 1) {
+ __sanitizer_start_switch_fiber(nullptr, main_thread_stack,
+ main_thread_stacksize);
+ CallNoReturn();
+ if (swapcontext(&child_context, &orig_context) < 0) {
+ perror("swapcontext");
+ _exit(1);
+ }
+ } else if (mode == 2) {
+ printf("NextChild stack: %p\n", next_child_stack);
+
+ getcontext(&next_child_context);
+ next_child_context.uc_stack.ss_sp = next_child_stack;
+ next_child_context.uc_stack.ss_size = kStackSize / 2;
+ makecontext(&next_child_context, (void (*)())NextChild, 0);
+ __sanitizer_start_switch_fiber(nullptr, next_child_context.uc_stack.ss_sp,
+ next_child_context.uc_stack.ss_size);
+ CallNoReturn();
+ if (swapcontext(&child_context, &next_child_context) < 0) {
+ perror("swapcontext");
+ _exit(1);
+ }
+ }
+}
+
+int Run(int arg, int mode, char *child_stack) {
+ printf("Child stack: %p\n", child_stack);
+ // Setup child context.
+ getcontext(&child_context);
+ child_context.uc_stack.ss_sp = child_stack;
+ child_context.uc_stack.ss_size = kStackSize / 2;
+ if (mode == 0) {
+ child_context.uc_link = &orig_context;
+ }
+ makecontext(&child_context, (void (*)())Child, 1, mode);
+ CallNoReturn();
+ void *fake_stack_save;
+ __sanitizer_start_switch_fiber(&fake_stack_save, child_context.uc_stack.ss_sp,
+ child_context.uc_stack.ss_size);
+ CallNoReturn();
+ if (swapcontext(&orig_context, &child_context) < 0) {
+ perror("swapcontext");
+ _exit(1);
+ }
+ CallNoReturn();
+ __sanitizer_finish_switch_fiber(fake_stack_save, &from_stack,
+ &from_stacksize);
+ CallNoReturn();
+ printf("Main context from: %p %zu\n", from_stack, from_stacksize);
+
+ // Touch childs's stack to make sure it's unpoisoned.
+ for (int i = 0; i < kStackSize; i++) {
+ child_stack[i] = i;
+ }
+ return child_stack[arg];
+}
+
+void handler(int sig) { CallNoReturn(); }
+
+int main(int argc, char **argv) {
+ // removed huge stack test since hwasan has no huge stack limitations
+
+ // set up a signal that will spam and trigger __hwasan_handle_vfork at
+ // tricky moments
+ struct sigaction act = {};
+ act.sa_handler = &handler;
+ if (sigaction(SIGPROF, &act, 0)) {
+ perror("sigaction");
+ _exit(1);
+ }
+
+ itimerval t;
+ t.it_interval.tv_sec = 0;
+ t.it_interval.tv_usec = 10;
+ t.it_value = t.it_interval;
+ if (setitimer(ITIMER_PROF, &t, 0)) {
+ perror("setitimer");
+ _exit(1);
+ }
+
+ char *heap = new char[kStackSize + 1];
+ next_child_stack = new char[kStackSize + 1];
+ char stack[kStackSize + 1];
+ int ret = 0;
+ // CHECK-NOT: WARNING: HWASan is ignoring requested __hwasan_handle_vfork
+ for (unsigned int i = 0; i < 30; ++i) {
+ ret += Run(argc - 1, 0, stack);
+ // LOOPCHECK: Child stack: [[CHILD_STACK:0x[0-9a-f]*]]
+ // LOOPCHECK: Main context from: [[CHILD_STACK]] 524288
+ ret += Run(argc - 1, 1, stack);
+ // LOOPCHECK: Child stack: [[CHILD_STACK:0x[0-9a-f]*]]
+ // LOOPCHECK: Main context from: [[CHILD_STACK]] 524288
+ ret += Run(argc - 1, 2, stack);
+ // LOOPCHECK: Child stack: [[CHILD_STACK:0x[0-9a-f]*]]
+ // LOOPCHECK: NextChild stack: [[NEXT_CHILD_STACK:0x[0-9a-f]*]]
+ // LOOPCHECK: NextChild from: [[CHILD_STACK]] 524288
+ // LOOPCHECK: Main context from: [[NEXT_CHILD_STACK]] 524288
+ ret += Run(argc - 1, 0, heap);
+ ret += Run(argc - 1, 1, heap);
+ ret += Run(argc - 1, 2, heap);
+ printf("Iteration %d passed\n", i);
+ }
+
+ // CHECK: Test passed
+ printf("Test passed\n");
+
+ delete[] heap;
+ delete[] next_child_stack;
+
+ return ret;
+}
|
|
Thank you for your patch. As far as I can tell from https://patch-diff.githubusercontent.com/raw/llvm/llvm-project/pull/153822.patch, you are using an anonymous email address alias from GitHub. The LLVM Project requires contributors to submit patches with an email address that they are monitoring. See https://discourse.llvm.org/t/hidden-emails-on-github-should-we-do-something-about-it/74223. |
There was a problem hiding this comment.
This should be memory_order_acquire to make sure we read the correct next_stack_bottom_ and next_stack_top_
There was a problem hiding this comment.
Fixed both Thread::StartSwitchFiber and Thread::FinishSwitchFiber
There was a problem hiding this comment.
Why this over __builtin_frame_address(0)?
There was a problem hiding this comment.
please add newline at end of file
There was a problem hiding this comment.
why not just void** in the parameter list?
There was a problem hiding this comment.
maybe
if (auto *t = GetCurrentThread())
t->StartSwitchFiber((uptr)bottom, size);
else
VReport(1, "__hwasan_start_switch_fiber called from unknown thread\n");
There was a problem hiding this comment.
without the context that this was copied from ASan this comment is hard to understand
There was a problem hiding this comment.
Comment added in testcase
There was a problem hiding this comment.
How useful is this though? HWASan is mostly used on AArch64.
There was a problem hiding this comment.
I have refined the testcase and tested on aarch64 platform
1. remove named unused param in `__sanitizer_start_switch_fiber` and `__sanitizer_finish_switch_fiber` 2. code style in `__sanitizer_start_switch_fiber` and `__sanitizer_finish_switch_fiber` 3. fix memory order action in `Thread::FinishSwitchFiber` 4. use `__builtin_frame_address` instead of `char local`
updated commit with public email |
| // RUN: %clangxx_hwasan -std=c++11 -lpthread -ldl -O1 %s -o %t && %run %t 2>&1 | FileCheck %s | ||
| // RUN: %clangxx_hwasan -std=c++11 -lpthread -ldl -O2 %s -o %t && %run %t 2>&1 | FileCheck %s | ||
| // RUN: %clangxx_hwasan -std=c++11 -lpthread -ldl -O3 %s -o %t && %run %t 2>&1 | FileCheck %s | ||
| // RUN: seq 30 | xargs -i -- grep LOOPCHECK %s > %t.checks |
There was a problem hiding this comment.
Out of interest, why this change?
There was a problem hiding this comment.
Original asan testcase tests on stack and heap, therefore it has 60 LOOPCHECK messages.
please correct this part of the commit message |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| _exit(1); | ||
| } | ||
|
|
||
| char *heap = (char *)malloc_func(kStackSize + 1); |
There was a problem hiding this comment.
Could we just make these two global variables and get rid of the dlsym dance?
Please fix formatting |
|
Please fix formatting of test case |
Fixed, however, my clang-format still complains about the |
Did you run |
Yeah, I ran this: The clang-format is built along with the project and the |
| *size_old = stack_top_ - stack_bottom_; | ||
| stack_bottom_ = next_stack_bottom_; | ||
| stack_top_ = next_stack_top_; | ||
| atomic_store(&stack_switching_, 0, memory_order_release); |
There was a problem hiding this comment.
not sure why this is before the assignments below. but it is the same in asan, so let's leave it like this for now.
|
@Tomahawkd Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
4 participants
Currently HwASan has no fiber switch interface for coroutines. This PR adds fiber switch interfaces similar to ASan which helps to pass sp check correctly on unwinding.
The only difference is HwASan does not need a fake stack since tags can do the same thing (e.g., detect UAR). Interfaces are made identical with ASan's.
Also adds unit test which is similar to ASan with minor adjustments:
__asan_handle_no_returnto__hwasan_handle_vfork__hwasan_handle_vforkhas no stack size limitation.longjmpThe testcase is tested on both x86 with alias mode enabled and aarch64.