From 54741074945306ee2018ad6e050e41e2238eca86 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Fri, 31 Mar 2023 08:04:06 +0100 Subject: [PATCH 01/13] Scrub deallocations before reallocation. --- CMakeLists.txt | 3 ++- src/snmalloc/ds_core/mitigations.h | 8 +++++++- src/snmalloc/mem/corealloc.h | 6 ++++++ 3 files changed, 15 insertions(+), 2 deletions(-) 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..ff5f75b7f 100644 --- a/src/snmalloc/ds_core/mitigations.h +++ b/src/snmalloc/ds_core/mitigations.h @@ -209,12 +209,18 @@ 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 + freelist_teardown_validate + random_initial + random_preserve + metadata_protection + random_extra_slab + reuse_LIFO + sanity_checks + - clear_meta + pal_enforce_access; + clear_meta + pal_enforce_access + scrub_free; constexpr mitigation::type no_checks{0}; diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 15167f2df..fbedb5ab5 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -688,6 +688,12 @@ namespace snmalloc is_start_of_object(entry.get_sizeclass(), address_cast(p)), "Not deallocating start of an object"); + if (mitigations(scrub_free)) + { + Config::Pal::zero( + p.unsafe_ptr(), sizeclass_full_to_size(entry.get_sizeclass())); + } + auto cp = p.as_static>(); auto& key = entropy.get_free_list_key(); From 9981bad730d9ed5ff8d0ad574cf6cba7c51cd5e1 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Fri, 31 Mar 2023 08:44:30 +0100 Subject: [PATCH 02/13] Add a test --- src/test/func/memory/memory.cc | 42 ++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/test/func/memory/memory.cc b/src/test/func/memory/memory.cc index 2a2ada2ee..78e91b3bc 100644 --- a/src/test/func/memory/memory.cc +++ b/src/test/func/memory/memory.cc @@ -515,6 +515,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; + + 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 +596,6 @@ int main(int argc, char** argv) test_calloc_16M(); #endif test_consolidaton_bug(); + test_scrub_free(); return 0; } From 55b54a5e2893dfc50bc575fd68518fde2b0c9c7d Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Fri, 31 Mar 2023 19:40:38 +0100 Subject: [PATCH 03/13] Make MSVC happy. --- src/test/func/memory/memory.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/test/func/memory/memory.cc b/src/test/func/memory/memory.cc index 78e91b3bc..45ce4b9a9 100644 --- a/src/test/func/memory/memory.cc +++ b/src/test/func/memory/memory.cc @@ -5,6 +5,8 @@ #include #include #include +#define __STDC_WANT_LIB_EXT1__ 1 +#include #if ((defined(__linux__) && !defined(__ANDROID__)) || defined(__sun)) && \ !defined(SNMALLOC_QEMU_WORKAROUND) /* @@ -527,8 +529,11 @@ void test_scrub_free() auto& alloc = ThreadAlloc::get(); auto secret = (char*)alloc.alloc(256); +#ifdef __STDC_LIB_EXT1__ + strcpy_s(secret, 256, "mypassword"); +#else strcpy(secret, "mypassword"); - +#endif auto leak = (void**)alloc.alloc(16 * sizeof(void*)); for (size_t i = 0; i < 16; i++) From 8f904d236130166e944335e83a6800519a96dcd3 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Fri, 31 Mar 2023 19:49:33 +0100 Subject: [PATCH 04/13] Make MSVC happy. --- src/test/func/memory/memory.cc | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/test/func/memory/memory.cc b/src/test/func/memory/memory.cc index 45ce4b9a9..ee5690918 100644 --- a/src/test/func/memory/memory.cc +++ b/src/test/func/memory/memory.cc @@ -1,3 +1,6 @@ +// Silence some warnings from MSVC headers +#define _CRT_SECURE_NO_WARNINGS + #include #include #include @@ -5,7 +8,6 @@ #include #include #include -#define __STDC_WANT_LIB_EXT1__ 1 #include #if ((defined(__linux__) && !defined(__ANDROID__)) || defined(__sun)) && \ !defined(SNMALLOC_QEMU_WORKAROUND) @@ -529,11 +531,7 @@ void test_scrub_free() auto& alloc = ThreadAlloc::get(); auto secret = (char*)alloc.alloc(256); -#ifdef __STDC_LIB_EXT1__ - strcpy_s(secret, 256, "mypassword"); -#else strcpy(secret, "mypassword"); -#endif auto leak = (void**)alloc.alloc(16 * sizeof(void*)); for (size_t i = 0; i < 16; i++) From bea29689f6cce7d27092e59c8ec9c0dcf15152b3 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Fri, 31 Mar 2023 20:10:11 +0100 Subject: [PATCH 05/13] Clangformat --- src/test/func/memory/memory.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/func/memory/memory.cc b/src/test/func/memory/memory.cc index ee5690918..86e516d69 100644 --- a/src/test/func/memory/memory.cc +++ b/src/test/func/memory/memory.cc @@ -3,12 +3,12 @@ #include #include +#include #include #include #include #include #include -#include #if ((defined(__linux__) && !defined(__ANDROID__)) || defined(__sun)) && \ !defined(SNMALLOC_QEMU_WORKAROUND) /* From ade6d2a6d39bd6978a6f58abcf6925c58f803f6c Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Wed, 9 Oct 2024 14:53:56 +0100 Subject: [PATCH 06/13] Check alternative clearing --- src/snmalloc/ds_core/mitigations.h | 2 +- src/snmalloc/mem/corealloc.h | 22 ++++++++++++++++++++-- src/test/func/memory/memory.cc | 1 + 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/snmalloc/ds_core/mitigations.h b/src/snmalloc/ds_core/mitigations.h index ff5f75b7f..f6fb2fa0b 100644 --- a/src/snmalloc/ds_core/mitigations.h +++ b/src/snmalloc/ds_core/mitigations.h @@ -220,7 +220,7 @@ namespace snmalloc random_larger_thresholds + freelist_forward_edge + freelist_backward_edge + freelist_teardown_validate + random_initial + random_preserve + metadata_protection + random_extra_slab + reuse_LIFO + sanity_checks + - clear_meta + pal_enforce_access + scrub_free; + clear_meta + pal_enforce_access; constexpr mitigation::type no_checks{0}; diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index fbedb5ab5..84302a74e 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -690,8 +690,26 @@ namespace snmalloc if (mitigations(scrub_free)) { - Config::Pal::zero( - p.unsafe_ptr(), sizeclass_full_to_size(entry.get_sizeclass())); + size_t block_size = 64; + auto size = sizeclass_full_to_size(entry.get_sizeclass()); + if ((size & (block_size -1)) != 0) + Config::Pal::zero(p.unsafe_ptr(), size); + else + { + auto p_unsafe = reinterpret_cast(p.unsafe_ptr()); + auto step = sizeof(uint64_t); + // Objects size is a multiple of 64, so we can write this differently + for (; size > 0; size -= block_size) + { + bool write = false; + auto p_unsafe2 = p_unsafe; + for (size_t j = block_size; j > 0; j -= step) + write |= *(p_unsafe++) != 0; + if (write) + for (size_t j = 0; j < block_size; j += 8) + *(p_unsafe2++) = 0; + } + } } auto cp = p.as_static>(); diff --git a/src/test/func/memory/memory.cc b/src/test/func/memory/memory.cc index 86e516d69..b1c696c12 100644 --- a/src/test/func/memory/memory.cc +++ b/src/test/func/memory/memory.cc @@ -527,6 +527,7 @@ void test_scrub_free() { if (!snmalloc::mitigations(snmalloc::scrub_free)) return; + std::cout << "Testing scrub free" << std::endl; auto& alloc = ThreadAlloc::get(); From 5f1af900a74f0ec6dbb73eecfc7d119af2b47548 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 17 Oct 2024 15:31:10 +0100 Subject: [PATCH 07/13] WesZero. --- src/snmalloc/mem/corealloc.h | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 84302a74e..c166aee90 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -690,24 +690,32 @@ namespace snmalloc if (mitigations(scrub_free)) { - size_t block_size = 64; + // 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; auto size = sizeclass_full_to_size(entry.get_sizeclass()); - if ((size & (block_size -1)) != 0) + if ((size & (block_size - 1)) != 0) Config::Pal::zero(p.unsafe_ptr(), size); else { auto p_unsafe = reinterpret_cast(p.unsafe_ptr()); auto step = sizeof(uint64_t); + auto p_end = p_unsafe + (size / step); // Objects size is a multiple of 64, so we can write this differently - for (; size > 0; size -= block_size) + while (p_unsafe != p_end) { - bool write = false; - auto p_unsafe2 = p_unsafe; - for (size_t j = block_size; j > 0; j -= step) - write |= *(p_unsafe++) != 0; - if (write) - for (size_t j = 0; j < block_size; j += 8) - *(p_unsafe2++) = 0; + auto block_end = p_unsafe + (block_size / step); + while (p_unsafe != block_end && *p_unsafe == 0) + { + p_unsafe++; + } + while (p_unsafe != block_end) + { + *(p_unsafe++) = 0; + } } } } From e88fbed696d2fce87c8f28fc5be9a7e90d81f6e5 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 17 Oct 2024 15:46:12 +0100 Subject: [PATCH 08/13] WesZero2 --- src/snmalloc/mem/corealloc.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index c166aee90..9100d5875 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -708,13 +708,18 @@ namespace snmalloc while (p_unsafe != p_end) { auto block_end = p_unsafe + (block_size / step); - while (p_unsafe != block_end && *p_unsafe == 0) + bool dirty = false; + size_t line = 8; + assert((block_size / step) % line == 0); + while (p_unsafe != block_end && !dirty) { - p_unsafe++; + for (size_t i = 0; i < line; i++) + dirty |= *p_unsafe++; } while (p_unsafe != block_end) { - *(p_unsafe++) = 0; + for (size_t i = 0; i < line; i++) + *p_unsafe++ = 0; } } } From 54c8ae5e373f41ddb137549702e75919b10ba072 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 17 Oct 2024 15:47:00 +0100 Subject: [PATCH 09/13] WesZero2 --- src/snmalloc/mem/corealloc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 9100d5875..3a841523f 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -710,7 +710,7 @@ namespace snmalloc auto block_end = p_unsafe + (block_size / step); bool dirty = false; size_t line = 8; - assert((block_size / step) % line == 0); + SNMALLOC_ASSERT((block_size / step) % line == 0); while (p_unsafe != block_end && !dirty) { for (size_t i = 0; i < line; i++) From b5d404a66811607f1a955dcc98bd7dd8bcb224f4 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 17 Oct 2024 15:54:45 +0100 Subject: [PATCH 10/13] WesZero3 --- src/snmalloc/mem/corealloc.h | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 3a841523f..02fce4f3c 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -696,25 +696,31 @@ namespace snmalloc // 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 & (block_size - 1)) != 0) + 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 step = sizeof(uint64_t); 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; - size_t line = 8; SNMALLOC_ASSERT((block_size / step) % line == 0); while (p_unsafe != block_end && !dirty) { for (size_t i = 0; i < line; i++) - dirty |= *p_unsafe++; + dirty |= (*p_unsafe++ != 0); } while (p_unsafe != block_end) { From 1698e52032ad24f589a6d3dca7203c3865297534 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 17 Oct 2024 15:55:31 +0100 Subject: [PATCH 11/13] WesZero3 --- src/snmalloc/mem/corealloc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 02fce4f3c..953c807fb 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -704,7 +704,7 @@ namespace snmalloc Config::Pal::zero(p.unsafe_ptr(), size); else { - if (size & (block_size - 1) != 0) + if ((size & (block_size - 1)) != 0) { // Not page aligned, so just run to the end. block_size = size; From 7253e8f09532aec050b735186c9d151971d2adcd Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 17 Oct 2024 18:27:49 +0100 Subject: [PATCH 12/13] Scrub FreeObject structure --- src/snmalloc/mem/localcache.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/snmalloc/mem/localcache.h b/src/snmalloc/mem/localcache.h index 68b232e4e..eded2667d 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)); + } + 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? From c7622a37d46e1de408f839cd7903c0fe805cce2a Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 17 Oct 2024 18:40:02 +0100 Subject: [PATCH 13/13] Scrub FreeObject structure --- src/snmalloc/mem/localcache.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/snmalloc/mem/localcache.h b/src/snmalloc/mem/localcache.h index eded2667d..fb35d0a46 100644 --- a/src/snmalloc/mem/localcache.h +++ b/src/snmalloc/mem/localcache.h @@ -27,7 +27,7 @@ namespace snmalloc if constexpr (mitigations(scrub_free)) { - Config::Pal::zero(r.unsafe_ptr(), sizeof(freelist::Object)); + 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));