From de8ef3efc7baa4b803d02a118a00278b7cf712cb Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 15 Jul 2021 18:31:28 +0100 Subject: [PATCH] Cleaner implementation of signed pointers. (#347) * Cleaner implementation of signed pointers. This encodes a back pointer in each node. The back pointer is stored in an encoded form so that it is hard to corrupt and trick the allocator into following incorrect pointers. This changes the encoding from previously being a Feistel network on the next pointer that was using the prev as part of the key, to now effectively using a doubly linked queue, where the back pointers are scrambled, so it is hard to forge them. This has the positive effects of - Not needing to store previous while building the list, as the append nows, curr and next at the point of writing into next, and does not need an additional previous. - The encoding is not affecting the actual next value, so more instructions can be executed in parallel by the CPU. Future extensions, store a changing key in the FreeListBuilder so it becomes harder to try to forge the previous token. This approach can also be applied to the remote list, and will in a subsequent PR. This enables the idea to be tested. * Remove unused header. * Apply suggestions from code review Co-authored-by: Nathaniel Wesley Filardo Co-authored-by: Nathaniel Wesley Filardo --- src/mem/corealloc.h | 2 - src/mem/freelist.h | 394 ++++++++++++++++---------------------------- 2 files changed, 138 insertions(+), 258 deletions(-) diff --git a/src/mem/corealloc.h b/src/mem/corealloc.h index 6bf140f..e307fb1 100644 --- a/src/mem/corealloc.h +++ b/src/mem/corealloc.h @@ -9,8 +9,6 @@ #include "sizeclasstable.h" #include "slaballocator.h" -#include - namespace snmalloc { template diff --git a/src/mem/freelist.h b/src/mem/freelist.h index 37809b4..4813791 100644 --- a/src/mem/freelist.h +++ b/src/mem/freelist.h @@ -1,7 +1,36 @@ #pragma once /** * This file encapsulates the in disused object free lists - * that are used per slab of small objects. + * that are used per slab of small objects. The implementation + * can be configured to introduce randomness to the reallocation, + * and also provide signing to detect free list corruption. + * + * # Corruption + * + * The corruption detection works as follows + * + * FreeObject + * ----------------------------- + * | next | prev_encoded | ... | + * ----------------------------- + * A free object contains a pointer to next object in the free list, and + * a prev pointer, but the prev pointer is really a signature with the + * following property + * + * If n = c->next && n != 0, then n->prev_encoded = f(c,n). + * + * If f just returns the first parameter, then this degenerates to a doubly + * linked list. (Note that doing the degenerate case can be useful for + * debugging snmalloc bugs.) By making it a function of both pointers, it + * makes it harder for an adversary to mutate prev_encoded to a valid value. + * + * This provides protection against the free-list being corrupted by memory + * safety issues. + * + * # Randomness + * + * The randomness is introduced by building two free lists simulatenously, + * and randomly deciding which list to add an element to. */ #include "../ds/address.h" @@ -12,100 +41,43 @@ namespace snmalloc { - static constexpr std::size_t PRESERVE_BOTTOM_BITS = 30; - - static inline bool different_slab(address_t p1, address_t p2) - { - return (p1 ^ p2) >= bits::one_at_bit(PRESERVE_BOTTOM_BITS); - } - - template - static inline bool different_slab(address_t p1, CapPtr p2) - { - return different_slab(p1, address_cast(p2)); - } - - template - static inline bool - different_slab(CapPtr p1, CapPtr p2) - { - return different_slab(address_cast(p1), address_cast(p2)); - } - - class FreeObject; - - class EncodeFreeObjectReference - { - CapPtr reference; - - /** - * On architectures which use IntegerPointers, we can obfuscate our free - * lists and use this to drive some probabilistic checks for integrity. - * - * There are two definitions of encode() below, which use std::enable_if_t - * to gate on do_encode. - */ -#ifndef CHECK_CLIENT - static constexpr bool do_encode = false; -#else - static constexpr bool do_encode = aal_supports; -#endif - - public: -#ifdef CHECK_CLIENT - template - static std::enable_if_t> encode( - uint32_t local_key, CapPtr next_object, LocalEntropy& entropy) - { - // Simple involutional encoding. The bottom half of each word is - // multiplied by a function of both global and local keys (the latter, - // in practice, being the address of the previous list entry) and the - // resulting word's top part is XORed into the pointer value before it - // is stored. - auto next = address_cast(next_object); - constexpr address_t MASK = bits::one_at_bit(PRESERVE_BOTTOM_BITS) - 1; - // Mix in local_key - address_t p1 = local_key + entropy.get_constant_key(); - address_t p2 = (next & MASK) - entropy.get_constant_key(); - next ^= (p1 * p2) & ~MASK; - return CapPtr(reinterpret_cast(next)); - } -#endif - - template - static std::enable_if_t> encode( - uint32_t local_key, CapPtr next_object, LocalEntropy& entropy) - { - UNUSED(local_key); - UNUSED(entropy); - return next_object; - } - - void store( - CapPtr value, - uint32_t local_key, - LocalEntropy& entropy) - { - reference = encode(local_key, value, entropy); - } - - CapPtr read(uint32_t local_key, LocalEntropy& entropy) - { - return encode(local_key, reference, entropy); - } - }; - struct Remote; + + /** + * This function is used to sign back pointers in the free list. + * + * TODO - Needs review. Should we shift low bits out as they help + * guess the key. + * + * TODO - We now have space in the FreeListBuilder for a fresh key for each + * list. + */ + inline static uintptr_t + signed_prev(address_t curr, address_t next, LocalEntropy& entropy) + { + auto c = curr; + auto n = next; + auto k = entropy.get_constant_key(); + return (c + k) * (n - k); + } + /** * Free objects within each slab point directly to the next. - * The next_object pointer can be encoded to detect - * corruption caused by writes in a UAF or a double free. + * There is an optional second field that is effectively a + * back pointer in a doubly linked list, however, it is encoded + * to prevent corruption. */ class FreeObject { - public: - EncodeFreeObjectReference next_object; + CapPtr next_object; +#ifdef CHECK_CLIENT + // Encoded representation of a back pointer. + // Hard to fake, and provides consistency on + // the next pointers. + address_t prev_encoded; +#endif + public: static CapPtr make(CapPtr p) { return p.template as_static(); @@ -121,11 +93,45 @@ namespace snmalloc } /** - * Read the next pointer handling any required decoding of the pointer + * Assign next_object and update its prev_encoded if CHECK_CLIENT. + * + * Static so that it can be used on reference to a FreeObject. + * + * Returns a pointer to the next_object field of the next parameter as an + * optimization for repeated snoc operations (in which + * next->next_object is nullptr). */ - CapPtr read_next(uint32_t key, LocalEntropy& entropy) + static CapPtr* store( + CapPtr* curr, + CapPtr next, + LocalEntropy& entropy) { - return next_object.read(key, entropy); + *curr = next; +#ifdef CHECK_CLIENT + next->prev_encoded = + signed_prev(address_cast(curr), address_cast(next), entropy); +#else + UNUSED(entropy); +#endif + return &(next->next_object); + } + + /** + * Check the signature of this FreeObject + */ + void check_prev(address_t signed_prev) + { + UNUSED(signed_prev); + check_client( + signed_prev == prev_encoded, "Heap corruption - free list corrupted!"); + } + + /** + * Read the next pointer + */ + CapPtr read_next() + { + return next_object; } }; @@ -136,30 +142,19 @@ namespace snmalloc */ class FreeListIter { - CapPtr curr{1}; + CapPtr curr{nullptr}; #ifdef CHECK_CLIENT address_t prev{0}; #endif - uint32_t get_prev() - { -#ifdef CHECK_CLIENT - return prev & 0xffff'ffff; -#else - return 0; -#endif - } - public: constexpr FreeListIter( CapPtr head, address_t prev_value) : curr(head) -#ifdef CHECK_CLIENT - , - prev(prev_value) -#endif { - // SNMALLOC_ASSERT(head != nullptr); +#ifdef CHECK_CLIENT + prev = prev_value; +#endif UNUSED(prev_value); } @@ -170,7 +165,7 @@ namespace snmalloc */ bool empty() { - return (address_cast(curr) & 1) == 1; + return curr == nullptr; } /** @@ -187,16 +182,17 @@ namespace snmalloc CapPtr take(LocalEntropy& entropy) { auto c = curr; - auto next = curr->read_next(get_prev(), entropy); - -#ifdef CHECK_CLIENT - check_client( - !different_slab(curr, next), "Heap corruption - free list corrupted!"); - prev = address_cast(curr); -#endif - curr = next; + auto next = curr->read_next(); Aal::prefetch(next.unsafe_ptr()); + curr = next; +#ifdef CHECK_CLIENT + c->check_prev(prev); + prev = signed_prev(address_cast(c), address_cast(next), entropy); +#else + UNUSED(entropy); +#endif + return c; } }; @@ -224,46 +220,21 @@ namespace snmalloc * If RANDOM is set to false, then the code does not perform any * randomisation. */ - template + template class FreeListBuilder { static constexpr size_t LENGTH = RANDOM ? 2 : 1; // Pointer to the first element. - EncodeFreeObjectReference head[LENGTH]; + CapPtr head[LENGTH]; // Pointer to the reference to the last element. // In the empty case end[i] == &head[i] // This enables branch free enqueuing. - EncodeFreeObjectReference* end[LENGTH]; -#ifdef CHECK_CLIENT - // The bottom 32 bits of the previous pointer - uint32_t prev[LENGTH]; -#endif + CapPtr* end[LENGTH]; + public: S s; - uint32_t get_prev(uint32_t index) - { -#ifdef CHECK_CLIENT - return prev[index]; -#else - UNUSED(index); - return 0; -#endif - } - - uint32_t get_curr(uint32_t index) - { -#ifdef CHECK_CLIENT - return address_cast(end[index]) & 0xffff'ffff; -#else - UNUSED(index); - return 0; -#endif - } - - static constexpr uint32_t HEAD_KEY = 1; - public: constexpr FreeListBuilder() { @@ -283,63 +254,18 @@ namespace snmalloc return true; } - bool debug_different_slab(CapPtr n) - { - for (size_t i = 0; i < LENGTH; i++) - { - if (!different_slab(address_cast(end[i]), n)) - return false; - } - return true; - } - /** * Adds an element to the builder */ void add(CapPtr n, LocalEntropy& entropy) { - SNMALLOC_ASSERT(!debug_different_slab(n) || empty()); - uint32_t index; if constexpr (RANDOM) index = entropy.next_bit(); else index = 0; - end[index]->store(n, get_prev(index), entropy); -#ifdef CHECK_CLIENT - prev[index] = get_curr(index); -#endif - end[index] = &(n->next_object); - } - - /** - * Calculates the length of the queue. - * This is O(n) as it walks the queue. - * If this is needed in a non-debug setting then - * we should look at redesigning the queue. - */ - size_t debug_length(LocalEntropy& entropy) - { - size_t count = 0; - for (size_t i = 0; i < LENGTH; i++) - { - uint32_t local_prev = HEAD_KEY; - EncodeFreeObjectReference* iter = &head[i]; - CapPtr prev_obj = iter->read(local_prev, entropy); - UNUSED(prev_obj); - uint32_t local_curr = address_cast(&head[i]) & 0xffff'ffff; - while (end[i] != iter) - { - CapPtr next = iter->read(local_prev, entropy); - check_client(!different_slab(next, prev_obj), "Heap corruption"); - local_prev = local_curr; - local_curr = address_cast(next) & 0xffff'ffff; - count++; - iter = &next->next_object; - } - } - return count; + end[index] = FreeObject::store(end[index], n, entropy); } /** @@ -351,29 +277,21 @@ namespace snmalloc SNMALLOC_FAST_PATH void terminate_list(uint32_t index, LocalEntropy& entropy) { - auto term = CapPtr( - reinterpret_cast(end[index]) | 1); - end[index]->store(term, get_prev(index), entropy); + UNUSED(entropy); + *end[index] = nullptr; + } + + address_t get_fake_signed_prev(uint32_t index, LocalEntropy& entropy) + { + return signed_prev( + address_cast(&head[index]), address_cast(head[index]), entropy); } /** - * Adds a terminator at the end of a free list, - * but does not close the builder. Thus new elements - * can still be added. It returns a new iterator to the - * list. - * - * This also collapses the two queues into one, so that it can - * be iterated easily. - * - * This is used to iterate an list that is being constructed. - * - * It is used with preserve_queue enabled to check - * invariants in Debug builds. - * - * It is used with preserve_queue disabled by close. + * Close a free list, and set the iterator parameter + * to iterate it. */ - SNMALLOC_FAST_PATH void terminate( - FreeListIter& fl, LocalEntropy& entropy, bool preserve_queue = true) + SNMALLOC_FAST_PATH void close(FreeListIter& fl, LocalEntropy& entropy) { if constexpr (RANDOM) { @@ -383,62 +301,29 @@ namespace snmalloc // If second list is non-empty, perform append. if (end[1] != &head[1]) { + // The start token has been corrupted. + // TOCTTOU issue, but small window here. + head[1]->check_prev(get_fake_signed_prev(1, entropy)); + terminate_list(1, entropy); // Append 1 to 0 - auto mid = head[1].read(HEAD_KEY, entropy); - end[0]->store(mid, get_prev(0), entropy); - // Re-code first link in second list (if there is one). - // The first link in the second list will be encoded with initial key - // of the head, But that needs to be changed to the curr of the first - // list. - if (mid != nullptr) - { - auto mid_next = - mid->read_next(address_cast(&head[1]) & 0xffff'ffff, entropy); - mid->next_object.store(mid_next, get_curr(0), entropy); - } - - auto h = head[0].read(HEAD_KEY, entropy); - - // If we need to continue adding to the builder - // Set up the second list as empty, - // and extend the first list to cover all of the second. - if (preserve_queue && h != nullptr) - { -#ifdef CHECK_CLIENT - prev[0] = prev[1]; -#endif - end[0] = end[1]; -#ifdef CHECK_CLIENT - prev[1] = HEAD_KEY; -#endif - end[1] = &(head[1]); - } + FreeObject::store(end[0], head[1], entropy); SNMALLOC_ASSERT(end[1] != &head[0]); SNMALLOC_ASSERT(end[0] != &head[1]); - - fl = {h, address_cast(&head[0])}; - return; + } + else + { + terminate_list(0, entropy); } } else { - UNUSED(preserve_queue); + terminate_list(0, entropy); } - terminate_list(0, entropy); - fl = {head[0].read(HEAD_KEY, entropy), address_cast(&head[0])}; - } - - /** - * Close a free list, and set the iterator parameter - * to iterate it. - */ - SNMALLOC_FAST_PATH void close(FreeListIter& dst, LocalEntropy& entropy) - { - terminate(dst, entropy, false); + fl = {head[0], get_fake_signed_prev(0, entropy)}; init(); } @@ -450,9 +335,6 @@ namespace snmalloc for (size_t i = 0; i < LENGTH; i++) { end[i] = &head[i]; -#ifdef CHECK_CLIENT - prev[i] = HEAD_KEY; -#endif } } };