[PATCH] D20084: [sanitizer] Initial implementation of a Hardened Allocator
Alexander Potapenko via llvm-commits
llvm-commits at lists.llvm.org
Thu May 12 05:30:26 PDT 2016
glider added inline comments.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:13
@@ +12,3 @@
+/// heap corruption vulnerabilities. It provides a checksum-guarded chunk
+/// header, a delayed free list, and various other security improvements.
+///
----------------
Please either elaborate what other "various security improvements" are there, or remove that phrase.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:115
@@ +114,3 @@
+ // CRC32 checksum of the Chunk pointer and its ChunkHeader.
+ // It currently uses the Intel Nehalem SSE4.2 crc32 64-bit instruction.
+ // TODO(kostyak): use a BSD checksum for the non-sse4.2 processors?
----------------
I would've started with a handwritten crc32 implementation and bother with hardware support only iff it's performance-critical (don't think it is)
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:161
@@ +160,3 @@
+ false,
+ __ATOMIC_ACQUIRE,
+ __ATOMIC_ACQUIRE)) {
----------------
Shouldn't the success memory order be __ATOMIC_RELEASE?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:175
@@ +174,3 @@
+
+static void teardownThread(void *p) {
+ uptr v = reinterpret_cast<uptr>(p);
----------------
Please remind if Scudo is going to be used together with any of the sanitizers.
If yes, the destructor magic won't probably work as intended, because other tools also play with it.
================
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;
----------------
Are we going to target 32-bit systems? This is gonna overflow uptr on x86.
================
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;
----------------
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.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:435
@@ +434,3 @@
+ initThread();
+ // CHECK(old_ptr && new_size); // Redundant with scudo_realloc
+ UnpackedHeader old_header;
----------------
So remove it, maybe?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.h:1
@@ +1,2 @@
+//===-- scudo_allocator.h ---------------------------------------*- C++ -*-===//
+//
----------------
I don't insist much, but I think either the library name should be "scudo" instead of "hardened_allocator", or the names of the files under hardened_allocator/ should start with "hardened_allocator_"
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_flags.inc:29
@@ +28,3 @@
+
+SCUDO_FLAG(bool, ZeroContents, false,
+ "Zero chunk contents on allocation and deallocation.")
----------------
Feature request: filling the chunk context with a nonzero byte.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:1
@@ +1,2 @@
+//===-- scudo_utils.cc ------------------------------------------*- C++ -*-===//
+//
----------------
Do the build configs prevent this file from being built on ARM?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:32
@@ +31,3 @@
+{
+ asm volatile("cpuid"
+ : "=a" (info->eax), "=b" (info->ebx), "=c" (info->ecx), "=d" (info->edx)
----------------
Do you really need the subleaf parameter now?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:39
@@ +38,3 @@
+// Returns true is the CPU is a "GenuineIntel"
+static bool IsIntelCPU()
+{
----------------
I believe requiring certain Intel CPU models in order for the allocator to work isn't a good idea.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:57
@@ +56,3 @@
+
+ if (kInfoInitialized == false) {
+ if (IsIntelCPU() == true)
----------------
Note this is thread-unsafe. Not sure if that matters here, but still.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:84
@@ +83,3 @@
+static u64 RdRand64() {
+ static s8 kHasRdRand = -1;
+ if (kHasRdRand == -1) {
----------------
Why not use a bool here?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:86
@@ +85,3 @@
+ if (kHasRdRand == -1) {
+ kHasRdRand = TestCPUFeature(RDRAND);
+ }
----------------
Usually a "k" prefix denotes a constant. If you're going to change it, that's just a regular variable named "has_rd_rand" (or something like that)
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:93
@@ +92,3 @@
+ // Normally we need only one execution
+ asm volatile("rdrand %0; setc %1": "=r" (rnd), "=qm" (carry));
+ if (carry != 0)
----------------
Please move this call inside the loop below.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:104
@@ +103,3 @@
+
+ // All attempts failed. Log CPU error and abort.
+ Printf("ERROR: CPU error detected during 64-bit RDRAND execution.\n");
----------------
Is this problem that severe that we want to abort?
Note that we don't abort if the CPU doesn't support rdrand.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:109
@@ +108,3 @@
+ std::hash<std::thread::id> hasher;
+ return RdTSC() ^ hasher(std::this_thread::get_id()) ^
+ std::chrono::high_resolution_clock::now().time_since_epoch().count();
----------------
My gut feeling is that XORing rdtsc and the time since epoch is actually reducing the entropy, not increasing it. Any idea if that's true?
Also, do we really need a dependency on std::chrono?
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.h:50
@@ +49,3 @@
+ }
+ void GetSeed(u64 *state_0, u64 *state_1) {
+ *state_0 = state_0_;
----------------
GetSeed is unused.
================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.h:58
@@ +57,3 @@
+ state_0_ = y;
+ x ^= x << 23; // a
+ state_1_ = x ^ y ^ (x >> 17) ^ (y >> 26); // b, c
----------------
These "a", "b", "c" comments don't help :(
Please remove them.
================
Comment at: projects/compiler-rt/test/hardened_allocator/alignment.cc:13
@@ +12,3 @@
+ void *p, *old_p;
+ size_t alignment = 1U << 16, size = 1U << 8;
+
----------------
`alignment` is unused.
================
Comment at: projects/compiler-rt/test/hardened_allocator/init.cc:1
@@ +1,2 @@
+// RUN: %clang_scudo %s -o %t && %run %t
+
----------------
Do you really need this test?
================
Comment at: projects/compiler-rt/test/hardened_allocator/malloc.cc:1
@@ +1,2 @@
+// RUN: %clang_scudo %s -o %t
+// RUN: %run %t 2>&1
----------------
Each test must have comments that describe its purpose.
================
Comment at: projects/compiler-rt/test/hardened_allocator/mismatch.cc:2
@@ +1,3 @@
+// RUN: %clang_scudo %s -o %t
+// RUN: SCUDO_OPTIONS=DeallocationTypeMismatch=1 not %run %t mallocdel 2>&1 | FileCheck %s
+// RUN: SCUDO_OPTIONS=DeallocationTypeMismatch=0 %run %t mallocdel 2>&1
----------------
FYI you can use --check-prefix to write more test-specific CHECK directives.
================
Comment at: projects/compiler-rt/test/hardened_allocator/mismatch.cc:26
@@ +25,2 @@
+// CHECK: ERROR: allocation type mismatch on address
+
----------------
Nit: spare newline
================
Comment at: projects/compiler-rt/test/hardened_allocator/quarantine.cc:15
@@ +14,3 @@
+ p = malloc(size);
+ if (p == nullptr)
+ return 1;
----------------
You should probably add nullptr checks to other allocations in other tests.
================
Comment at: projects/compiler-rt/test/hardened_allocator/realloc.cc:29
@@ +28,3 @@
+ return 1;
+ // And a new one if the size is greater
+ p = realloc(p, size + 1);
----------------
Nit: a comment on a separat line must end with a period.
================
Comment at: projects/compiler-rt/test/hardened_allocator/sizes.cc:19
@@ +18,3 @@
+ if (!strcmp(argv[1], "malloc")) {
+ // Currently the maximum size the allocator can fullfill is 1ULL<<40 bytes
+ size_t size = std::numeric_limits<size_t>::max();
----------------
s/fulfill/allocate?
================
Comment at: projects/compiler-rt/test/hardened_allocator/sizes.cc:53
@@ +52,3 @@
+
+// CHECK: allocator is terminating the process
+
----------------
Where does this line come from? I don't see the allocator printing it anywhere.
http://reviews.llvm.org/D20084
More information about the llvm-commits
mailing list