[compiler-rt] 745758a - [hwasan] Fix incorrect candidate matching for stack OOB.

[Florian Mayer via llvm-commits]

Author: Florian Mayer
Date: 2021-07-06T12:24:07+01:00
New Revision: 745758acf3c295e3bf9f9dd283a3568c912a1827

URL: https://github.com/llvm/llvm-project/commit/745758acf3c295e3bf9f9dd283a3568c912a1827
DIFF: https://github.com/llvm/llvm-project/commit/745758acf3c295e3bf9f9dd283a3568c912a1827.diff

LOG: [hwasan] Fix incorrect candidate matching for stack OOB.

We would find an address with matching tag, only to discover in
ShowCandidate that it's very far away from [stack].

Reviewed By: eugenis

Differential Revision: https://reviews.llvm.org/D105197

Added: 
    

Modified: 
    compiler-rt/lib/hwasan/hwasan_report.cpp
    compiler-rt/test/hwasan/TestCases/stack-oob.c

Removed: 
    


################################################################################
diff  --git a/compiler-rt/lib/hwasan/hwasan_report.cpp b/compiler-rt/lib/hwasan/hwasan_report.cpp
index cf23c2962889..44047c9fdaf8 100644
--- a/compiler-rt/lib/hwasan/hwasan_report.cpp
+++ b/compiler-rt/lib/hwasan/hwasan_report.cpp
@@ -296,8 +296,8 @@ static uptr GetGlobalSizeFromDescriptor(uptr ptr) {
   return 0;
 }
 
-static void ShowCandidate(uptr untagged_addr, tag_t *candidate, tag_t *left,
-                          tag_t *right) {
+static void ShowHeapOrGlobalCandidate(uptr untagged_addr, tag_t *candidate,
+                                      tag_t *left, tag_t *right) {
   Decorator d;
   uptr mem = ShadowToMem(reinterpret_cast<uptr>(candidate));
   HwasanChunkView chunk = FindHeapChunkByAddress(mem);
@@ -386,11 +386,36 @@ void PrintAddressDescription(
            d.Default());
   }
 
+  tag_t addr_tag = GetTagFromPointer(tagged_addr);
+
+  bool on_stack = false;
+  // Check stack first. If the address is on the stack of a live thread, we
+  // know it cannot be a heap / global overflow.
+  hwasanThreadList().VisitAllLiveThreads([&](Thread *t) {
+    if (t->AddrIsInStack(untagged_addr)) {
+      on_stack = true;
+      // TODO(fmayer): figure out how to distinguish use-after-return and
+      // stack-buffer-overflow.
+      Printf("%s", d.Error());
+      Printf("\nCause: stack tag-mismatch\n");
+      Printf("%s", d.Location());
+      Printf("Address %p is located in stack of thread T%zd\n", untagged_addr,
+             t->unique_id());
+      Printf("%s", d.Default());
+      t->Announce();
+
+      auto *sa = (t == GetCurrentThread() && current_stack_allocations)
+                     ? current_stack_allocations
+                     : t->stack_allocations();
+      PrintStackAllocations(sa, addr_tag, untagged_addr);
+      num_descriptions_printed++;
+    }
+  });
+
   // Check if this looks like a heap buffer overflow by scanning
   // the shadow left and right and looking for the first adjacent
   // object with a 
diff erent memory tag. If that tag matches addr_tag,
   // check the allocator if it has a live chunk there.
-  tag_t addr_tag = GetTagFromPointer(tagged_addr);
   tag_t *tag_ptr = reinterpret_cast<tag_t*>(MemToShadow(untagged_addr));
   tag_t *candidate = nullptr, *left = tag_ptr, *right = tag_ptr;
   uptr candidate_distance = 0;
@@ -411,31 +436,11 @@ void PrintAddressDescription(
 
   constexpr auto kCloseCandidateDistance = 1;
 
-  if (candidate && candidate_distance <= kCloseCandidateDistance) {
-    ShowCandidate(untagged_addr, candidate, left, right);
+  if (!on_stack && candidate && candidate_distance <= kCloseCandidateDistance) {
+    ShowHeapOrGlobalCandidate(untagged_addr, candidate, left, right);
     num_descriptions_printed++;
   }
 
-  hwasanThreadList().VisitAllLiveThreads([&](Thread *t) {
-    if (t->AddrIsInStack(untagged_addr)) {
-      // TODO(fmayer): figure out how to distinguish use-after-return and
-      // stack-buffer-overflow.
-      Printf("%s", d.Error());
-      Printf("\nCause: stack tag-mismatch\n");
-      Printf("%s", d.Location());
-      Printf("Address %p is located in stack of thread T%zd\n", untagged_addr,
-             t->unique_id());
-      Printf("%s", d.Default());
-      t->Announce();
-
-      auto *sa = (t == GetCurrentThread() && current_stack_allocations)
-                     ? current_stack_allocations
-                     : t->stack_allocations();
-      PrintStackAllocations(sa, addr_tag, untagged_addr);
-      num_descriptions_printed++;
-    }
-  });
-
   hwasanThreadList().VisitAllLiveThreads([&](Thread *t) {
     // Scan all threads' ring buffers to find if it's a heap-use-after-free.
     HeapAllocationRecord har;
@@ -474,7 +479,7 @@ void PrintAddressDescription(
   });
 
   if (candidate && num_descriptions_printed == 0) {
-    ShowCandidate(untagged_addr, candidate, left, right);
+    ShowHeapOrGlobalCandidate(untagged_addr, candidate, left, right);
     num_descriptions_printed++;
   }
 

diff  --git a/compiler-rt/test/hwasan/TestCases/stack-oob.c b/compiler-rt/test/hwasan/TestCases/stack-oob.c
index 7dfd7deb2767..1e6cefaf03f1 100644
--- a/compiler-rt/test/hwasan/TestCases/stack-oob.c
+++ b/compiler-rt/test/hwasan/TestCases/stack-oob.c
@@ -27,8 +27,10 @@ int main() {
   // CHECK: READ of size 1 at
   // CHECK: #0 {{.*}} in f{{.*}}stack-oob.c:[[@LINE-6]]
 
+  // CHECK-NOT: Cause: global-overflow
   // CHECK: Cause: stack tag-mismatch
   // CHECK: is located in stack of threa
+  // CHECK-NOT: Cause: global-overflow
 
   // CHECK: SUMMARY: HWAddressSanitizer: tag-mismatch {{.*}} in f
 }