From 74dbfc30bebc2e7e48e88edf3cf66b35c057b16f Mon Sep 17 00:00:00 2001 From: daan Date: Thu, 21 Nov 2019 15:21:23 -0800 Subject: [PATCH] improved security by encoding NULL values; double free mitigation on by default; more precise free list corruption detection --- CMakeLists.txt | 15 ++++-------- include/mimalloc-internal.h | 48 ++++++++++++++++++++++++++----------- include/mimalloc-types.h | 6 ++--- src/alloc.c | 4 ++-- src/page.c | 6 ++--- test/main-override-static.c | 6 ++--- 6 files changed, 49 insertions(+), 36 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index aa9c126f..467fad95 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7,8 +7,7 @@ set(CMAKE_CXX_STANDARD 17) option(MI_OVERRIDE "Override the standard malloc interface" ON) option(MI_INTERPOSE "Use interpose to override standard malloc on macOS" ON) option(MI_DEBUG_FULL "Use full internal heap invariant checking in DEBUG mode" OFF) -option(MI_SECURE "Use security mitigations (like guard pages, allocation randomization, and free-list corruption detection)" OFF) -option(MI_SECURE_FULL "Use full security mitigations, may be more expensive (includes double-free mitigation)" OFF) +option(MI_SECURE "Use full security mitigations (like guard pages, allocation randomization, double-free mitigation, and free-list corruption detection)" OFF) option(MI_USE_CXX "Use the C++ compiler to compile the library" OFF) option(MI_SEE_ASM "Generate assembly files" OFF) option(MI_LOCAL_DYNAMIC_TLS "Use slightly slower, dlopen-compatible TLS mechanism (Unix)" OFF) @@ -70,15 +69,9 @@ if(MI_OVERRIDE MATCHES "ON") endif() endif() -if(MI_SECURE_FULL MATCHES "ON") - message(STATUS "Set full secure build (may be more expensive) (MI_SECURE_FULL=ON)") +if(MI_SECURE MATCHES "ON") + message(STATUS "Set full secure build (MI_SECURE=ON)") list(APPEND mi_defines MI_SECURE=4) - set(MI_SECURE "ON") -else() - if(MI_SECURE MATCHES "ON") - message(STATUS "Set secure build (MI_SECURE=ON)") - list(APPEND mi_defines MI_SECURE=3) - endif() endif() if(MI_SEE_ASM MATCHES "ON") @@ -92,7 +85,7 @@ if(MI_CHECK_FULL MATCHES "ON") endif() if(MI_DEBUG_FULL MATCHES "ON") - message(STATUS "Set debug level to full invariant checking (MI_DEBUG_FULL=ON)") + message(STATUS "Set debug level to full internal invariant checking (MI_DEBUG_FULL=ON)") list(APPEND mi_defines MI_DEBUG=3) # full invariant checking endif() diff --git a/include/mimalloc-internal.h b/include/mimalloc-internal.h index 73849337..452f0b68 100644 --- a/include/mimalloc-internal.h +++ b/include/mimalloc-internal.h @@ -275,14 +275,20 @@ static inline mi_segment_t* _mi_page_segment(const mi_page_t* page) { return segment; } -// Get the page containing the pointer -static inline mi_page_t* _mi_segment_page_of(const mi_segment_t* segment, const void* p) { +// used internally +static inline uintptr_t _mi_segment_page_idx_of(const mi_segment_t* segment, const void* p) { // if (segment->page_size > MI_SEGMENT_SIZE) return &segment->pages[0]; // huge pages ptrdiff_t diff = (uint8_t*)p - (uint8_t*)segment; mi_assert_internal(diff >= 0 && diff < MI_SEGMENT_SIZE); uintptr_t idx = (uintptr_t)diff >> segment->page_shift; mi_assert_internal(idx < segment->capacity); mi_assert_internal(segment->page_kind <= MI_PAGE_MEDIUM || idx == 0); + return idx; +} + +// Get the page containing the pointer +static inline mi_page_t* _mi_segment_page_of(const mi_segment_t* segment, const void* p) { + uintptr_t idx = _mi_segment_page_idx_of(segment, p); return &((mi_segment_t*)segment)->pages[idx]; } @@ -373,53 +379,67 @@ static inline void mi_page_set_has_aligned(mi_page_t* page, bool has_aligned) { // ------------------------------------------------------------------- // Encoding/Decoding the free list next pointers +// Note: we pass a `null` value to be used as the `NULL` value for the +// end of a free list. This is to prevent the cookie itself to ever +// be present among user blocks (as `cookie^0==cookie`). // ------------------------------------------------------------------- static inline bool mi_is_in_same_segment(const void* p, const void* q) { return (_mi_ptr_segment(p) == _mi_ptr_segment(q)); } -static inline mi_block_t* mi_block_nextx( uintptr_t cookie, const mi_block_t* block ) { +static inline bool mi_is_in_same_page(const void* p, const void* q) { + mi_segment_t* segmentp = _mi_ptr_segment(p); + mi_segment_t* segmentq = _mi_ptr_segment(q); + if (segmentp != segmentq) return false; + uintptr_t idxp = _mi_segment_page_idx_of(segmentp, p); + uintptr_t idxq = _mi_segment_page_idx_of(segmentq, q); + return (idxp == idxq); +} + +static inline mi_block_t* mi_block_nextx( const void* null, const mi_block_t* block, uintptr_t cookie ) { #ifdef MI_ENCODE_FREELIST - return (mi_block_t*)(block->next ^ cookie); + mi_block_t* b = (mi_block_t*)(block->next ^ cookie); + if (mi_unlikely((void*)b==null)) { b = NULL; } + return b; #else - UNUSED(cookie); + UNUSED(cookie); UNUSED(null); return (mi_block_t*)block->next; #endif } -static inline void mi_block_set_nextx(uintptr_t cookie, mi_block_t* block, const mi_block_t* next) { +static inline void mi_block_set_nextx(const void* null, mi_block_t* block, const mi_block_t* next, uintptr_t cookie) { #ifdef MI_ENCODE_FREELIST + if (mi_unlikely(next==NULL)) { next = (mi_block_t*)null; } block->next = (mi_encoded_t)next ^ cookie; #else - UNUSED(cookie); + UNUSED(cookie); UNUSED(null); block->next = (mi_encoded_t)next; #endif } static inline mi_block_t* mi_block_next(const mi_page_t* page, const mi_block_t* block) { #ifdef MI_ENCODE_FREELIST - mi_block_t* next = mi_block_nextx(page->cookie,block); + mi_block_t* next = mi_block_nextx(page,block,page->cookie); // check for free list corruption: is `next` at least in our segment range? - // TODO: it is better to check if it is actually inside our page but that is more expensive - // to calculate. Perhaps with a relative free list this becomes feasible? - if (next!=NULL && !mi_is_in_same_segment(block, next)) { + // TODO: check if `next` is `page->block_size` aligned? + if (next!=NULL && !mi_is_in_same_page(block, next)) { _mi_fatal_error("corrupted free list entry of size %zub at %p: value 0x%zx\n", page->block_size, block, (uintptr_t)next); next = NULL; } return next; #else UNUSED(page); - return mi_block_nextx(0, block); + return mi_block_nextx(page,block,0); #endif } static inline void mi_block_set_next(const mi_page_t* page, mi_block_t* block, const mi_block_t* next) { #ifdef MI_ENCODE_FREELIST - mi_block_set_nextx(page->cookie,block,next); + mi_block_set_nextx(page,block,next, page->cookie); #else UNUSED(page); - mi_block_set_nextx(0, block, next); + mi_block_set_nextx(page,block, next,0); #endif } diff --git a/include/mimalloc-types.h b/include/mimalloc-types.h index 893dcd67..9c5d3c19 100644 --- a/include/mimalloc-types.h +++ b/include/mimalloc-types.h @@ -26,16 +26,16 @@ terms of the MIT license. A copy of the license can be found in the file // #define MI_SECURE 1 // guard page around metadata // #define MI_SECURE 2 // guard page around each mimalloc page // #define MI_SECURE 3 // encode free lists (detect corrupted free list (buffer overflow), and invalid pointer free) -// #define MI_SECURE 4 // experimental, may be more expensive: checks for double free. (cmake -DMI_SECURE_FULL=ON) +// #define MI_SECURE 4 // checks for double free. (may be more expensive) #if !defined(MI_SECURE) -#define MI_SECURE 0 +#define MI_SECURE 4 #endif // Define MI_DEBUG for debug mode // #define MI_DEBUG 1 // basic assertion checks and statistics, check double free, corrupted free list, and invalid pointer free. // #define MI_DEBUG 2 // + internal assertion checks -// #define MI_DEBUG 3 // + extensive internal invariant checking (cmake -DMI_CHECK_FULL=ON) +// #define MI_DEBUG 3 // + extensive internal invariant checking (cmake -DMI_DEBUG_FULL=ON) #if !defined(MI_DEBUG) #if !defined(NDEBUG) || defined(_DEBUG) #define MI_DEBUG 2 diff --git a/src/alloc.c b/src/alloc.c index c4863115..e68b48d2 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -157,7 +157,7 @@ static mi_decl_noinline bool mi_check_is_double_freex(const mi_page_t* page, con } static inline bool mi_check_is_double_free(const mi_page_t* page, const mi_block_t* block) { - mi_block_t* n = mi_block_nextx(page->cookie, block); // pretend it is freed, and get the decoded first field + mi_block_t* n = mi_block_nextx(page, block, page->cookie); // pretend it is freed, and get the decoded first field if (((uintptr_t)n & (MI_INTPTR_SIZE-1))==0 && // quick check: aligned pointer? (n==NULL || mi_is_in_same_segment(block, n))) // quick check: in same segment or NULL? { @@ -242,7 +242,7 @@ static mi_decl_noinline void _mi_free_block_mt(mi_page_t* page, mi_block_t* bloc mi_block_t* dfree; do { dfree = (mi_block_t*)heap->thread_delayed_free; - mi_block_set_nextx(heap->cookie,block,dfree); + mi_block_set_nextx(heap,block,dfree, heap->cookie); } while (!mi_atomic_cas_ptr_weak(mi_atomic_cast(void*,&heap->thread_delayed_free), block, dfree)); } diff --git a/src/page.c b/src/page.c index a8115d27..437cd0a5 100644 --- a/src/page.c +++ b/src/page.c @@ -283,7 +283,7 @@ void _mi_heap_delayed_free(mi_heap_t* heap) { // and free them all while(block != NULL) { - mi_block_t* next = mi_block_nextx(heap->cookie,block); + mi_block_t* next = mi_block_nextx(heap,block, heap->cookie); // use internal free instead of regular one to keep stats etc correct if (!_mi_free_delayed_block(block)) { // we might already start delayed freeing while another thread has not yet @@ -291,7 +291,7 @@ void _mi_heap_delayed_free(mi_heap_t* heap) { mi_block_t* dfree; do { dfree = (mi_block_t*)heap->thread_delayed_free; - mi_block_set_nextx(heap->cookie, block, dfree); + mi_block_set_nextx(heap, block, dfree, heap->cookie); } while (!mi_atomic_cas_ptr_weak(mi_atomic_cast(void*,&heap->thread_delayed_free), block, dfree)); } @@ -356,7 +356,7 @@ void _mi_page_abandon(mi_page_t* page, mi_page_queue_t* pq) { #if MI_DEBUG>1 // check there are no references left.. - for (mi_block_t* block = (mi_block_t*)pheap->thread_delayed_free; block != NULL; block = mi_block_nextx(pheap->cookie, block)) { + for (mi_block_t* block = (mi_block_t*)pheap->thread_delayed_free; block != NULL; block = mi_block_nextx(pheap, block, pheap->cookie)) { mi_assert_internal(_mi_ptr_page(block) != page); } #endif diff --git a/test/main-override-static.c b/test/main-override-static.c index 19712411..b04bfeef 100644 --- a/test/main-override-static.c +++ b/test/main-override-static.c @@ -15,9 +15,9 @@ int main() { mi_version(); // detect double frees and heap corruption - //double_free1(); - //double_free2(); - //corrupt_free(); + // double_free1(); + // double_free2(); + // corrupt_free(); void* p1 = malloc(78); void* p2 = malloc(24);