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

[Kostya Kortchinsky via llvm-commits]

cryptoad added inline comments.

================
Comment at: lib/hardened_allocator/scudo_allocator.cc:123
@@ +122,3 @@
+    crc = _mm_crc32_u64(crc, header_holder[1]);
+    return static_cast<u16>(crc ^ cookie);
+  }
----------------
aizatsky wrote:
> This uses this.cookie. Is this intended?
I am not sure what you mean here, as there is no cookie member to the struct. Could you please clarify?

================
Comment at: lib/hardened_allocator/scudo_allocator.cc:141
@@ +140,3 @@
+  // A different one would mean that another thread would have raced us.
+  void StoreHeader(UnpackedHeader *new_unpacked_header,
+                   UnpackedHeader *old_unpacked_header) {
----------------
aizatsky wrote:
> Instead of having optional parameter, maybe its better to split this function into two? One simple copying, another - verify and copy.
I liked having a single one to avoid duplication, but I can see the interest in having two. I can change that if you feel strongly about it.

================
Comment at: lib/hardened_allocator/scudo_allocator.cc:440
@@ +439,3 @@
+  // Reallocates a chunk. We can save on a new allocation if the new requested
+  // size still fits in the chunk.
+  void *Reallocate(void *old_ptr, uptr new_size) {
----------------
kcc wrote:
> Are you sure we want to save an alloc? 
> Does this actually improve security, or weaken it? 
> (I don't know, just asking)
That is a valid point. I haven't thought about the security implications of reallocating in place when the size allows. I will look into it.

================
Comment at: lib/hardened_allocator/scudo_allocator.cc:331
@@ +330,3 @@
+    header.requested_size = size;
+    header.salt = static_cast<u16>(prng.Next());
+    chunk->StoreHeader(&header, nullptr);
----------------
glider wrote:
> Is the salt value used anywhere? If not, what's its point?
The salt will be part of the data checksumed.
This allows a chunk to not always have the same checksum even if it's "metadata" is identical.
On the downside, tampering with the salt might be used to do a compensation attack.
I felt the benefit outweighed the disadvantage, but it's debatable.

================
Comment at: lib/hardened_allocator/scudo_allocator.h:25
@@ +24,3 @@
+// CheckFailedCallback function, which could be abused by a potential attacker.
+#ifdef CHECK_IMPL
+#undef CHECK_IMPL
----------------
kcc wrote:
> We shouldn't be doing this. 
> Instead, please send a separate change to replace all CHECK_* inside the allocator code
> with something like ALLOCATOR_CHECK that will invoke a method that you can redefine using C++ mechanisms (template parameters or virtual functions). 
> Since this is not a hot code, virtual functions will work, but given that we don't use them anywhere else in the allocator, template parameter is preferred. 
> 
Will do.

================
Comment at: lib/hardened_allocator/scudo_utils.h:39
@@ +38,3 @@
+
+// Tiny PRNG based on https://en.wikipedia.org/wiki/Xorshift#xorshift.2B
+// The state (128 bits) will be stored in thread local storage
----------------
glider wrote:
> Note that Xorshift isn't cryptographically secure, and may be easy to predict.
I am OK with the PRNG not being cryptographically secure, its purpose is to add some randomness to the process.


http://reviews.llvm.org/D20084