This is an archive of the discontinued LLVM Phabricator instance.

scudo: Add an API for disabling memory initialization per-thread.
ClosedPublic

Authored by pcc on Sep 15 2020, 8:59 PM.

Details

Summary

Here "memory initialization" refers to zero- or pattern-init on
non-MTE hardware, or (where possible to avoid) memory tagging on MTE
hardware. With shared TSD the per-thread memory initialization state
is stored in bit 0 of the TLS slot, similar to PointerIntPair in LLVM.

Diff Detail

Event Timeline

pcc created this revision.Sep 15 2020, 8:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 8:59 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
pcc requested review of this revision.Sep 15 2020, 8:59 PM
pcc added a comment.Sep 15 2020, 9:09 PM

This is the patch that I've been using to stress test my implementation:

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index 6c39b8f361e7..54ea5f487ac9 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -9,6 +9,8 @@
 #ifndef SCUDO_COMBINED_H_
 #define SCUDO_COMBINED_H_
 
+#include <stdlib.h>
+
 #include "chunk.h"
 #include "common.h"
 #include "flags.h"
@@ -268,6 +270,14 @@ public:
                           bool ZeroContents = false) {
     initThreadMaybe();
 
+    {
+      int R = rand();
+      if (R % 16 == 0)
+        TSDRegistry.setOption(Option::ThreadDisableMemInit, 0);
+      if (R % 16 == 1)
+        TSDRegistry.setOption(Option::ThreadDisableMemInit, 1);
+    }
+
 #ifdef GWP_ASAN_HOOKS
     if (UNLIKELY(GuardedAlloc.shouldSample())) {
       if (void *Ptr = GuardedAlloc.allocate(roundUpTo(Size, Alignment)))

With this applied and booting Android on FVP I sometimes get an application crash in com.android.settings or com.android.systemui. The tombstone starts like this:

Build fingerprint: 'Android/fvp/fvpbase:S/AOSP.MASTER/eng.pcc.20200813.203906:eng/test-keys'
Revision: '0'
ABI: 'arm64'
Timestamp: 2020-09-15 23:35:00.120584420+0000
pid: 1366, tid: 1707, name: RenderThread  >>> com.android.systemui <<<
uid: 10100
tagged_addr_ctrl: 000000000007fff5
signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x35338100000010
    x0  0000007a599b10b0  x1  0a00007b9dcb2210  x2  0000000000000003  x3  0000000000000003
    x4  0000000000000000  x5  0000007a599b1400  x6  00000000000003ff  x7  0000000000000400
    x8  0000000000000002  x9  0000000000000003  x10 0000000000000002  x11 0000000000000013
    x12 0600007afdd915a0  x13 0000000000000002  x14 0035338100000000  x15 0000000000000007
    x16 0000007d6f9db4a0  x17 0000007d6f76c7c4  x18 0000007a50878000  x19 0a00007b9dcb2210
    x20 0000007a599b10b0  x21 0000007a599b0e40  x22 0000000000000003  x23 0000000000000003
    x24 0000007a599b0d78  x25 0000007a599b3000  x26 0000000000000000  x27 0d00007b9dcc7578
    x28 0000000000000000  x29 0000007a599b0cf0
    lr  0000007d6f814f38  sp  0000007a599b0ce0  pc  0000007d6f7e3e04  pst 0000000020001000

backtrace:
      #00 pc 00000000003e1e04  /system/lib64/libhwui.so (GrResourceAllocator::addInterval(GrSurfaceProxy*, unsigned int, unsigned int)+132) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #01 pc 0000000000412f34  /system/lib64/libhwui.so (GrProcessorSet::visitProxies(std::__1::function<void (GrSurfaceProxy*)> const&) const+304) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #02 pc 00000000003ded90  /system/lib64/libhwui.so (GrRenderTargetOpList::OpChain::visitProxies(std::__1::function<void (GrSurfaceProxy*)> const&, GrOp::VisitorType) const+76) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #03 pc 00000000003e0c60  /system/lib64/libhwui.so (GrRenderTargetOpList::gatherProxyIntervals(GrResourceAllocator*) const+296) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #04 pc 00000000003b9988  /system/lib64/libhwui.so (GrDrawingManager::flush(GrSurfaceProxy*, SkSurface::BackendSurfaceAccess, GrFlushFlags, int, GrBackendSemaphore*, void (*)(void*), void*)+1980) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #05 pc 00000000003ba2b8  /system/lib64/libhwui.so (GrDrawingManager::prepareSurfaceForExternalIO(GrSurfaceProxy*, SkSurface::BackendSurfaceAccess, GrFlushFlags, int, GrBackendSemaphore*, void (*)(void*), void*)+244) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #06 pc 00000000003dd038  /system/lib64/libhwui.so (GrRenderTargetContext::prepareForExternalIO(SkSurface::BackendSurfaceAccess, GrFlushFlags, int, GrBackendSemaphore*, void (*)(void*), void*)+252) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #07 pc 000000000018b0f4  /system/lib64/libhwui.so (android::uirenderer::skiapipeline::SkiaPipeline::renderFrame(android::uirenderer::LayerUpdateQueue const&, SkRect const&, std::__1::vector<android::sp<android::uirenderer::RenderNode>, std::__1::allocator<android::sp<android::uirenderer::RenderNode> > > const&, bool, android::uirenderer::Rect const&, sk_sp<SkSurface>, SkMatrix const&)+208) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #08 pc 0000000000188d04  /system/lib64/libhwui.so (android::uirenderer::skiapipeline::SkiaOpenGLPipeline::draw(android::uirenderer::renderthread::Frame const&, SkRect const&, SkRect const&, android::uirenderer::LightGeometry const&, android::uirenderer::LayerUpdateQueue*, android::uirenderer::Rect const&, bool, android::uirenderer::LightInfo const&, std::__1::vector<android::sp<android::uirenderer::RenderNode>, std::__1::allocator<android::sp<android::uirenderer::RenderNode> > > const&, android::uirenderer::FrameInfoVisualizer*)+496) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #09 pc 00000000001943e4  /system/lib64/libhwui.so (android::uirenderer::renderthread::CanvasContext::draw()+392) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #10 pc 000000000019636c  /system/lib64/libhwui.so (android::uirenderer::renderthread::DrawFrameTask::run()+428) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #11 pc 0000000000179dfc  /system/lib64/libhwui.so (android::uirenderer::WorkQueue::process()+400) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #12 pc 00000000001a56a8  /system/lib64/libhwui.so (android::uirenderer::renderthread::RenderThread::threadLoop()+128) (BuildId: ecfd2fad5f2e390ec6cf7b8577e85b87)
      #13 pc 0000000000014444  /system/lib64/libutils.so (android::Thread::_threadLoop(void*)+260) (BuildId: dfc95e0a7c124e5bf890590bb0f3c55f)
      #14 pc 0000000000013cf8  /system/lib64/libutils.so (thread_data_t::trampoline(void*)+424) (BuildId: dfc95e0a7c124e5bf890590bb0f3c55f)
      #15 pc 00000000000af5fc  /apex/com.android.runtime/lib64/bionic/libc.so (__pthread_start(void*)+344) (BuildId: 6cd1bc2d17bd5e5999e53add462790e2)
      #16 pc 000000000004f448  /apex/com.android.runtime/lib64/bionic/libc.so (__start_thread+64) (BuildId: 6cd1bc2d17bd5e5999e53add462790e2)

And the top frame symbolizes to this line of code:
https://cs.android.com/android/platform/superproject/+/master:external/skia/src/core/SkTDynamicHash.h;l=98
which is accessing a pointer from a calloc'd region and expecting it to either be zero or a stored value, indicating that the memory was not zero initialized correctly.

hctim added inline comments.Sep 16 2020, 11:07 AM
compiler-rt/lib/scudo/standalone/combined.h
498

Isn't this now broken under dealloc_type_mismatch and MTE?

556

same here, broken under dealloc_type_mismatch when memory is zeroed?

pcc added inline comments.Sep 16 2020, 11:51 AM
compiler-rt/lib/scudo/standalone/combined.h
498

No, because the field is intended to only have the "was zeroed" meaning while the chunk is not allocated. Note that the field is read before the call to quarantineOrDeallocateChunk later in this function causes it to take the "was zeroed" meaning.

556

This is before the call to quarantineOrDeallocateChunk.

hctim added inline comments.Sep 16 2020, 12:32 PM
compiler-rt/lib/scudo/standalone/combined.h
498

I see - thanks for the clarification. Maybe OriginIfAliveOrWasZeroedIfDead? A bit wordy (can't think of anything more concise right now), but otherwise it's a little hard to infer this which meaning applies at which time (especially given that SizeOrUnusedBytes has different semantics). Either that or a comment in the declaration explaining the semantics would probably suffice.

eugenis added inline comments.Sep 16 2020, 4:20 PM
compiler-rt/lib/scudo/standalone/combined.h
417

resizeTaggedChunk does not zero memory. I think this memset needs to cover the entire allocation?

417

Ah no, I misread. Ignore this.

428

Rename setRandomTag to setRandomTagAndZeroData ?

pcc added inline comments.Sep 16 2020, 5:01 PM
compiler-rt/lib/scudo/standalone/combined.h
417

I think the problem is related to this line actually. resizeTaggedChunk rounds up to 16 before it starts zeroing, so the memset needs to stop at the rounded-up original bound, otherwise it can leave the end of the original last granule uninitialized. I am testing this diff:

@@ -414,12 +424,18 @@ public:
             // UAF tag. But if tagging was disabled per-thread when the memory
             // was freed, it would not have been retagged and thus zeroed, and
             // therefore it needs to be zeroed now.
-            memset(TaggedPtr, 0, Min(Size, PrevEnd - TaggedUserPtr));
+            memset(TaggedPtr, 0,
+                   Min(Size, roundUpTo(PrevEnd - TaggedUserPtr,
+                                       archMemoryTagGranuleSize())));
           } else if (Size) {
             // Clear any stack metadata that may have previously been stored in
             // the chunk data.
428

Does that really need to be in the name of the function or would a comment be enough? I think most of the functions in memtag.h end up zeroing parts of the memory that they touch.

pcc updated this revision to Diff 292400.Sep 16 2020, 7:25 PM
pcc retitled this revision from [WIP] scudo: Add an API for disabling memory initialization per-thread. to scudo: Add an API for disabling memory initialization per-thread..
pcc edited the summary of this revision. (Show Details)

Fix bug, add test, address review comment

pcc added inline comments.Sep 16 2020, 7:26 PM
compiler-rt/lib/scudo/standalone/combined.h
417

Okay, this looks like this fixed the problem (stress tested on Android/FVP with a more rigorous version of my stress testing patch that verifies that calloc allocations are zeroed at allocation time). Also added a test case that fails with the previous version of my patch.

498

Added a comment in the declaration.

eugenis added inline comments.Sep 17 2020, 12:42 PM
compiler-rt/lib/scudo/standalone/combined.h
410

This implies that MTE mode does not support pattern init - and it's also clear from the rest of the code. Should we have setFillContents do something about that? It does not have a return value, but it could complain in stderr (not sure it's a good idea) or fall back to zero-init.

428

Thanks. Does not have to be in the function name - it's just was something that made me go and look at the implementation.

pcc added inline comments.Sep 17 2020, 2:47 PM
compiler-rt/lib/scudo/standalone/combined.h
410

I think it would be fine to allow setting it to PatternOrZeroFill to go through without warning. As the name implies, it would be valid to implement it using zero init.

eugenis accepted this revision.Sep 17 2020, 3:13 PM

LGTM

compiler-rt/lib/scudo/standalone/combined.h
410

Yes, good point.

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
535

make it a named constant, or use Ptrs.size()

This revision is now accepted and ready to land.Sep 17 2020, 3:13 PM
cryptoad added inline comments.Sep 17 2020, 3:28 PM
compiler-rt/lib/scudo/standalone/combined.h
1034

Looks like some tabs snuck in maybe? You might want to clang-format -style=llvm

eugenis added inline comments.Sep 17 2020, 3:31 PM
compiler-rt/lib/scudo/standalone/combined.h
1034

No, it's a new thing that Phabricator is doing when the code is re-indented :)

cryptoad added inline comments.Sep 17 2020, 3:39 PM
compiler-rt/lib/scudo/standalone/combined.h
1034

Ack!

This revision was landed with ongoing or failed builds.Sep 18 2020, 12:11 PM
This revision was automatically updated to reflect the committed changes.