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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 2 inline comments as done.
pcc added inline comments.


================
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,
----------------
hctim wrote:
> 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.
That will only cover the cases that we can think of. It seems better to rely on a fuzzer since it will also (in principle) cover the cases that we can't think of.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:185
+inline uint8_t extractTag(uptr Ptr) {
+  return Ptr >> 56;
+}
----------------
pcc wrote:
> hctim wrote:
> > Mask TBI bits here? We don't need them, and we should allow users to have them.
> I'm not sure that that's the only change that we would need to make in order to support passing TBI bits into the allocator, so I'd prefer not to unless we have some kind of test for that capability.
Actually, since this function takes a pointer derived from a fault address (i.e. not explicitly passed into the allocator), it may legitimately have TBI bits, so I'll mask them out here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77283