[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.
Matt Morehouse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 24 18:18:25 PDT 2019
morehouse added inline comments.
================
Comment at: compiler-rt/CMakeLists.txt:281
+option(GWP_ASAN_SCUDO_HOOKS
+ "When building scudo, should we install GWP-ASan hooks (default=False)?" OFF)
+pythonize_bool(GWP_ASAN_SCUDO_HOOKS)
----------------
I think the "(default=False)" is unnecessary since the default value is clearly "OFF".
Also, shouldn't this go in the Scudo cmake?
================
Comment at: compiler-rt/cmake/config-ix.cmake:674
+# is planned for these platforms. Darwin is also not supported due to potential
+# TLS allocations on first-use.
+if (COMPILER_RT_HAS_SANITIZER_COMMON AND GWP_ASAN_SUPPORTED_ARCH AND
----------------
I think "TLS calling malloc on first-use" is clearer.
================
Comment at: compiler-rt/cmake/config-ix.cmake:675
+# TLS allocations on first-use.
+if (COMPILER_RT_HAS_SANITIZER_COMMON AND GWP_ASAN_SUPPORTED_ARCH AND
+ OS_NAME MATCHES "Android|Linux")
----------------
Does this mean GWP-ASan requires sanitizer_common to be present?
================
Comment at: compiler-rt/lib/gwp_asan/CMakeLists.txt:25
+# environment variable GWP_ASAN_FLAGS, but the allocator can choose to implement
+# its own flag parsing and populate the Options struct themselves.
+set(GWP_ASAN_FLAG_PARSER_SOURCES
----------------
s/themselves/itself
================
Comment at: compiler-rt/lib/gwp_asan/CMakeLists.txt:60
+ # include the sanitizer_common flag parsing object lib
+ # 'RTSanitizerCommonNoTermination'.
+ add_compiler_rt_object_libraries(RTGwpAsanFlagParser
----------------
What does RTSanitizerCommonNoTermination do?
================
Comment at: compiler-rt/lib/gwp_asan/allocation_metadata.h:1
+//===-- allocation_metadata.h ---------------------------------------------===//
+//
----------------
Only `GuardedPoolAllocator` ever uses `AllocationMetadata`, and it is only used privately. Since `AllocationMetadata` is small, why not put it directly in `guarded_pool_allocator.h`?
================
Comment at: compiler-rt/lib/gwp_asan/allocation_metadata.h:25
+ // we did not collect a trace.
+ uintptr_t Trace[MaximumStackFrames] = { 0lu };
+ // The thread ID for this trace, or UINT64_MAX if not available.
----------------
Nit: the `lu` seems unnecessary here. In fact the 0 is unnecessary as well. We could just have `= {};`
================
Comment at: compiler-rt/lib/gwp_asan/allocation_metadata.h:39
+ // Whether this allocation has been deallocated yet.
+ bool IsDeallocated = false;
+};
----------------
Probably don't need this. Just set `DeallocationTrace[0] = 0` on alloc.
================
Comment at: compiler-rt/lib/gwp_asan/allocation_metadata.h:41
+};
+}; // namespace gwp_asan
+
----------------
Unnecessary semicolon.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:23
+ SingletonPtr = this;
+
+ // Note: We return from the constructor here if GWP-ASan is not available.
----------------
We should crash with an error if SingletonPtr is already set.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:43
+ std::size_t BytesRequired = NumGuardedSlots * sizeof(AllocationMetadata);
+ BytesRequired += PageSize - (BytesRequired % PageSize);
+ Metadata = reinterpret_cast<AllocationMetadata *>(mapMemory(BytesRequired));
----------------
This rounding is unnecessary unless we're actually using the extra memory. (Below as well)
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:62
+ else
+ __atomic_store_n(&AdjustedSampleRate, 1, __ATOMIC_RELAXED);
+
----------------
What if `Opts.SampleRate == 0`?
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:76
+ __ATOMIC_RELAXED);
+ __atomic_store_n(&IsInitialised, true, __ATOMIC_RELAXED);
+}
----------------
I think we need to hold `PoolMutex` during `init`. Otherwise we have no guarantees that everything else is initialized once `IsInitialized = true` (instructions may be executed out-of-order).
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:90
+ uintptr_t Ptr = slotToAddr(Index);
+ markReadWrite(reinterpret_cast<void *>(Ptr), maximumAllocationSize());
+
----------------
Suppose `Size <= PageSize < maximumAllocationSize()`. In this case, we can `mprotect` a single page rather than the whole slot to improve bug-detection.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:104
+ Meta->AllocationTrace.Trace[0] = 0lu;
+ Meta->DeallocationTrace.Trace[0] = 0lu;
+
----------------
`lu` is unnecessary.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:142
+ assert(pointerIsMine(reinterpret_cast<void *>(Ptr)));
+ return Metadata + addrToSlot(Ptr);
+}
----------------
Nit: I think `return &Metadata[addrToSlot(Ptr)]` is a little clearer.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:155
+uintptr_t GuardedPoolAllocator::getPageAddr(uintptr_t Ptr) const {
+ return Ptr & ~(static_cast<uintptr_t>(PageSize) - 1);
+}
----------------
This rounds down to page size, not slot size. So we will have unintended behavior when `AllocSize < PageSize < SlotSize` and the alloc is right-aligned.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:159
+bool GuardedPoolAllocator::isGuardPage(uintptr_t Ptr) const {
+ return ((Ptr - GetGuardedPagePool()) / PageSize) % 2 == 0;
+}
----------------
Only works if `PageSize == SlotSize`.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:196
+ else if (Size > 8 && (Size & 15) != 0)
+ Size += 16 - (Size & 15);
+ }
----------------
This logic would be clearer if we use `% 16` instead of `& 15`.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:206
+ if (SingletonPtr == nullptr)
+ exit(EXIT_FAILURE);
+ SingletonPtr->reportErrorAndDieInternal(AccessPtr, Error);
----------------
We should have an error message so the user knows what happened.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:213
+ return 0;
+ else if (Ptr > GetGuardedPagePoolEnd() - PageSize)
+ return NumGuardedSlots - 1;
----------------
Nit: else is unnecessary.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:237
+ bool SlotRoundedDown = (Meta->Addr < AccessPtr);
+ if (SlotRoundedDown)
+ *Error = GwpAsanError::BUFFER_OVERFLOW;
----------------
Simpler: `if (Meta->Addr < AccessPtr)`
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:243
+ return Meta;
+ } else {
+ // Access wasn't a guard page, check for use-after-free.
----------------
Nit: else is unnecessary
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:262
+ case GuardedPoolAllocator::GwpAsanError::UNKNOWN:
+ (*Printf)(
+ "The cause of the error is indeterminate. GWP-ASan couldn't "
----------------
Nit: I think `Printf(...)` suffices.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:267
+ "itself. It was likely either caused by a memory bug for a non-sampled "
+ "allocation, or a wild memory access into the GWP-ASan pool.\n");
+ (*Printf)("When trying to access memory at: 0x%zx\n", AccessPtr);
----------------
This error message is verbose. The first two sentences say the same thing, and I don't think we need to mention the possibility of bugs in GWP-ASan.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:268
+ "allocation, or a wild memory access into the GWP-ASan pool.\n");
+ (*Printf)("When trying to access memory at: 0x%zx\n", AccessPtr);
+ exit(EXIT_FAILURE);
----------------
The message might be a little confusing. What happened "when trying to access memory"?
Also, let's combine this with the previous printf.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:338
+ if (!pointerIsMine(reinterpret_cast<void *>(AccessPtr))) {
+ Error = GwpAsanError::UNKNOWN;
+ } else if (Error == GwpAsanError::UNKNOWN) {
----------------
Hmm... So any segfault will trigger a GWP-ASan report, even if its very far from the guarded region?
================
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();
+ }
----------------
vlad.tsyrklevich wrote:
> 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.)
Perhaps we can require that GPA has been initialized prior to calling `pointerIsMine`.
Scudo has some code like
```
allocate() {
initScudoIfNecessary();
...
}
```
If we ensure that GPA is initialized inside `initScudo`, we guarantee that `PointerIsMine` is only ever called after `GPA::Init`.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:26
+public:
+ enum class GwpAsanError {
+ UNKNOWN,
----------------
`GwpAsan` seems superfluous since we're either using this inside `GuardedPoolAllocator`, or we're fully-qualifying as `GuardedPoolAllocator::GwpAsanError::*`.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:42
+ constexpr GuardedPoolAllocator(){};
+ GuardedPoolAllocator(const GuardedPoolAllocator &) = delete;
+
----------------
Also delete `operator=`.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:67
+ return NextSampleCounter-- == 1 &&
+ __atomic_load_n(&IsInitialised, __ATOMIC_RELAXED);
+ }
----------------
Can perf be improved by marking the `== 1` branch as `UNLIKELY`?
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:113
+ void markReadWrite(void *Ptr, std::size_t Size) const;
+ void markInaccessible(void *Ptr, std::size_t Size) const;
+
----------------
Can these be static?
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:121
+ // be called once, and the result should be cached in PageSize in this class.
+ std::size_t getPlatformPageSize();
+
----------------
static?
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:142
+
+ // Returns a pointer to the N-th guarded slot.
+ uintptr_t slotToAddr(std::size_t N) const;
----------------
s/a pointer to/the address of
Technically we're not returning a pointer.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:152
+
+ // Gets the nearest slot the the provided address.
+ std::size_t getNearestSlot(uintptr_t Ptr) const;
----------------
s/the the/to the
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:162
+ // slots are available.
+ bool reserveSlot(std::size_t *PageIndex);
+
----------------
`ssize_t reserveSlot()` seems cleaner to me. Also the function comment could be reworded clearer.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:171
+ // slot in which the allocation should live.
+ void applyAllocationStrategy(std::size_t Size, uintptr_t *AllocationPtr);
+
----------------
Confusing interface and comment. How about `uintptr_t AlignAllocation(size_t SlotIdx, size_t Size)`?
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:177
+ // slot.
+ void tryLockAllMutexes();
+
----------------
There's only one mutex, so maybe just do the try-lock inline.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:183
+ // resulted in GwpAsanError::INVALID_FREE or GwpAsanError::UNKNOWN.
+ AllocationMetadata *diagnoseUnknownError(uintptr_t AccessPtr,
+ GwpAsanError *Error);
----------------
The name `diagnoseUnknownError` suggests that the error type is being returned. I'd suggest a signature like this:
```
GwpAsanError diagnoseError(uintptr_t AccessPtr, AllocationMetadata *ErrorMetadata)
```
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:189
+
+private:
+ // Whether this class has been initialised. This means the class is ready to
----------------
Unnecessary private.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:203
+ // Record the number of currently used slots. We store this amount so that we
+ // don't potentially randomly choose to recycle a slot that previously had an
+ // allocation before all the slots have been utilised.
----------------
nit: s/potentially//
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:233
+ // recalcualting the sample rate.
+ ALIGNED(8) uint64_t AdjustedSampleRate = UINT64_MAX - 1;
+ // Thread-local decrementing counter that indicates that a given allocation
----------------
Why not `UINT64_MAX`?
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:236
+ // should be sampled when it reaches zero.
+ static TLS_INITIAL_EXEC uint64_t NextSampleCounter;
+
----------------
Nit: static is unnecessary.
================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:246
+ "GuardedPoolAllocator must be trivially destructible.");
+}; // namespace gwp_asan
+
----------------
Unnecessary semicolon.
================
Comment at: compiler-rt/lib/gwp_asan/options.inc:21
+ "for performance reasons. Setting this to true can find single byte "
+ "buffer-overflows at the cost of performance.")
+
----------------
We should also mention that perfect-right-alignment will break on some architectures.
================
Comment at: compiler-rt/lib/gwp_asan/options.inc:28
+GWP_ASAN_OPTION(
+ unsigned long, SampleRate, 5000,
+ "The probability (1 / SampleRate) that an allocation is selected for "
----------------
I think this needs to be `uint64` or `unsigned long long`.
================
Comment at: compiler-rt/lib/gwp_asan/options.inc:30
+ "The probability (1 / SampleRate) that an allocation is selected for "
+ "GWP-ASan sampling. Default is 5000. Sample rates up to 1 / (2^63) are "
+ "supported.")
----------------
Replace 1 / (2^63) with 2^63
================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:305
+#ifdef GWP_ASAN_HOOKS
+ if (UNLIKELY(GuardedAlloc.shouldSample())) {
+ if (void *Ptr = GuardedAlloc.allocate(Size))
----------------
What does the assembly for this look like on the fast path?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60593/new/
https://reviews.llvm.org/D60593
More information about the llvm-commits
mailing list