[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.

Mon Apr 22 17:35:24 PDT 2019

vlad.tsyrklevich accepted this revision.
vlad.tsyrklevich added a comment.
This revision is now accepted and ready to land.

LGTM other than nits

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:22
+void GuardedPoolAllocator::init(const options::Options &Opts) {
+  SingletonPtr = this;
+
----------------
Move below the Opts.Enabled check? Not sure if we need this if GWP-ASan isn't enabled.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:64
+
+  if (Opts.InstallSignalHandlers)
+    installSignalHandlers();
----------------
nit: I'd move to just before the IsInitialised store (we do use GuardedPagePool in the signal handler though it's not likely to cause an error as it is now.)

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:95
+  Meta->Size = Size;
+  applyAllocationStrategy(Size, &Ptr);
+
----------------
nit: we can move this up above the Meta->Size store now.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:271
+  case GuardedPoolAllocator::GwpAsanError::USE_AFTER_FREE:
+    (*Printf)("Use after free occured when accessing memory at: 0x%zx\n",
+              AccessPtr);
----------------
nit: occurred here and below.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:298
+  (*Printf)("0x%zx is located ", AccessPtr);
+  if (AccessPtr < Meta->Addr)
+    (*Printf)("%zu bytes to the left ", Meta->Addr - AccessPtr);
----------------
This will print "0 bytes to the right" on UAF at the address of the allocation. Perhaps elide the left/right calculation and just print the info for the allocation.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:47
+  // and metadata allocations during destruction. We can't clean up these areas
+  // as this may cause a racy use-after-free on shutdown.
+  ~GuardedPoolAllocator() = default;
----------------
Nit: Delete racy (I think it can cause plain ol' UAF depending on destructor ordering.)

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:232
+  // when GWP-ASan is disabled, we wish to never spend wasted cycles
+  // recalcualting the sample rate.
+  ALIGNED(8) uint64_t AdjustedSampleRate = UINT64_MAX - 1;
----------------
nit: recalculating

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:74
+    uintptr_t P = reinterpret_cast<uintptr_t>(Ptr);
+    return GetGuardedPagePool() <= P && P < GetGuardedPagePoolEnd();
+  }
----------------
hctim wrote:
> vlad.tsyrklevich wrote:
> > We've given GuardedPagePool/GuardedPagePoolEnd the atomic treatment but since we access them sequentially it's possible that we get the sequence:
> > T1 Load GuardedPagePool
> > T2 Lazy Init
> > T1 Load GuardedPagePoolEnd
> > 
> > and give a bad value if the pointer is between 0 and GuardedPagePoolEnd. It seems like these values should not be atomic but that instead we are forced to perform an IsInitialised check first instead.
> Alternatively, we could set `GuardedPagePool = UINTPTR_MAX` in the constructor for value-initialisation, and not have the overhead of the check. Do you see any problems with this?
I'm not familiar with the atomic orderings but it seems like technically that's still not correct because __ATOMIC_RELAXED doesn't guarantee ordering (though I suppose loading IsInitialized here wouldn't fix that either.) We might need to change the memory order of the writes (hopefully not the reads for optimality) and then we could make that scheme work. (Make sure to document it somewhere.)

================
Comment at: compiler-rt/lib/gwp_asan/optional/runtime_env_flag_parser.cpp:29
+
+static Options GWPAsanFlags; // Use via getOptions().
+
----------------
Nit: GwpAsan or GWPASan are internally coherent capitalization-wise.

================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:222
+#ifdef GWP_ASAN_HOOKS
+  static gwp_asan::GuardedPoolAllocator GuardedAlloc;
+#endif // GWP_ASAN_HOOKS
----------------
nit: spacing doesn't match surrounding code

================
Comment at: compiler-rt/test/gwp_asan/alignment_power_of_two.cpp:18
+  for (const auto& P : AllocSizeToAlignment) {
+    char *Ptr = reinterpret_cast<char *>(malloc(P.first));
+    if (reinterpret_cast<uintptr_t>(Ptr) % P.second != 0) {
----------------
Do we need to ensure this allocation is right-aligned? Or do both left-/right-?

================
Comment at: compiler-rt/test/gwp_asan/alignment_static.cpp:10
+
+int main(int argc, char** argv) {
+  if (argc < 2)
----------------
Seems identical to the last test, delete?

================
Comment at: compiler-rt/test/gwp_asan/allocator_fallback.cpp:9
+// RUN: %env_gwp_asan_options=NumUsableGuardedPages=15 %run %t
+// RUN: %env_gwp_asan_options=NumUsableGuardedPages=16 %run %t
+// RUN: %env_gwp_asan_options=NumUsableGuardedPages=17 %run %t
----------------
We test both sides of the "cliff" here but the cliff is actually at 12 because max alloc size is 1<<12

================
Comment at: compiler-rt/test/gwp_asan/allocator_fallback.cpp:20
+    for (unsigned j = 0; j < i; ++j) {
+      *(Ptr + j) = 0x0;
+    }
----------------
j < (1 << i) ? Can probably just write Ptr[0] and Ptr[(1<<i )- 1]

================
Comment at: compiler-rt/test/gwp_asan/pattern_new_delete.cpp:9
+int main() {
+  for (unsigned i = 1; i < 0x100000; i <<= 1) {
+    char *Ptr = new char;
----------------
These values are not used anywhere.

================
Comment at: llvm/docs/GWPASan.rst:36
+
+GWP-ASan's is only capable of finding a subset of the memory issues detected by
+ASan. Furthermore, GWP-ASan's bug detection capabilities are only probabilistic.
----------------
s/'s//

================
Comment at: llvm/docs/GWPASan.rst:99
+
+In order to increase our detection of overflows, we generally align each sampled
+allocation to the right hand side of the guard page. This means that a 1-byte
----------------
This paragraph is outdated.

================
Comment at: llvm/docs/GWPASan.rst:114
+Please note that the use-after-free detection for a guarded allocation is only
+emphemeral. We reuse inaccessible slots on-demand as we have a fixed number of
+them, and wish to avoid starving the allocation pool to solely catch
----------------
nit: ephemeral

================
Comment at: llvm/docs/GWPASan.rst:146
+  to be parsed. Options defined this way will override any definition made
+  through ``__gwp_asandefault_options``.
+
----------------
nit: missing _?

================
Comment at: llvm/docs/GWPASan.rst:173
+| RightAlignAll          |  false             |  Should we right-align all allocations? By default (false), we left-align       |
+|                        |                    |  even-sized allocations, and right-align odd-sized allocations.                 |
++------------------------+--------------------+---------------------------------------------------------------------------------+
----------------
nit: outdated

================
Comment at: llvm/docs/GWPASan.rst:175
++------------------------+--------------------+---------------------------------------------------------------------------------+
+| AlignmentStrategy      |  PowerOfTwo        |  When allocations are right-aligned, should we perfectly align them up to the   |
+|                        |                    |  page boundary? By default (false), we round up allocation size to the nearest  |
----------------
nit: delete

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593