[hwasan] Fix incorrect candidate matching for stack OOB.
authorFlorian Mayer <fmayer@google.com>
Wed, 30 Jun 2021 14:47:53 +0000 (15:47 +0100)
committerFlorian Mayer <fmayer@google.com>
Tue, 6 Jul 2021 11:24:07 +0000 (12:24 +0100)
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

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

index cf23c29..44047c9 100644 (file)
@@ -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 different 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,32 +436,12 @@ 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;
     uptr ring_index, num_matching_addrs, num_matching_addrs_4b;
@@ -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++;
   }
 
index 7dfd7de..1e6cefa 100644 (file)
@@ -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
 }