diff --git a/CMakeLists.txt b/CMakeLists.txt index 443476f0..81cc339a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -68,7 +68,7 @@ endif() if(MI_SECURE MATCHES "ON") message(STATUS "Set secure build (MI_SECURE=ON)") - list(APPEND mi_defines MI_SECURE=2) + list(APPEND mi_defines MI_SECURE=3) endif() if(MI_SEE_ASM MATCHES "ON") diff --git a/ide/vs2019/mimalloc.vcxproj b/ide/vs2019/mimalloc.vcxproj index 5658b536..28e96d71 100644 --- a/ide/vs2019/mimalloc.vcxproj +++ b/ide/vs2019/mimalloc.vcxproj @@ -111,12 +111,12 @@ - Level3 + Level2 Disabled true true ../../include - MI_DEBUG=3;%(PreprocessorDefinitions); + MI_DEBUG=1;%(PreprocessorDefinitions); CompileAsCpp false stdcpp17 diff --git a/include/mimalloc-internal.h b/include/mimalloc-internal.h index 4c47af94..7bffb6ac 100644 --- a/include/mimalloc-internal.h +++ b/include/mimalloc-internal.h @@ -20,6 +20,18 @@ terms of the MIT license. A copy of the license can be found in the file #define mi_trace_message(...) #endif +#if defined(_MSC_VER) +#define mi_decl_noinline __declspec(noinline) +#define mi_attr_noreturn +#elif defined(__GNUC__) || defined(__clang__) +#define mi_decl_noinline __attribute__((noinline)) +#define mi_attr_noreturn __attribute__((noreturn)) +#else +#define mi_decl_noinline +#define mi_attr_noreturn +#endif + + // "options.c" void _mi_fputs(mi_output_fun* out, const char* prefix, const char* message); void _mi_fprintf(mi_output_fun* out, const char* fmt, ...); @@ -28,6 +40,7 @@ void _mi_warning_message(const char* fmt, ...); void _mi_verbose_message(const char* fmt, ...); void _mi_trace_message(const char* fmt, ...); void _mi_options_init(void); +void _mi_fatal_error(const char* fmt, ...) mi_attr_noreturn; // "init.c" extern mi_stats_t _mi_stats_main; @@ -124,13 +137,6 @@ bool _mi_page_is_valid(mi_page_t* page); #define __has_builtin(x) 0 #endif -#if defined(_MSC_VER) -#define mi_decl_noinline __declspec(noinline) -#elif defined(__GNUC__) || defined(__clang__) -#define mi_decl_noinline __attribute__((noinline)) -#else -#define mi_decl_noinline -#endif /* ----------------------------------------------------------- @@ -365,8 +371,12 @@ static inline void mi_page_set_has_aligned(mi_page_t* page, bool has_aligned) { // Encoding/Decoding the free list next pointers // ------------------------------------------------------------------- -static inline mi_block_t* mi_block_nextx( uintptr_t cookie, mi_block_t* block ) { - #if MI_SECURE +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 ) { + #if MI_SECURE return (mi_block_t*)(block->next ^ cookie); #else UNUSED(cookie); @@ -374,7 +384,7 @@ static inline mi_block_t* mi_block_nextx( uintptr_t cookie, mi_block_t* block ) #endif } -static inline void mi_block_set_nextx(uintptr_t cookie, mi_block_t* block, mi_block_t* next) { +static inline void mi_block_set_nextx(uintptr_t cookie, mi_block_t* block, const mi_block_t* next) { #if MI_SECURE block->next = (mi_encoded_t)next ^ cookie; #else @@ -383,16 +393,25 @@ static inline void mi_block_set_nextx(uintptr_t cookie, mi_block_t* block, mi_bl #endif } -static inline mi_block_t* mi_block_next(mi_page_t* page, mi_block_t* block) { +static inline mi_block_t* mi_block_next(const mi_page_t* page, const mi_block_t* block) { #if MI_SECURE - return mi_block_nextx(page->cookie,block); + mi_block_t* next = mi_block_nextx(page->cookie,block); + #if MI_SECURE >= 4 + // check if next is 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)) { + _mi_fatal_error("corrupted free list entry at %p: %zx\n", block, (uintptr_t)next); + } + #endif + return next; #else UNUSED(page); return mi_block_nextx(0, block); #endif } -static inline void mi_block_set_next(mi_page_t* page, mi_block_t* block, mi_block_t* next) { +static inline void mi_block_set_next(const mi_page_t* page, mi_block_t* block, const mi_block_t* next) { #if MI_SECURE mi_block_set_nextx(page->cookie,block,next); #else diff --git a/include/mimalloc-types.h b/include/mimalloc-types.h index eea76a25..00a83839 100644 --- a/include/mimalloc-types.h +++ b/include/mimalloc-types.h @@ -22,8 +22,11 @@ terms of the MIT license. A copy of the license can be found in the file // Define MI_STAT as 1 to maintain statistics; set it to 2 to have detailed statistics (but costs some performance). // #define MI_STAT 1 -// Define MI_SECURE as 1 to encode free lists -// #define MI_SECURE 1 +// Define MI_SECURE to enable security mitigations +// #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 +// #define MI_SECURE 4 // all security enabled (checks for double free, corrupted free list and invalid pointer free) #if !defined(MI_SECURE) #define MI_SECURE 0 diff --git a/src/alloc.c b/src/alloc.c index 0c399671..f5208a0a 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -124,10 +124,50 @@ mi_decl_allocator void* mi_zalloc(size_t size) mi_attr_noexcept { } +// ------------------------------------------------------ +// Check for double free in secure mode +// ------------------------------------------------------ + +#if MI_SECURE>=4 +static bool mi_list_contains(const mi_page_t* page, const mi_block_t* list, const mi_block_t* elem) { + while (list != NULL) { + if (elem==list) return true; + list = mi_block_next(page, list); + } + return false; +} + +static mi_decl_noinline void mi_free_check_blockx(const mi_page_t* page, const mi_block_t* block, const mi_block_t* n) { + size_t psize; + uint8_t* pstart = _mi_page_start(_mi_page_segment(page), page, &psize); + if ((uint8_t*)n >= pstart && (uint8_t*)n < (pstart + psize)) { + // Suspicious: the decoded value is in the same page. + // Walk the free lists to see if it is already freed + if (mi_list_contains(page, page->free, n) || + mi_list_contains(page, page->local_free, n) || + mi_list_contains(page, (const mi_block_t*)mi_atomic_read_ptr_relaxed(mi_atomic_cast(void*,&page->thread_free)), n)) + { + _mi_fatal_error("double free detected of block %p with size %zu\n", block, page->block_size); + } + } +} + +static inline void mi_free_check_block(const mi_page_t* page, const mi_block_t* block) { + mi_block_t* n = (mi_block_t*)(block->next ^ page->cookie); + if (n!=NULL && mi_is_in_same_segment(block, n)) { // quick check + // Suspicous: decoded value in block is in the same segment, maybe a double free? + mi_free_check_blockx(page, block, n); + } + return; +} +#endif + + // ------------------------------------------------------ // Free // ------------------------------------------------------ + // multi-threaded free static mi_decl_noinline void _mi_free_block_mt(mi_page_t* page, mi_block_t* block) { @@ -251,14 +291,16 @@ void mi_free(void* p) mi_attr_noexcept #if (MI_DEBUG>0) if (mi_unlikely(!mi_is_in_heap_region(p))) { - _mi_warning_message("possibly trying to mi_free a pointer that does not point to a valid heap region: 0x%p\n" + _mi_warning_message("possibly trying to free a pointer that does not point to a valid heap region: 0x%p\n" "(this may still be a valid very large allocation (over 64MiB))\n", p); if (mi_likely(_mi_ptr_cookie(segment) == segment->cookie)) { _mi_warning_message("(yes, the previous pointer 0x%p was valid after all)\n", p); } } +#endif +#if (MI_DEBUG>0 || MI_SECURE>=4) if (mi_unlikely(_mi_ptr_cookie(segment) != segment->cookie)) { - _mi_error_message("trying to mi_free a pointer that does not point to a valid heap space: %p\n", p); + _mi_error_message("trying to free a pointer that does not point to a valid heap space: %p\n", p); return; } #endif @@ -278,6 +320,9 @@ void mi_free(void* p) mi_attr_noexcept if (mi_likely(tid == segment->thread_id && page->flags.full_aligned == 0)) { // the thread id matches and it is not a full page, nor has aligned blocks // local, and not full or aligned mi_block_t* block = (mi_block_t*)p; + #if MI_SECURE>=4 + mi_free_check_block(page,block); + #endif mi_block_set_next(page, block, page->local_free); page->local_free = block; page->used--; diff --git a/src/options.c b/src/options.c index 4e2bdeaa..e74d9eb5 100644 --- a/src/options.c +++ b/src/options.c @@ -285,6 +285,14 @@ void _mi_assert_fail(const char* assertion, const char* fname, unsigned line, co } #endif +mi_attr_noreturn void _mi_fatal_error(const char* fmt, ...) { + va_list args; + va_start(args, fmt); + mi_vfprintf(NULL, "mimalloc: fatal: ", fmt, args); + va_end(args); + exit(99); +} + // -------------------------------------------------------- // Initialize options by checking the environment // -------------------------------------------------------- diff --git a/test/main-override-static.c b/test/main-override-static.c index 6ddf4f37..d8369389 100644 --- a/test/main-override-static.c +++ b/test/main-override-static.c @@ -2,12 +2,16 @@ #include #include #include +#include #include #include // redefines malloc etc. +static void double_free(); + int main() { mi_version(); + double_free(); void* p1 = malloc(78); void* p2 = malloc(24); free(p1); @@ -29,3 +33,19 @@ int main() { mi_stats_print(NULL); return 0; } + +static void double_free() { + void* p[256]; + uintptr_t buf[256]; + + p[0] = mi_malloc(622616); + p[1] = mi_malloc(655362); + p[2] = mi_malloc(786432); + mi_free(p[2]); + // [VULN] Double free + mi_free(p[2]); + p[3] = mi_malloc(786456); + // [BUG] Found overlap + // p[3]=0x429b2ea2000 (size=917504), p[1]=0x429b2e42000 (size=786432) + fprintf(stderr, "p3: %p-%p, p1: %p-%p, p2: %p\n", p[3], (uint8_t*)(p[3]) + 786456, p[1], (uint8_t*)(p[1]) + 655362, p[2]); +} diff --git a/test/main-override.cpp b/test/main-override.cpp index 4bc91ae8..ea940061 100644 --- a/test/main-override.cpp +++ b/test/main-override.cpp @@ -2,10 +2,13 @@ #include #include #include +#include #include #include +static void double_free(); + static void* p = malloc(8); void free_p() { @@ -24,6 +27,7 @@ public: int main() { //mi_stats_reset(); // ignore earlier allocations + double_free(); atexit(free_p); void* p1 = malloc(78); void* p2 = mi_malloc_aligned(16,24); @@ -66,3 +70,5 @@ public: }; static Static s = Static(); + +