[PATCH] D100911: scudo: Make prepareTaggedChunk() and resizeTaggedChunk() generic.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis accepted this revision.
eugenis added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:1146
+    uptr UntaggedNewPtr = untagPointer(RoundNewPtr);
+    if (UntaggedNewPtr != BlockEnd)
+      storeTag(UntaggedNewPtr);
----------------
pcc wrote:
> vitalybuka wrote:
> > vitalybuka wrote:
> > > pcc wrote:
> > > > eugenis wrote:
> > > > > vitalybuka wrote:
> > > > > > pcc wrote:
> > > > > > > vitalybuka wrote:
> > > > > > > > Why we don't tag entire unused tail? It may detect UseAfterFree.
> > > > > > > We detect UAF by retagging on free. So from that perspective there's no advantage to retagging the tail here.
> > > > > > then why do we need to remove a tag on a single granule here?
> > > > > > To distinguish UAF and out of bounds?
> > > > > For guaranteed linear overflow detection.
> > > > > 
> > > > > In fact, by not retagging the tail, we have guaranteed false negative for overflow by 16 bytes, right? Should we change this? It should be pretty cheap in the primary allocator.
> > > > Hmm, I would expect the cost increase to correspond to the average ratio between size classes (i.e. around sqrt(2)).
> > > > 
> > > > Let's investigate doing that as part of a separate change. It should be easier to implement once we have the generic resizeTaggedChunk() implementation in.
> > > So UAF is not guaranteed, but we want linear overflow guaranteed and without spending cycles to retag entire [RoundNewPtr, BlockEnd)?
> > > Hmm, I would expect the cost increase to correspond to the average ratio between size classes (i.e. around sqrt(2)).
> > > 
> > > Let's investigate doing that as part of a separate change. It should be easier to implement once we have the generic resizeTaggedChunk() implementation in.
> > 
> > Agree, this is unchanged in the patch and better addressed (if needed) separately.
> Immediate UAF is guaranteed due to retag on free -- that retags the entire previous allocation (of course, we don't know how large the next allocation to use that chunk will be).
> 
> Yes, the reason why we didn't retag that entire region with 0 was to save cycles.
well probably half of that, on average.
SGTM, let's not worry about that for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100911