[PATCH] D20084: [sanitizer] Initial implementation of a Hardened Allocator

Thu May 12 14:21:32 PDT 2016

cryptoad added inline comments.

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_allocator.cc:87
@@ +86,3 @@
+  // 2nd 8 bytes
+  u64 offset         : 24;
+  u64 unused_1_      : 24;
----------------
dvyukov wrote:
> Don't we need only 20 bits here?
This is indeed the case.
I figured I would align that one to a multiple of 8 bits as we have some space in the second half of the 128-bit integer.
I am not opposed to shortening to the actual needed bit size if you feel strongly about it.

================
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);
----------------
filcab wrote:
> Why are you not using the C++11 atomics?
Using std::atomic<unsigned __int128>?

================
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));
----------------
filcab wrote:
> Why?
Sorry this was a remainder of a debugging session. I switched it back to the original plan which was to use the thread_local QuarantineCache.

================
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;
----------------
filcab wrote:
> 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.
No plan to support 32-bit as of yet, but yes we will use FIRST_32_SECOND_64 if we do.

================
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
----------------
filcab wrote:
> Pobably best to zero the whole thing (`needed_size - ChunkHeaderSize`)?
I guess we have several choices here:

  - move it up and zero the whole thing prior to the header being store

  - leave it here and zero the whole thing post header, which will have to account for the offset (needed_size - (chunk_beg - alloc_beg))

I think the first option would be better.

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_rtl.cc:50
@@ +49,3 @@
+#else
+#error "Can't use .preinit_array"
+#endif
----------------
filcab wrote:
> Should this be an `#error`?
I figured the additional initialization techniques used by ASan et al. could be added later on. Hence the #error for now.

================
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)
----------------
glider wrote:
> Do you really need the subleaf parameter now?
I figured it could be useful if a feature such as RDSEED was needed.

================
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()
+{
----------------
glider wrote:
> I believe requiring certain Intel CPU models in order for the allocator to work isn't a good idea.
My point of view when writing this was that I had to be as competitive as can be with other allocators, so that the benefit of additional checks would not be offseted by a dramatic decrease in performances. In the initial stages, it was determined that a BSD checksum vs the CPU backed CRC32 induced a performance gain of about 10% in pure allocation benchmarks, so I went that way.

I am not opposed to doing something purely software, but I'd rather start this way and then expend it to be more portable.

================
Comment at: projects/compiler-rt/lib/hardened_allocator/scudo_utils.cc:84
@@ +83,3 @@
+static u64 RdRand64() {
+  static s8 kHasRdRand = -1;
+  if (kHasRdRand == -1) {
----------------
glider wrote:
> Why not use a bool here?
I wanted to use a 3 state variable: unintialized, true, false. Hence the -1, 0, 1.

================
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));
----------------
filcab wrote:
> Nit: Why not a simple `int`? Closer to the "usual idiom" in C++.
I tried to be consistent using the Sanitizer types. But I see your point.

================
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();
----------------
dvyukov wrote:
> glider wrote:
> > glider wrote:
> > > 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?
> > As a data point, I've ran RdTSC() and std::chrono::high_resolution_clock::now().time_since_epoch().count() 200278017 times.
> > The number of unique values of both variables was exactly 200278017, while the number of unique XOR values was only 200205416, i.e. there were 0.036% collisions.
> This is used to initialize global cookie. I would use /dev/random. Or there must be 16 bytes of good randomness in auxv.
Is your suggestion to get rid of the epoch component?

================
Comment at: projects/compiler-rt/test/hardened_allocator/sizes.cc:53
@@ +52,3 @@
+
+// CHECK: allocator is terminating the process
+
----------------
glider wrote:
> Where does this line come from? I don't see the allocator printing it anywhere.
This is from sanitizer_allocator.cc, that handles this condition.

http://reviews.llvm.org/D20084