scudo: Require fault address to be in bounds for UAF.

[pcc]

Summary

The bounds check that we previously had here was suitable for secondary
allocations but not for UAF on primary allocations, where it is likely
to result in false positives. Fix it by using a different bounds check
for UAF that requires the fault address to be in bounds.

[eugenis]

compiler-rt/lib/scudo/standalone/combined.h

1350

getPageSizeCached is an arbitrary threshold for reporting secondary oob, right? That could use a comment. In general, it would be great to list the assumption reporting code makes about the buffer contents - ex. the fact that allocation-without-deallocation entries are only possible for secondary.

[pcc]

compiler-rt/lib/scudo/standalone/combined.h

1350

It's based on the size of the guard region on either side of the allocation, which is guaranteed to be at least a page (guard page on the right, guard page + tagged region on the left). I'll add some comments here.

[eugenis]

compiler-rt/lib/scudo/standalone/combined.h

1350

Ah good point. Since we do not tag secondary allocations, we would not know what to do with anything we find beyond the guard page anyway!

[pcc]

• Add comments

[eugenis]

LGTM

compiler-rt/lib/scudo/standalone/combined.h

auto *RingBuffer =
         reinterpret_cast<const AllocationRingBuffer *>(RingBufferPtr);
     uptr Pos = atomic_load_relaxed(&RingBuffer->Pos);
 
     for (uptr I = Pos - 1; I != Pos - 1 - AllocationRingBuffer::NumEntries &&
                            NextErrorReport != NumErrorReports;
          --I) {
       auto *Entry = &RingBuffer->Entries[I % AllocationRingBuffer::NumEntries];
       uptr EntryPtr = atomic_load_relaxed(&Entry->Ptr);
-      uptr UntaggedEntryPtr = untagPointer(EntryPtr);
-      uptr EntrySize = atomic_load_relaxed(&Entry->AllocationSize);
-      if (!EntryPtr || FaultAddr < EntryPtr - getPageSizeCached() ||
-          FaultAddr >= EntryPtr + EntrySize + getPageSizeCached())
+      if (!EntryPtr)
         continue;
 
+      uptr UntaggedEntryPtr = untagPointer(EntryPtr);
+      uptr EntrySize = atomic_load_relaxed(&Entry->AllocationSize);
       u32 AllocationTrace = atomic_load_relaxed(&Entry->AllocationTrace);
       u32 AllocationTid = atomic_load_relaxed(&Entry->AllocationTid);
       u32 DeallocationTrace = atomic_load_relaxed(&Entry->DeallocationTrace);
       u32 DeallocationTid = atomic_load_relaxed(&Entry->DeallocationTid);
 
+      if (DeallocationTid) {
+        // For UAF we only consider in-bounds fault addresses because
+        // out-of-bounds UAF is rare and attempting to detect it is very likely
+        // to result in false positives.
+        if (FaultAddr < EntryPtr || FaultAddr >= EntryPtr + EntrySize)
+          continue;
+      } else {
+        // Ring buffer OOB is only possible with secondary allocations. In this
+        // case we are guaranteed a guard region of at least a page on either
+        // side of the allocation (guard page on the right, guard page + tagged
+        // region on the left), so ignore any faults outside of that range.
+        if (FaultAddr < EntryPtr - getPageSizeCached() ||
+            FaultAddr >= EntryPtr + EntrySize + getPageSizeCached())
+          continue;
+
         // For UAF the ring buffer will contain two entries, one for the
         // allocation and another for the deallocation. Don't report buffer
         // overflow/underflow using the allocation entry if we have already
         // collected a report from the deallocation entry.
-      if (!DeallocationTrace) {
         bool Found = false;
         for (uptr J = 0; J != NextErrorReport; ++J) {
           if (ErrorInfo->reports[J].allocation_address == UntaggedEntryPtr) {
             Found = true;
             break;
           }
         }
         if (Found)