[asan] Add experimental 'poison_history_size' flag

[thurstond]

This adds an experimental flag that will keep track of where the manual memory poisoning (__asan_poison_memory_region) is called from, and print the stack trace if the poisoned region is accessed. (Absent this flag, ASan will tell you what code accessed a poisoned region, but not which code set the poison.)

This implementation performs best-effort record keeping using ring buffers, as suggested by Vitaly. The size of each ring buffer is set by the poison_history_size flag.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Thurston Dang (thurstond)

Changes

This adds an experimental flag that will keep track of where the manual memory poisoning (__asan_poison_memory_region) is called from, and print the stack trace if the poisoned region is accessed. (Currently, ASan will tell you what code accessed a poisoned region, but not which code set the poison.)

This implementation performs best-effort record keeping using ring buffers, as suggested by Vitaly. The size of each ring buffer is set by the track_poison flag value. Some records may be lost in multi-threaded programs.

---

Full diff: https://github.com/llvm/llvm-project/pull/133175.diff

7 Files Affected:

• (modified) compiler-rt/lib/asan/asan_descriptions.h (+4)
• (modified) compiler-rt/lib/asan/asan_errors.cpp (+64-2)
• (modified) compiler-rt/lib/asan/asan_flags.inc (+3)
• (modified) compiler-rt/lib/asan/asan_poisoning.cpp (+71-3)
• (modified) compiler-rt/lib/asan/asan_poisoning.h (+51)
• (modified) compiler-rt/lib/asan/asan_rtl.cpp (+2)
• (added) compiler-rt/test/asan/TestCases/use-after-poison-tracked.cpp (+47)

diff --git a/compiler-rt/lib/asan/asan_descriptions.h b/compiler-rt/lib/asan/asan_descriptions.h
index a614f47d461bb..6e23555b35d28 100644
--- a/compiler-rt/lib/asan/asan_descriptions.h
+++ b/compiler-rt/lib/asan/asan_descriptions.h
@@ -15,6 +15,7 @@
 #define ASAN_DESCRIPTIONS_H
 
 #include "asan_allocator.h"
+#include "asan_poisoning.h"
 #include "asan_thread.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_report_decorator.h"
@@ -46,6 +47,9 @@ class Decorator : public __sanitizer::SanitizerCommonDecorator {
   const char *Allocation() { return Magenta(); }
 
   const char *ShadowByte(u8 byte) {
+    if (IsPoisonTrackingMagic(byte))
+      return Blue();
+
     switch (byte) {
       case kAsanHeapLeftRedzoneMagic:
       case kAsanArrayCookieMagic:
diff --git a/compiler-rt/lib/asan/asan_errors.cpp b/compiler-rt/lib/asan/asan_errors.cpp
index 4f112cc5d1bca..d0be3d48172da 100644
--- a/compiler-rt/lib/asan/asan_errors.cpp
+++ b/compiler-rt/lib/asan/asan_errors.cpp
@@ -12,8 +12,10 @@
 //===----------------------------------------------------------------------===//
 
 #include "asan_errors.h"
+
 #include "asan_descriptions.h"
 #include "asan_mapping.h"
+#include "asan_poisoning.h"
 #include "asan_report.h"
 #include "asan_stack.h"
 #include "sanitizer_common/sanitizer_stackdepot.h"
@@ -505,6 +507,19 @@ ErrorGeneric::ErrorGeneric(u32 tid, uptr pc_, uptr bp_, uptr sp_, uptr addr,
           far_from_bounds = AdjacentShadowValuesAreFullyPoisoned(shadow_addr);
           break;
       }
+
+      if (flags()->track_poison > 0 && IsPoisonTrackingMagic(shadow_val)) {
+        if (internal_strcmp(bug_descr, "unknown-crash") != 0) {
+          Printf(
+              "ERROR: use-after-poison tracking magic values overlap with "
+              "other constants.\n");
+          Printf("Please file a bug.\n");
+        } else {
+          bug_descr = "use-after-poison";
+          bug_type_score = 20;
+        }
+      }
+
       scariness.Scare(bug_type_score + read_after_free_bonus, bug_descr);
       if (far_from_bounds) scariness.Scare(10, "far-from-bounds");
     }
@@ -550,8 +565,12 @@ static void PrintLegend(InternalScopedString *str) {
   PrintShadowByte(str, "  Global redzone:          ", kAsanGlobalRedzoneMagic);
   PrintShadowByte(str, "  Global init order:       ",
                   kAsanInitializationOrderMagic);
-  PrintShadowByte(str, "  Poisoned by user:        ",
-                  kAsanUserPoisonedMemoryMagic);
+  // TODO: sync description with PoisonTrackingMagicValues
+  PrintShadowByte(
+      str, "  Poisoned by user:        ", kAsanUserPoisonedMemoryMagic,
+      flags()->track_poison > 0 ? " with detailed tracking using {0x80-0x8F, "
+                                  "0x90-0x9F, 0xD0-0xDF, 0xE0-0xEF}\n"
+                                : "\n");
   PrintShadowByte(str, "  Container overflow:      ",
                   kAsanContiguousContainerOOBMagic);
   PrintShadowByte(str, "  Array cookie:            ",
@@ -600,6 +619,44 @@ static void PrintShadowMemoryForAddress(uptr addr) {
   Printf("%s", str.data());
 }
 
+static void CheckPoisonRecords(uptr addr) {
+  if (!AddrIsInMem(addr))
+    return;
+  uptr shadow_addr = MemToShadow(addr);
+  unsigned char poison_magic = *(reinterpret_cast<u8 *>(shadow_addr));
+  int poison_index = PoisonTrackingMagicToIndex[poison_magic];
+
+  if (poison_index < 0 || poison_index >= NumPoisonTrackingMagicValues)
+    return;
+
+  PoisonRecordRingBuffer *PoisonRecord =
+      reinterpret_cast<PoisonRecordRingBuffer *>(PoisonRecords[poison_index]);
+  if (PoisonRecord) {
+    bool FoundMatch = false;
+
+    for (unsigned int i = 0; i < PoisonRecord->size(); i++) {
+      struct PoisonRecord Record = (*PoisonRecord)[i];
+      if (Record.begin <= addr && addr <= Record.end) {
+        FoundMatch = true;
+
+        StackTrace poison_stack = StackDepotGet(Record.stack_id);
+
+        Printf("\n");
+        Printf("Memory was manually poisoned by thread T%u:\n",
+               Record.thread_id);
+        poison_stack.Print();
+
+        break;
+      }
+    }
+
+    if (!FoundMatch) {
+      Printf("ERROR: no matching poison tracking record found.\n");
+      Printf("Try setting a larger track_poison value.\n");
+    }
+  }
+}
+
 void ErrorGeneric::Print() {
   Decorator d;
   Printf("%s", d.Error());
@@ -623,6 +680,11 @@ void ErrorGeneric::Print() {
     PrintContainerOverflowHint();
   ReportErrorSummary(bug_descr, &stack);
   PrintShadowMemoryForAddress(addr);
+
+  // This uses a range of shadow values, hence it is not convenient to make a
+  // specific error handler.
+  if (flags()->track_poison > 0)
+    CheckPoisonRecords(addr);
 }
 
 }  // namespace __asan
diff --git a/compiler-rt/lib/asan/asan_flags.inc b/compiler-rt/lib/asan/asan_flags.inc
index fad1577d912a5..2e3373f55c887 100644
--- a/compiler-rt/lib/asan/asan_flags.inc
+++ b/compiler-rt/lib/asan/asan_flags.inc
@@ -116,6 +116,9 @@ ASAN_FLAG(bool, poison_partial, true,
           "stack buffers.")
 ASAN_FLAG(bool, poison_array_cookie, true,
           "Poison (or not) the array cookie after operator new[].")
+ASAN_FLAG(int, track_poison, 0,
+          "[EXPERIMENTAL] If non-zero, record the stack trace of manual "
+          "memory poisoning calls.")
 
 // Turn off alloc/dealloc mismatch checker on Mac and Windows for now.
 // https://github.com/google/sanitizers/issues/131
diff --git a/compiler-rt/lib/asan/asan_poisoning.cpp b/compiler-rt/lib/asan/asan_poisoning.cpp
index 762670632f4e0..b65d21fe2ce4c 100644
--- a/compiler-rt/lib/asan/asan_poisoning.cpp
+++ b/compiler-rt/lib/asan/asan_poisoning.cpp
@@ -20,11 +20,48 @@
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_interface_internal.h"
 #include "sanitizer_common/sanitizer_libc.h"
+#include "sanitizer_common/sanitizer_stackdepot.h"
 
 namespace __asan {
 
 static atomic_uint8_t can_poison_memory;
 
+PoisonRecordRingBuffer *PoisonRecords[NumPoisonTrackingMagicValues] = {0};
+int PoisonTrackingMagicToIndex[256] = {-1};
+
+void InitializePoisonTracking() {
+  if (flags()->track_poison <= 0)
+    return;
+
+  for (unsigned int i = 0; i < sizeof(PoisonTrackingMagicToIndex) / sizeof(int);
+       i++) {
+    PoisonTrackingMagicToIndex[i] = -1;
+  }
+
+  for (unsigned int i = 0; i < NumPoisonTrackingMagicValues; i++) {
+    int magic = PoisonTrackingIndexToMagic[i];
+    CHECK(magic > 0);
+    CHECK((unsigned int)magic <
+          sizeof(PoisonTrackingMagicToIndex) / sizeof(int));
+
+    // Necessary for AddressIsPoisoned calculations
+    CHECK((char)magic < 0);
+
+    PoisonTrackingMagicToIndex[magic] = i;
+
+    PoisonRecords[i] = PoisonRecordRingBuffer::New(flags()->track_poison);
+  }
+}
+
+bool IsPoisonTrackingMagic(int byte) {
+  return (byte >= 0 &&
+          (unsigned long)byte <
+              (sizeof(PoisonTrackingMagicToIndex) / sizeof(int)) &&
+          PoisonTrackingMagicToIndex[byte] >= 0 &&
+          PoisonTrackingMagicToIndex[byte] < NumPoisonTrackingMagicValues &&
+          PoisonTrackingIndexToMagic[PoisonTrackingMagicToIndex[byte]] == byte);
+}
+
 void SetCanPoisonMemory(bool value) {
   atomic_store(&can_poison_memory, value, memory_order_release);
 }
@@ -107,6 +144,31 @@ void __asan_poison_memory_region(void const volatile *addr, uptr size) {
   uptr end_addr = beg_addr + size;
   VPrintf(3, "Trying to poison memory region [%p, %p)\n", (void *)beg_addr,
           (void *)end_addr);
+
+  u32 poison_magic = kAsanUserPoisonedMemoryMagic;
+
+  GET_CALLER_PC_BP;
+  GET_STORE_STACK_TRACE_PC_BP(pc, bp);
+  // TODO: garbage collect stacks once they fall off the ring buffer?
+  // StackDepot doesn't currently have a way to delete stacks.
+  u32 stack_id = StackDepotPut(stack);
+
+  if (flags()->track_poison > 0) {
+    u32 current_tid = GetCurrentTidOrInvalid();
+    u32 poison_index = ((stack_id * 151157) ^ (current_tid * 733123)) %
+                       NumPoisonTrackingMagicValues;
+    poison_magic = PoisonTrackingIndexToMagic[poison_index];
+    PoisonRecord record{.stack_id = stack_id,
+                        .thread_id = current_tid,
+                        .begin = beg_addr,
+                        .end = end_addr};
+    // This is racy: with concurrent writes, some records may be lost,
+    // but it's a sacrifice I am willing to make for speed.
+    // The sharding across PoisonRecords reduces the likelihood of
+    // concurrent writes.
+    PoisonRecords[poison_index]->push(record);
+  }
+
   ShadowSegmentEndpoint beg(beg_addr);
   ShadowSegmentEndpoint end(end_addr);
   if (beg.chunk == end.chunk) {
@@ -119,7 +181,7 @@ void __asan_poison_memory_region(void const volatile *addr, uptr size) {
       if (beg.offset > 0) {
         *beg.chunk = Min(value, beg.offset);
       } else {
-        *beg.chunk = kAsanUserPoisonedMemoryMagic;
+        *beg.chunk = poison_magic;
       }
     }
     return;
@@ -134,10 +196,11 @@ void __asan_poison_memory_region(void const volatile *addr, uptr size) {
     }
     beg.chunk++;
   }
-  REAL(memset)(beg.chunk, kAsanUserPoisonedMemoryMagic, end.chunk - beg.chunk);
+
+  REAL(memset)(beg.chunk, poison_magic, end.chunk - beg.chunk);
   // Poison if byte in end.offset is unaddressable.
   if (end.value > 0 && end.value <= end.offset) {
-    *end.chunk = kAsanUserPoisonedMemoryMagic;
+    *end.chunk = poison_magic;
   }
 }
 
@@ -147,6 +210,11 @@ void __asan_unpoison_memory_region(void const volatile *addr, uptr size) {
   uptr end_addr = beg_addr + size;
   VPrintf(3, "Trying to unpoison memory region [%p, %p)\n", (void *)beg_addr,
           (void *)end_addr);
+
+  // Note: we don't need to update the poison tracking here. Since the shadow
+  // memory will be unpoisoned, the poison tracking ring buffer entries will be
+  // ignored.
+
   ShadowSegmentEndpoint beg(beg_addr);
   ShadowSegmentEndpoint end(end_addr);
   if (beg.chunk == end.chunk) {
diff --git a/compiler-rt/lib/asan/asan_poisoning.h b/compiler-rt/lib/asan/asan_poisoning.h
index 600bd011f304c..82d1961c92cbf 100644
--- a/compiler-rt/lib/asan/asan_poisoning.h
+++ b/compiler-rt/lib/asan/asan_poisoning.h
@@ -10,15 +10,64 @@
 //
 // Shadow memory poisoning by ASan RTL and by user application.
 //===----------------------------------------------------------------------===//
+#ifndef ASAN_POISONING_H
+#define ASAN_POISONING_H
 
 #include "asan_interceptors.h"
 #include "asan_internal.h"
 #include "asan_mapping.h"
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_platform.h"
+#include "sanitizer_common/sanitizer_ring_buffer.h"
+
+// For platforms which support slow unwinder only, we restrict the store context
+// size to 1, basically only storing the current pc. We do this because the slow
+// unwinder which is based on libunwind is not async signal safe and causes
+// random freezes in forking applications as well as in signal handlers.
+#define GET_STORE_STACK_TRACE_PC_BP(pc, bp)                            \
+  UNINITIALIZED BufferedStackTrace stack;                              \
+  int max_stack = 16;                                                  \
+  if (!SANITIZER_CAN_FAST_UNWIND)                                      \
+    max_stack = Min(max_stack, 1);                                     \
+  stack.Unwind(pc, bp, nullptr, common_flags()->fast_unwind_on_malloc, \
+               max_stack);
+
+#define GET_STORE_STACK_TRACE \
+  GET_STORE_STACK_TRACE_PC_BP(StackTrace::GetCurrentPc(), GET_CURRENT_FRAME())
 
 namespace __asan {
 
+// These need to be negative chars (i.e., in the range [0x80 .. 0xff]) for
+// AddressIsPoisoned calculations.
+static const int PoisonTrackingIndexToMagic[] = {
+    0x80, 0x81, 0x82, 0x83, 0x84, 0x85, 0x86, 0x87, 0x88, 0x89, 0x8a,
+    0x8b, 0x8c, 0x8d, 0x8e, 0x8f, 0x90, 0x91, 0x92, 0x93, 0x94, 0x95,
+    0x96, 0x97, 0x98, 0x99, 0x9a, 0x9b, 0x9c, 0x9d, 0x9e, 0x9f, 0xd0,
+    0xd1, 0xd2, 0xd3, 0xd4, 0xd5, 0xd6, 0xd7, 0xd8, 0xd9, 0xda, 0xdb,
+    0xdc, 0xdd, 0xde, 0xdf, 0xe0, 0xe1, 0xe2, 0xe3, 0xe4, 0xe5, 0xe6,
+    0xe7, 0xe8, 0xe9, 0xea, 0xeb, 0xec, 0xed, 0xee, 0xef,
+};
+static const int NumPoisonTrackingMagicValues =
+    sizeof(PoisonTrackingIndexToMagic) / sizeof(int);
+
+extern int PoisonTrackingMagicToIndex[256];
+
+struct PoisonRecord {
+  unsigned int stack_id;
+  unsigned int thread_id;
+  uptr begin;
+  uptr end;
+};
+
+typedef RingBuffer<struct PoisonRecord> PoisonRecordRingBuffer;
+extern PoisonRecordRingBuffer* PoisonRecords[NumPoisonTrackingMagicValues];
+
+// Set up data structures for track_poison.
+void InitializePoisonTracking();
+
+// Is this number a magic value used for poison tracking?
+bool IsPoisonTrackingMagic(int byte);
+
 // Enable/disable memory poisoning.
 void SetCanPoisonMemory(bool value);
 bool CanPoisonMemory();
@@ -96,3 +145,5 @@ ALWAYS_INLINE void FastPoisonShadowPartialRightRedzone(
 void FlushUnneededASanShadowMemory(uptr p, uptr size);
 
 }  // namespace __asan
+
+#endif  // ASAN_POISONING_H
diff --git a/compiler-rt/lib/asan/asan_rtl.cpp b/compiler-rt/lib/asan/asan_rtl.cpp
index 19c6c210b564c..379949cdc29f4 100644
--- a/compiler-rt/lib/asan/asan_rtl.cpp
+++ b/compiler-rt/lib/asan/asan_rtl.cpp
@@ -460,6 +460,8 @@ static bool AsanInitInternal() {
   allocator_options.SetFrom(flags(), common_flags());
   InitializeAllocator(allocator_options);
 
+  InitializePoisonTracking();
+
   if (SANITIZER_START_BACKGROUND_THREAD_IN_ASAN_INTERNAL)
     MaybeStartBackgroudThread();
 
diff --git a/compiler-rt/test/asan/TestCases/use-after-poison-tracked.cpp b/compiler-rt/test/asan/TestCases/use-after-poison-tracked.cpp
new file mode 100644
index 0000000000000..c4167279f5164
--- /dev/null
+++ b/compiler-rt/test/asan/TestCases/use-after-poison-tracked.cpp
@@ -0,0 +1,47 @@
+// Check that __asan_poison_memory_region and ASAN_OPTIONS=track_poison work.
+//
+// RUN: %clangxx_asan -O0 %s -o %t && env ASAN_OPTIONS=track_poison=1000 not %run %t       2>&1 | FileCheck %s --check-prefixes=CHECK-AC,CHECK-A
+// RUN: %clangxx_asan -O0 %s -o %t && env ASAN_OPTIONS=track_poison=1000     %run %t 20    2>&1 | FileCheck %s --check-prefixes=CHECK-B
+// RUN: %clangxx_asan -O0 %s -o %t && env ASAN_OPTIONS=track_poison=1000 not %run %t 30 30 2>&1 | FileCheck %s --check-prefixes=CHECK-AC,CHECK-C
+
+#include <stdio.h>
+#include <stdlib.h>
+
+extern "C" void __asan_poison_memory_region(void *, size_t);
+extern "C" void __asan_unpoison_memory_region(void *, size_t);
+
+void novichok(char *x) {
+  __asan_poison_memory_region(x, 64);       // A
+  __asan_unpoison_memory_region(x + 16, 8); // B
+  __asan_poison_memory_region(x + 24, 16);  // C
+}
+
+void fsb(char *x) { novichok(x); }
+
+int main(int argc, char **argv) {
+  char *x = new char[64];
+  x[10] = 0;
+  fsb(x);
+  // Bytes [ 0, 15]: poisoned by A
+  // Bytes [16, 23]: unpoisoned by B
+  // Bytes [24, 63]: poisoned by C
+
+  int res = x[argc * 10]; // BOOOM
+  // CHECK-AC: ERROR: AddressSanitizer: use-after-poison
+  // CHECK-AC: main{{.*}}use-after-poison-tracked.cpp:[[@LINE-2]]
+  // CHECK-B-NOT: ERROR: AddressSanitizer: use-after-poison
+
+  // CHECK-AC: Memory was manually poisoned by thread T0:
+  // CHECK-A: novichok{{.*}}use-after-poison-tracked.cpp:[[@LINE-21]]
+  // CHECK-C: novichok{{.*}}use-after-poison-tracked.cpp:[[@LINE-20]]
+  // CHECK-AC: fsb{{.*}}use-after-poison-tracked.cpp:[[@LINE-18]]
+  // CHECK-AC: main{{.*}}use-after-poison-tracked.cpp:[[@LINE-14]]
+  // CHECK-B-NOT: Memory was manually poisoned by thread T0:
+
+  delete[] x;
+
+  printf("End of program reached\n");
+  // CHECK-B: End of program reached
+
+  return 0;
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[vitalybuka]

More comments, but mostly nitpicking

[vitalybuka]

with one unresolved comment

[thurstond]

with one unresolved comment

Partial granule check fixed in f4c7b3f

[nico]

https://green.lab.llvm.org/job/llvm.org/job/clang-stage1-RA/4113/ has been red since this landed. Please take a look and revert for now if it takes a while to fix.

[nico]

Oh, 3ad2cd5 might help, sorry for the noise.

[thurstond]

Oh, 3ad2cd5 might help, sorry for the noise.

No worries. My apologies for breaking the build!