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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

The code now implements UAF checks by retagging on free.

In D70762#1767533 <https://reviews.llvm.org/D70762#1767533>, @cryptoad wrote:

> In D70762#1767420 <https://reviews.llvm.org/D70762#1767420>, @pcc wrote:
>
> > When you say reclaiming you mean calling releasePagesToOS(), correct? In that case, wouldn't that cause the header to be set to 0, which would put us in the same state as if we hadn't used the chunk before?
>
>
> This is correct, with the caveat that it was allocated and freed as opposed to never used.


I discovered that it is possible for a chunk to be partially reclaimed, which means that the header could still be there, but part or all of the data could be reclaimed. The code that I've added to `allocate()` handles the various possibilities.

In D70762#1762390 <https://reviews.llvm.org/D70762#1762390>, @pcc wrote:

> One complication is that we need to handle the case where the new allocation has lower alignment than the old allocation. In that case, malloc will need to set tags on both sides of the allocation (because the previous free will have retagged starting from a higher address).


I tried implementing this but was uncomfortable with the level of code complexity, so the code only tries to reuse tags if our start address is the same as that of the previous allocation. This should be true in the majority of cases, so it seems fine to me.



================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:293
 
+    Ptr = maybeUntagPointer(Ptr);
+
----------------
hctim wrote:
> eugenis wrote:
> > Do we want to touch memory with the tagged pointer first to catch double-free & invalid-free bugs?
> Should be handled below in the chunk header check, no?
Yes, let's leave it to the header check.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:510
 
+  static_assert(!PrimaryT::SupportsMemoryTagging ||
+                MinAlignment >= archMemoryTagGranuleSize(), "");
----------------
pcc wrote:
> cryptoad wrote:
> > I you could use `COMPILER_CHECK`, as it is used in other places.
> > It ends up being a `static_assert` but it feel more consistent.
> The rest of LLVM uses `static_assert` directly. I'll change this one to `COMPILER_CHECK` but maybe this could be changed over to being consistent with LLVM as well?
Left as is due to D70793


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:23
+
+inline constexpr bool archSupportsMemoryTagging() { return true; }
+inline constexpr size_t archMemoryTagGranuleSize() { return 16; }
----------------
pcc wrote:
> cryptoad wrote:
> > If you could use the `INLINE` macros for consistency please.
> Sure, again `inline` is what the rest of LLVM uses.
Left as is due to D70793


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:50
+  void *TaggedPtr, *Cur, *End;
+  __asm__ __volatile__(
+      R"(
----------------
hctim wrote:
> These asm stubs seem mostly abstractable - which would allow us to extend to future platforms easier, and make the intermediate [read - non-mte instructions] code easier to maintain.
> 
> Looks like we could abstract away to `storeZeroTag` abd `randomTagMemory` (or similar).
I created `setRandomTag` since I needed it for tag-on-free, although I feel that it's better to keep things at the chunk level here as splitting things into too many pieces can make it harder to understand the big picture.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70762