[PATCH] D20084: [sanitizer] Initial implementation of a Hardened Allocator
Filipe Cabecinhas via llvm-commits
llvm-commits at lists.llvm.org
Thu May 12 09:50:24 PDT 2016
filcab added a subscriber: filcab.
================
Comment at: docs/HardenedAllocator.rst:47
@@ +46,3 @@
+The checksum is computed as a CRC32 (if the associated CPU instructions are
+available) of the chunk pointer itself, and the 16 bytes of header with the
+checksum field zeroed out. The result is then xored with a global secret.
----------------
I think this parenthesis should be somewhere else. The crc32 instruction is an actual requirement of the allocator right now.
P.S: Alternatively, remove the "if ..." and say it's a requirement, like you do in the next paragraph.
================
Comment at: docs/HardenedAllocator.rst:110
@@ +109,3 @@
+- zero_chunk_contents (boolean, defaults to false): whether or not we zero
+ chunk contents on allocation and deallocation.
+
----------------
If we're making a "hardened allocator", shouldn't the default be to zero out all chunks?
(of course, performance would suffer a lot. I'm just curious if there are other reasons)
================
Comment at: projects/compiler-rt/cmake/config-ix.cmake:196
@@ -195,2 +195,3 @@
set(ALL_ESAN_SUPPORTED_ARCH ${X86_64})
+set(ALL_HARDENED_ALLOCATOR_SUPPORTED_ARCH ${X86_64})
----------------
I know safestack, cfi, and ESan don't follow this, but we can probably put the HARDENED_ALLOCATOR stuff in alphabetical order with the other stuff above.
================
Comment at: projects/compiler-rt/lib/CMakeLists.txt:58
@@ -55,1 +57,3 @@
+ add_subdirectory(hardened_allocator)
+ endif()
endif()
----------------
Same here.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:32
@@ +31,3 @@
+void NORETURN Die() {
+ // TODO(kostyak): do we want to be able to abort?
+ if (common_flags()->abort_on_error)
----------------
I would remove the TODO.
If we're in a debugger, Abort() might trigger the debugger and stop the program execution. exit() will simply close the program. We've added the abort in ASan, back in the day (D12332), due to this.
It's the default for ASan on OS X and on the PS4.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:48
@@ +47,3 @@
+
+// TODO(kostyak): currently we have one Prng per thread, is it necessary?
+static thread_local Xorshift128Plus Prng;
----------------
It's very likely a smaller price to pay than to have to use atomic updates + spin on update collisions.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:60
@@ +59,3 @@
+typedef unsigned __int128 u128;
+typedef u128 PackedHeader;
+
----------------
I could do without the `u128` typedef. You're only using it for the `PackedHeader` typedef.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:93
@@ +92,3 @@
+COMPILER_CHECK(sizeof(UnpackedHeader) == sizeof(u128));
+COMPILER_CHECK(sizeof(PackedHeader) == sizeof(u128));
+
----------------
I'd suggest:
COMPILER_CHECK(sizeof(UnpackedHeader) == sizeof(PackedHeader));
Since it makes it explicit we want those two to be the same and that it's not a coincidence that we're expecting both to "happen to be" the same size as `u128`.
The `sizeof(PackedHeader)` isn't needed, since it's a typedef for `u128` (even after my proposed change, it won't be needed).
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:95
@@ +94,3 @@
+
+static const uptr ChunkHeaderSize = sizeof(PackedHeader);
+
----------------
Remove the `static` here, since you're not adding it to other const-qualified variables when it isn't needed.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:117
@@ +116,3 @@
+ // TODO(kostyak): use a BSD checksum for the non-sse4.2 processors?
+ __attribute__((target("sse4.2")))
+ u16 Checksum(UnpackedHeader *header) const {
----------------
No need to do this. Much better to just handle it on the CMake side (which you're already doing).
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:123
@@ +122,3 @@
+ // This is somewhat of a shortcut. The checksum is stored in the 16 least
+ // significant bits of the header, hence zero-ing those bits out. It would
+ // be more valid to zero the checksum field of the UnpackedHeader, but
----------------
"... 16 least significant bits of the header of the first 8 bytes..."
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:134
@@ +133,3 @@
+ PackedHeader packed_header;
+ __atomic_load(reinterpret_cast<const PackedHeader *>(this), &packed_header,
+ __ATOMIC_ACQUIRE);
----------------
Why are you not using the C++11 atomics?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:242
@@ +241,3 @@
+// static thread_local QuarantineCache quarantine_cache;
+static THREADLOCAL uptr quarantine_cache[4] = {};
+COMPILER_CHECK(sizeof(QuarantineCache) <= sizeof(quarantine_cache));
----------------
Why?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:262
@@ +261,3 @@
+struct Allocator {
+ static const uptr MaxAllowedMallocSize = 1ULL << 40;
+ static const uptr MaxThreadLocalQuarantine = 1U << 20;
----------------
glider wrote:
> Are we going to target 32-bit systems? This is gonna overflow uptr on x86.
If this ends up being ported, it's a simple matter of using the `FIRST_32_SECOND_64` macro.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:278
@@ +277,3 @@
+ void Initialize(const AllocatorOptions &options) {
+ CHECK(TestCPUFeature(SSE4_2)); // for crc32
+ DeallocationTypeMismatch = options.DeallocationTypeMismatch;
----------------
glider wrote:
> Despite SSE 4.2 may be quite common at Google, I don't think it's a good idea to bail out if it's unsupported.
> Note TestCPUFeature() doesn't work on AMD processors yet.
Source files are compiled assuming that feature is available.
We'll have to add a fallback checksum (plus change build and this check) to address this comment.
I would be ok with keeping the SSE4.2 requirement until we get a non-zero amount of requests/bug reports.
P.S: http://store.steampowered.com/hwsurvey (first result for "hardware survey". It's clearly biased, but I'd guess developer CPUs are also biased to be more recent/powerful than an average computer) puts SSE4.2 adoption at ~80%.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:334
@@ +333,3 @@
+ if (ZeroContents && allocator.FromPrimary(ptr))
+ memset(user_ptr, 0, size);
+ // TODO(kostyak): hooks sound like a terrible idea security wise but might
----------------
Pobably best to zero the whole thing (`needed_size - ChunkHeaderSize`)?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:373
@@ +372,3 @@
+ if (!(new_header.alloc_type == FromMemalign &&
+ alloc_type == FromMalloc)) {
+ Printf("ERROR: allocation type mismatch on address %p\n", chunk);
----------------
Please push the negation through.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:449
@@ +448,3 @@
+ if (new_size <= usable_size) {
+ // TODO(kostyak): zero the additional contents
+ new_header.requested_size = new_size;
----------------
I'm guessing you only need to zero the additional contents to account for possible overflows that might have happened.
Otherwise:
`ZeroContents` = true -> they're already zeroed
`ZeroContents` = false -> No need to zero.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:473
@@ +472,3 @@
+ void *ptr = Allocate(total, MinAlignment, FromMalloc);
+ if (ptr && allocator.FromPrimary(ptr))
+ memset(ptr, 0, total);
----------------
If we have the `ZeroContents`, no need to zero again.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:474
@@ +473,3 @@
+ if (ptr && allocator.FromPrimary(ptr))
+ memset(ptr, 0, total);
+ return ptr;
----------------
Should we zero the whole block, instead of just the size we were asked?
(Hardening it a tiny bit against overflows)
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.h:41
@@ +40,3 @@
+ } while (false) \
+/**/
+
----------------
Why the empty comment trailing the `while (false)`?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_malloc_linux.cc:1
@@ +1,2 @@
+//===-- scudo_malloc_linux.cc -----------------------------------*- C++ -*-===//
+//
----------------
`scudo_interceptors.cc` (`.cpp` in the future, but do what Vitaly suggested and change the names only after approval to ease code review)
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_malloc_linux.cc:15
@@ +14,3 @@
+#include "sanitizer_common/sanitizer_platform.h"
+#if SANITIZER_LINUX
+
----------------
Don't add these to the whole file.
I'm ok with protecting Linux/glibc-specific functions like `pvalloc`, etc. Those will need it anyway if this gets ported somewhere.
No need to protect the `malloc`/`free` interceptors with `SANITIZER_LINUX`, though.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_rtl.cc:50
@@ +49,3 @@
+#else
+#error "Can't use .preinit_array"
+#endif
----------------
Should this be an `#error`?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:59
@@ +58,3 @@
+ if (IsIntelCPU() == true)
+ cpuid(&kCPUInfo, 1, 0);
+ kInfoInitialized = true;
----------------
else
UNIMPLEMENTED();
(or something similar)
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:89
@@ +88,3 @@
+ if (kHasRdRand == 1) {
+ register u64 rnd;
+ u8 carry;
----------------
Does gcc actually do anything with this?
If not, then just delete it. AFAICT, clang doesn't care unless you have an asm attribute to tie it to a specific register.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:98
@@ +97,3 @@
+ // If the first attempt failed, we fall back to retries.
+ for (s32 c = 10; c != 0; --c) {
+ asm volatile("rdrand %0; setc %1": "=r" (rnd), "=qm" (carry));
----------------
Nit: Why not a simple `int`? Closer to the "usual idiom" in C++.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.h:26
@@ +25,3 @@
+ typedef char VerifySizesAreEqual[sizeof(Dest) == sizeof(Source) ? 1 : -1]
+ UNUSED;
+ Dest dest;
----------------
`static_assert(sizeof(Dest) == sizeof(Source), "Sized are not equal!");`
================
Comment at: projects/compiler-rt/test/hardened_allocator/double-free.cc:28
@@ +27,3 @@
+ }
+ return 0;
+}
----------------
Add a `posix_memalign` version. We have a special case in `free` for it.
================
Comment at: projects/compiler-rt/test/hardened_allocator/mismatch.cc:21
@@ +20,3 @@
+ free((void *)p);
+ }
+ return 0;
----------------
You should add, at least, the memalign -> something_other_than_free case, since it's a special case.
================
Comment at: projects/compiler-rt/test/hardened_allocator/quarantine.cc:15
@@ +14,3 @@
+ p = malloc(size);
+ if (p == nullptr)
+ return 1;
----------------
glider wrote:
> You should probably add nullptr checks to other allocations in other tests.
`if (p)`
http://reviews.llvm.org/D20084
More information about the llvm-commits
mailing list