[compiler-rt] [scudo] Add flags to secondary cache entries. (PR #177506)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Christopher Ferris (cferris1000)

<details>
<summary>Changes</summary>

Instead of setting a cache entry back to read-write based on whether MTE is enabled, add a flag that indicates if this entry was made inaccessible. On Android, we have seen strange crashes were MTE is disabled for a process, but the cache entry is inaccessible, causing an access error. This indicates MTE was enabled at some point, then disabled again. There is likely some race, or method to disable without making inaccessible entries read-writable again. Adding the flags should avoid this happening in the future.

Remove the resetting of the secondary cache entries when disabling MTE since it can be done as necessary.

This change will also allow us to always make the secondary cache entries inaccessible in the future to increase security measures even when MTE is not enabled.

Current unit tests already cover this change.

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


1 Files Affected:

- (modified) compiler-rt/lib/scudo/standalone/secondary.h (+22-13) 


``````````diff
diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index 04e33c04baa34..c2c6ebc337608 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -100,6 +100,12 @@ struct CachedBlock {
   u16 Next = 0;
   u16 Prev = 0;
 
+  enum CacheFlags : u16 {
+    None = 0,
+    NoAccess = 0x1,
+  };
+  CacheFlags Flags = CachedBlock::None;
+
   bool isValid() { return CommitBase != 0; }
 
   void invalidate() { CommitBase = 0; }
@@ -299,7 +305,9 @@ class MapAllocatorCache {
         Entry.MemMap.setMemoryPermission(Entry.CommitBase, Entry.CommitSize,
                                          MAP_NOACCESS);
       }
-    }
+      Entry.Flags = CachedBlock::NoAccess;
+    } else
+      Entry.Flags = CachedBlock::None;
 
     // Usually only one entry will be evicted from the cache.
     // Only in the rare event that the cache shrinks in real-time
@@ -522,20 +530,18 @@ class MapAllocatorCache {
   }
 
   void disableMemoryTagging() EXCLUDES(Mutex) {
+    if (Config::getQuarantineDisabled())
+      return;
+
     ScopedLock L(Mutex);
-    if (!Config::getQuarantineDisabled()) {
-      for (u32 I = 0; I != Config::getQuarantineSize(); ++I) {
-        if (Quarantine[I].isValid()) {
-          MemMapT &MemMap = Quarantine[I].MemMap;
-          unmapCallBack(MemMap);
-          Quarantine[I].invalidate();
-        }
+    for (u32 I = 0; I != Config::getQuarantineSize(); ++I) {
+      if (Quarantine[I].isValid()) {
+        MemMapT &MemMap = Quarantine[I].MemMap;
+        unmapCallBack(MemMap);
+        Quarantine[I].invalidate();
       }
-      QuarantinePos = -1U;
     }
-
-    for (CachedBlock &Entry : LRUEntries)
-      Entry.MemMap.setMemoryPermission(Entry.CommitBase, Entry.CommitSize, 0);
+    QuarantinePos = -1U;
   }
 
   void disable() NO_THREAD_SAFETY_ANALYSIS { Mutex.lock(); }
@@ -754,9 +760,12 @@ MapAllocator<Config>::tryAllocateFromCache(const Options &Options, uptr Size,
   LargeBlock::Header *H = reinterpret_cast<LargeBlock::Header *>(
       LargeBlock::addHeaderTag<Config>(EntryHeaderPos));
   bool Zeroed = Entry.Time == 0;
+
+  if (UNLIKELY(Entry.Flags & CachedBlock::NoAccess))
+    Entry.MemMap.setMemoryPermission(Entry.CommitBase, Entry.CommitSize, 0);
+
   if (useMemoryTagging<Config>(Options)) {
     uptr NewBlockBegin = reinterpret_cast<uptr>(H + 1);
-    Entry.MemMap.setMemoryPermission(Entry.CommitBase, Entry.CommitSize, 0);
     if (Zeroed) {
       storeTags(LargeBlock::addHeaderTag<Config>(Entry.CommitBase),
                 NewBlockBegin);

``````````

</details>


https://github.com/llvm/llvm-project/pull/177506