diff --git a/CMakeLists.txt b/CMakeLists.txt index b1358d1cd..63352651d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -490,7 +490,8 @@ if(NOT SNMALLOC_HEADER_ONLY_LIBRARY) random_larger_thresholds; random_initial; random_preserve; - random_extra_slab) + random_extra_slab; + scrub_free) foreach (MITIGATION ${MITIGATIONS}) diff --git a/src/snmalloc/ds_core/mitigations.h b/src/snmalloc/ds_core/mitigations.h index 88547dcc7..f6fb2fa0b 100644 --- a/src/snmalloc/ds_core/mitigations.h +++ b/src/snmalloc/ds_core/mitigations.h @@ -209,6 +209,12 @@ namespace snmalloc * model. */ static constexpr mitigation::type pal_enforce_access{1 << 13}; + /** + * If this mitigation is enabled, then deallocations are + * scrubbed before reallocation. This prevents data leaks + * by looking into uninitialised memory. + */ + static constexpr mitigation::type scrub_free{1 << 14}; constexpr mitigation::type full_checks = random_pagemap + random_larger_thresholds + freelist_forward_edge + freelist_backward_edge + diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 15167f2df..953c807fb 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -688,6 +688,49 @@ namespace snmalloc is_start_of_object(entry.get_sizeclass(), address_cast(p)), "Not deallocating start of an object"); + if (mitigations(scrub_free)) + { + // Scan for dirty pages and zero. + // This code is designed to handle the case + // where a lot of pages weren't touched by the application. + // This is not particularly realistic, but happens in a lot of + // benchmarks. + size_t block_size = Config::Pal::page_size; + size_t line = 8; + auto step = sizeof(uint64_t); + + auto size = sizeclass_full_to_size(entry.get_sizeclass()); + if ((size & ((line*step) - 1)) != 0) + Config::Pal::zero(p.unsafe_ptr(), size); + else + { + if ((size & (block_size - 1)) != 0) + { + // Not page aligned, so just run to the end. + block_size = size; + } + auto p_unsafe = reinterpret_cast(p.unsafe_ptr()); + auto p_end = p_unsafe + (size / step); + // Objects size is a multiple of 64, so we can write this differently + while (p_unsafe != p_end) + { + auto block_end = p_unsafe + (block_size / step); + bool dirty = false; + SNMALLOC_ASSERT((block_size / step) % line == 0); + while (p_unsafe != block_end && !dirty) + { + for (size_t i = 0; i < line; i++) + dirty |= (*p_unsafe++ != 0); + } + while (p_unsafe != block_end) + { + for (size_t i = 0; i < line; i++) + *p_unsafe++ = 0; + } + } + } + } + auto cp = p.as_static>(); auto& key = entropy.get_free_list_key(); diff --git a/src/snmalloc/mem/localcache.h b/src/snmalloc/mem/localcache.h index 68b232e4e..fb35d0a46 100644 --- a/src/snmalloc/mem/localcache.h +++ b/src/snmalloc/mem/localcache.h @@ -25,9 +25,13 @@ namespace snmalloc { auto r = finish_alloc_no_zero(p, sizeclass); - if constexpr (zero_mem == YesZero) + if constexpr (mitigations(scrub_free)) + { + Config::Pal::zero(r.unsafe_ptr(), sizeof(freelist::Object::T<>)); + } + else if constexpr (zero_mem == YesZero) Config::Pal::zero(r.unsafe_ptr(), sizeclass_to_size(sizeclass)); - + // TODO: Should this be zeroing the free Object state, in the non-zeroing // case? diff --git a/src/test/func/memory/memory.cc b/src/test/func/memory/memory.cc index 2a2ada2ee..b1c696c12 100644 --- a/src/test/func/memory/memory.cc +++ b/src/test/func/memory/memory.cc @@ -1,5 +1,9 @@ +// Silence some warnings from MSVC headers +#define _CRT_SECURE_NO_WARNINGS + #include #include +#include #include #include #include @@ -515,6 +519,47 @@ void test_consolidaton_bug() } } +/** + * Test that scrub free does not allow a secret to leak to the + * next allocation. + */ +void test_scrub_free() +{ + if (!snmalloc::mitigations(snmalloc::scrub_free)) + return; + std::cout << "Testing scrub free" << std::endl; + + auto& alloc = ThreadAlloc::get(); + + auto secret = (char*)alloc.alloc(256); + strcpy(secret, "mypassword"); + auto leak = (void**)alloc.alloc(16 * sizeof(void*)); + + for (size_t i = 0; i < 16; i++) + { + leak[i] = secret; + } + + alloc.dealloc(leak); + + for (size_t i = 0; i < 10000; i++) + { + auto search = (char**)alloc.alloc(16 * sizeof(void*)); + + for (size_t j = 0; j < 16; j++) + { + if (search[j] == secret) + { + printf( + "Secret \"%s\" after %zu index %zu @%p\n", search[j], i, j, search); + SNMALLOC_CHECK(false); + } + } + + alloc.dealloc(search); + } +} + int main(int argc, char** argv) { setup(); @@ -555,5 +600,6 @@ int main(int argc, char** argv) test_calloc_16M(); #endif test_consolidaton_bug(); + test_scrub_free(); return 0; }