[PATCH] D77283: scudo: Add support for diagnosing memory errors when memory tagging is enabled.

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:746
+    *ErrorInfo = {};
+    if (!PrimaryT::SupportsMemoryTagging)
+      return;
----------------
I don't think we can assume that `typeof(PrimaryT)` is the same between dead process, and crash handler, right? Same as below with `findNearestBlock()`? A 32-bit process can use a 64-bit crash_handler, right (and I'm not sure whether there's any svelte vs. non-svelte crossover)? I see from your AOSP patches right now we unconditionally create `ScudoCrashData` even though there might be incompatibilities.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:739
+
+  static void getErrorInfo(struct scudo_error_info *ErrorInfo, uintptr_t Ptr,
+                           const char *DepotPtr, const char *RegionInfoPtr,
----------------
pcc wrote:
> hctim wrote:
> > This function makes me a little nervous, as it makes the assumption that the pointers we're provided are valid structs.
> > 
> > Especially in async mode, we can't make that assumption. Given that an attacker may have arbitrary write for some amount of time between fault and trap time, they have full access to whatever structs the crash handler is about to blindly use.
> > 
> > We should validate all these input structs for validity before operating on them. This function should be highly tested, and highly fuzzed. Thankfully, the crash handler on Android is always spawned with less privileges than the crashed process, but RCE in the crash handler is still no fun.
> I think it makes sense to set up a fuzzer for this. As for testing, I feel that integration tests (like the ones that I'm adding to system/core in AOSP) are the appropriate level of testing here, because of the number of moving pieces that need to be set up for this function to work.
Okay - but we definitely neet to test with a garbage depot/regioninfo/memory/memorytags, etc.


================
Comment at: compiler-rt/lib/scudo/standalone/stack_depot.h:50
+  static const uptr kRingBits = 19;
+  static const uptr kRingSize = 1 << kRingBits;
+  static const uptr kRingMask = kRingSize - 1;
----------------
pcc wrote:
> hctim wrote:
> > ```
> > #ifdef __LP64__
> >   static const uptr kRingSize = kRingBits * 8;
> > #else
> >   static const uptr kRingSize = kRingBits * 4;
> > #endif
> > ```
> > 
> > ... as we can ask for trace collection on 32-bit?
> You mean `static const uptr kRingSize = (1 << kRingBits) * 8;` etc.? This is the number of elements, not the size in bytes. If we wanted to shrink this on 32-bit there would be more needed than this. We would probably start by using `uptr` for the type of `ring`, but that's not enough because we're using the upper 32 bits of ring header entries to store the size. This code isn't being used on 32-bit so I'd prefer to waste most of the upper 32 bits on 32-bit for now, rather than write dead code.
Sure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77283/new/

https://reviews.llvm.org/D77283