[PATCH] D70762: scudo: Add initial memory tagging support.

[Peter Collingbourne via Phabricator via llvm-commits]

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


================
Comment at: compiler-rt/lib/scudo/standalone/allocator_config.h:43
   // 1GB regions
-  typedef SizeClassAllocator64<SizeClassMap, 30U> Primary;
+  typedef SizeClassAllocator64<SizeClassMap, 30U, true> Primary;
 #else
----------------
hctim wrote:
> Maybe:
> `typedef SizeClassAllocator64<SizeClassMap, 30U, /*MaySupportMemoryTagging=*/ true> Primary;`?
Will do


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:30
+  return (getauxval(AT_HWCAP2) & HWCAP2_MTE) &&
+         (prctl(PR_GET_TAGGED_ADDR_CTRL, 0, 0, 0, 0) & PR_MTE_TCF_MASK) !=
+             PR_MTE_TCF_NONE;
----------------
kevin.brodsky wrote:
> I wonder if this is really a good thing. If libc fails to enable tag checking before the allocator is initialised (which is quite possible, given that until recently `malloc()` was called very early in Bionic's libc_init), then Scudo will not tag anything. Wouldn't it be possible instead to explicitly ask Scudo to use tagging when it is initialised? This would also be more consistent with the `malloc_disable_memory_tagging()` interface: Scudo does not take care of enabling / disabling tag checking, so arguably it shouldn't check if it is enabled either.
So the motivation behind adding the check here was along the lines of "if the application doesn't enable memory tag checks then there's no point in enabling memory tagging". But I can see the value in decoupling these two things especially since the libc might not have a chance to turn on memory tagging before the first allocation. I'll change this to be purely based on the hwcap and let the application call `malloc_disable_memory_tagging()` early if it doesn't want to use memory tagging.

I'd prefer not to add an explicit initialization step to the allocator since the allocation functions can in principle be called at any time, so we'd need additional complexity to handle the case where the allocation functions were called before the allocator was formally initialized.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:76
+        [ End ] "=&r"(End)
+      : [ Ptr ] "r"(Ptr), [ Size ] "r"(Size));
+}
----------------
kevin.brodsky wrote:
> Since this asm statement is modifying memory, is it safe to use it without a "memory" clobber? It certainly isn't safe in general. Same comment for the other asm statements that use `st*g`.
I was somehow under the impression that `volatile` implied the `"memory"` clobber, but that doesn't appear to be backed up by the documentation so I'll add the clobbers here and elsewhere.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:97
+  // purpose of catching linear overflows in this case.
+  uptr UntaggedEnd = untagPointer(TaggedEnd);
+  if (UntaggedEnd != BlockEnd)
----------------
hctim wrote:
> `Size % 16 == 0` always here, so this could just be `UntaggedEnd = Ptr + Size`?
The caller isn't rounding `Size` as far as I can tell, so this isn't guaranteed to be the case.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:108
+  if (RoundOldPtr >= NewPtr) {
+    // If the allocation is shrinking we just need to set the tag past the end
+    // of the allocation to 0. See explanation in prepareTaggedChunk above.
----------------
hctim wrote:
> 13.8% of chromium fuzzing-found heap OOB are > 16 bytes stride. Given that this is primary-only, the cost  of  retagging the `OldChunk - NewChunk` might be an acceptable performance penalty.
This seems more like something that we'd want to do in precise mode than in mitigation mode. Recall that even with a large stride we (probabilistically) can't go outside of the bounds of the chunk. The chunk is cleared during dealloc with stzg so there's no info leak potential either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70762