From fc43fed8ea76c376046ca94d407e3e669d6e0592 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Tue, 9 Aug 2022 13:53:10 +0200 Subject: [PATCH 1/4] heap: Provide definition of the tlsf_check_hook() declared in the tlsf submodule Add the definition of tlsf_check_hook() in multi_heap if MULTI_HEAP_POISONING is set. This definition calls the multi_heap_internal_check_block_poisoning() to check the memory of a free block for corruption. If the light poisoinng is set this function returns true. If the comprehensive poisoning is set, this function will check that all byte of memory in the memory chunk passed as parameter are set to the right FILL pattern. --- components/heap/include/multi_heap.h | 2 ++ components/heap/multi_heap.c | 35 +++++++++++++++++++++++++++- components/heap/tlsf | 2 +- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/components/heap/include/multi_heap.h b/components/heap/include/multi_heap.h index 6b0990c72d..e2aa6672d7 100644 --- a/components/heap/include/multi_heap.h +++ b/components/heap/include/multi_heap.h @@ -125,6 +125,8 @@ void multi_heap_dump(multi_heap_handle_t heap); * can be optionally printed to stderr. Print behaviour can be overridden at compile time by defining * MULTI_CHECK_FAIL_PRINTF in multi_heap_platform.h. * + * @note This function is not thread-safe as it sets a global variable with the value of print_errors. + * * @param heap Handle to a registered heap. * @param print_errors If true, errors will be printed to stderr. * @return true if heap is valid, false otherwise. diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c index 1a0871f457..9431a23771 100644 --- a/components/heap/multi_heap.c +++ b/components/heap/multi_heap.c @@ -309,13 +309,46 @@ void *multi_heap_aligned_alloc_impl(multi_heap_handle_t heap, size_t size, size_ return multi_heap_aligned_alloc_impl_offs(heap, size, alignment, 0); } +#ifdef MULTI_HEAP_POISONING +/*! + * @brief Global definition of print_errors set in multi_heap_check() when + * MULTI_HEAP_POISONING is active. Allows the transfer of the value to + * multi_heap_poisoning.c without having to propagate it to the tlsf submodule + * and back. + */ +static bool g_print_errors = false; + +/*! + * @brief Definition of the weak function declared in TLSF repository. + * The call of this function execute a check for block poisoning on the memory + * chunk passed as parameter. + * + * @param start: pointer to the start of the memory region to check for corruption + * @param size: size of the memory region to check for corruption + * @param is_free: indicate if the pattern to use the fill the region should be + * an after free or after allocation pattern. + * + * @return bool: true if the the memory is not corrupted, false if the memory if corrupted. + */ +bool tlsf_check_hook(void *start, size_t size, bool is_free) +{ + return multi_heap_internal_check_block_poisoning(start, size, is_free, g_print_errors); +} +#endif // MULTI_HEAP_POISONING + bool multi_heap_check(multi_heap_handle_t heap, bool print_errors) { - (void)print_errors; bool valid = true; assert(heap != NULL); multi_heap_internal_lock(heap); + +#ifdef MULTI_HEAP_POISONING + g_print_errors = print_errors; +#else + (void) print_errors; +#endif + if(tlsf_check(heap->heap_data)) { valid = false; } diff --git a/components/heap/tlsf b/components/heap/tlsf index ff11688f24..ab17d6798d 160000 --- a/components/heap/tlsf +++ b/components/heap/tlsf @@ -1 +1 @@ -Subproject commit ff11688f242b28b3918c2cdaa20738d12d73b5f4 +Subproject commit ab17d6798d1561758827b6553d56d57f19aa4d66 From b8f682a11b787675c23cf0ac92b697b1de50ad2b Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Fri, 12 Aug 2022 11:28:16 +0200 Subject: [PATCH 2/4] esp-rom: create a patch of tlsf_check() for target(s) supporting ROM implementation of TLSF The tlsf implementation in the ROM does not provide a mechanism to register a callback to be called in by tlsf_check(). This commit is creating a patch of the tlsf implementation to provide a definition of the function allowing to register the callback called in tlsf_check() and add the call of this callback in tlsf_check(). This patch is only compiled for target(s) with ESP_ROM_HAS_HEAP_TLSF set and ESP_ROM_TLSF_CHECK_PATCH set. For all the other configurations the environment remains unchanged by those modifications. --- components/esp_rom/CMakeLists.txt | 5 + .../esp_rom/esp32c2/Kconfig.soc_caps.in | 4 + components/esp_rom/esp32c2/esp_rom_caps.h | 1 + components/esp_rom/include/esp32c2/rom/tlsf.h | 9 + components/esp_rom/patches/esp_rom_tlsf.c | 240 ++++++++++++++++++ components/heap/multi_heap_poisoning.c | 5 +- 6 files changed, 262 insertions(+), 2 deletions(-) create mode 100644 components/esp_rom/patches/esp_rom_tlsf.c diff --git a/components/esp_rom/CMakeLists.txt b/components/esp_rom/CMakeLists.txt index 5938c3a823..5fe5dc2f50 100644 --- a/components/esp_rom/CMakeLists.txt +++ b/components/esp_rom/CMakeLists.txt @@ -20,6 +20,11 @@ else() "patches/esp_rom_spiflash.c" "patches/esp_rom_regi2c.c" "patches/esp_rom_efuse.c") + + if(CONFIG_ESP_ROM_HAS_HEAP_TLSF AND CONFIG_ESP_ROM_TLSF_CHECK_PATCH) + list(APPEND sources "patches/esp_rom_tlsf.c") + endif() + list(APPEND private_required_comp soc hal) endif() diff --git a/components/esp_rom/esp32c2/Kconfig.soc_caps.in b/components/esp_rom/esp32c2/Kconfig.soc_caps.in index b6e13da822..d676d6ce03 100644 --- a/components/esp_rom/esp32c2/Kconfig.soc_caps.in +++ b/components/esp_rom/esp32c2/Kconfig.soc_caps.in @@ -38,3 +38,7 @@ config ESP_ROM_HAS_HAL_SYSTIMER config ESP_ROM_HAS_HEAP_TLSF bool default y + +config ESP_ROM_TLSF_CHECK_PATCH + bool + default y diff --git a/components/esp_rom/esp32c2/esp_rom_caps.h b/components/esp_rom/esp32c2/esp_rom_caps.h index eee69daaaa..60706ac706 100644 --- a/components/esp_rom/esp32c2/esp_rom_caps.h +++ b/components/esp_rom/esp32c2/esp_rom_caps.h @@ -15,3 +15,4 @@ #define ESP_ROM_HAS_HAL_WDT (1) // ROM has the implementation of Watchdog HAL driver #define ESP_ROM_HAS_HAL_SYSTIMER (1) // ROM has the implementation of Systimer HAL driver #define ESP_ROM_HAS_HEAP_TLSF (1) // ROM has the implementation of the tlsf and multi-heap library +#define ESP_ROM_TLSF_CHECK_PATCH (1) // ROM does not contain the patch of tlsf_check() diff --git a/components/esp_rom/include/esp32c2/rom/tlsf.h b/components/esp_rom/include/esp32c2/rom/tlsf.h index 516822190e..347ef5eddd 100644 --- a/components/esp_rom/include/esp32c2/rom/tlsf.h +++ b/components/esp_rom/include/esp32c2/rom/tlsf.h @@ -18,6 +18,15 @@ extern "C" { */ void tlsf_poison_fill_pfunc_set(void *pfunc); +/*! + * @brief Set the function to call for checking memory region when + * poisoning is configured. + * + * @param pfunc The callback function to trigger for checking + * the content of a memory region. + */ +void tlsf_poison_check_pfunc_set(void *pfunc); + #ifdef __cplusplus } #endif diff --git a/components/esp_rom/patches/esp_rom_tlsf.c b/components/esp_rom/patches/esp_rom_tlsf.c new file mode 100644 index 0000000000..aaf9d2dc17 --- /dev/null +++ b/components/esp_rom/patches/esp_rom_tlsf.c @@ -0,0 +1,240 @@ +/* + * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ + +/* + * This file is a patch for the tlsf implementation stored in ROM + * - tlsf_check() now implements a call to a hook giving the user the possibility + * to implement specific checks on the memory of every free blocks. + * - The function tlsf_poison_check_pfunc_set() was added to allow the user to + * register the hook function called in tlsf_check(). + */ + +#include +#include + +#include "esp_rom_caps.h" +#include "rom/tlsf.h" + +/* ---------------------------------------------------------------- + * Bring certain inline functions, macro and structures from the + * tlsf ROM implementation to be able to compile the patch. + * ---------------------------------------------------------------- */ + +#define tlsf_cast(t, exp) ((t) (exp)) + +enum tlsf_config { + /* log2 of number of linear subdivisions of block sizes. Larger + ** values require more memory in the control structure. Values of + ** 4 or 5 are typical. + */ + SL_INDEX_COUNT_LOG2 = 5, + + /* All allocation sizes and addresses are aligned to 4 bytes. */ + ALIGN_SIZE_LOG2 = 2, + ALIGN_SIZE = (1 << ALIGN_SIZE_LOG2), + +/* + ** We support allocations of sizes up to (1 << FL_INDEX_MAX) bits. + ** However, because we linearly subdivide the second-level lists, and + ** our minimum size granularity is 4 bytes, it doesn't make sense to + ** create first-level lists for sizes smaller than SL_INDEX_COUNT * 4, + ** or (1 << (SL_INDEX_COUNT_LOG2 + 2)) bytes, as there we will be + ** trying to split size ranges into more slots than we have available. + ** Instead, we calculate the minimum threshold size, and place all + ** blocks below that size into the 0th first-level list. + */ + + /* Fix the value of FL_INDEX_MAX to match the value that is defined + * in the ROM implementation. */ + FL_INDEX_MAX = 18, //Each pool can have up 256KB + + SL_INDEX_COUNT = (1 << SL_INDEX_COUNT_LOG2), + FL_INDEX_SHIFT = (SL_INDEX_COUNT_LOG2 + ALIGN_SIZE_LOG2), + FL_INDEX_COUNT = (FL_INDEX_MAX - FL_INDEX_SHIFT + 1), + + SMALL_BLOCK_SIZE = (1 << FL_INDEX_SHIFT), +}; + +#define block_header_free_bit (1 << 0) +#define block_header_prev_free_bit (1 << 1) +#define block_header_overhead (sizeof(size_t)) +#define block_start_offset (offsetof(block_header_t, size) + sizeof(size_t)) +#define block_size_min (sizeof(block_header_t) - sizeof(block_header_t*)) + +typedef ptrdiff_t tlsfptr_t; + +typedef struct block_header_t +{ + /* Points to the previous physical block. */ + struct block_header_t* prev_phys_block; + + /* The size of this block, excluding the block header. */ + size_t size; + + /* Next and previous free blocks. */ + struct block_header_t* next_free; + struct block_header_t* prev_free; +} block_header_t; + +/* The TLSF control structure. */ +typedef struct control_t +{ + /* Empty lists point at this block to indicate they are free. */ + block_header_t block_null; + + /* Bitmaps for free lists. */ + unsigned int fl_bitmap; + unsigned int sl_bitmap[FL_INDEX_COUNT]; + + /* Head of free lists. */ + block_header_t* blocks[FL_INDEX_COUNT][SL_INDEX_COUNT]; +} control_t; + +static inline __attribute__((__always_inline__)) int tlsf_fls(unsigned int word) +{ + const int bit = word ? 32 - __builtin_clz(word) : 0; + return bit - 1; +} + +static inline __attribute__((__always_inline__)) size_t block_size(const block_header_t* block) +{ + return block->size & ~(block_header_free_bit | block_header_prev_free_bit); +} + +static inline __attribute__((__always_inline__)) int block_is_free(const block_header_t* block) +{ + return tlsf_cast(int, block->size & block_header_free_bit); +} + +static inline __attribute__((__always_inline__)) int block_is_prev_free(const block_header_t* block) +{ + return tlsf_cast(int, block->size & block_header_prev_free_bit); +} + +static inline __attribute__((__always_inline__)) block_header_t* offset_to_block(const void* ptr, size_t size) +{ + return tlsf_cast(block_header_t*, tlsf_cast(tlsfptr_t, ptr) + size); +} + +static inline __attribute__((__always_inline__)) void* block_to_ptr(const block_header_t* block) +{ + return tlsf_cast(void*, + tlsf_cast(unsigned char*, block) + block_start_offset); +} + +static inline __attribute__((__always_inline__)) block_header_t* block_next(const block_header_t* block) +{ + block_header_t* next = offset_to_block(block_to_ptr(block), + block_size(block) - block_header_overhead); + return next; +} + +static inline __attribute__((__always_inline__)) void mapping_insert(size_t size, int* fli, int* sli) +{ + int fl, sl; + if (size < SMALL_BLOCK_SIZE) + { + /* Store small blocks in first list. */ + fl = 0; + sl = tlsf_cast(int, size) >> 2; + } + else + { + fl = tlsf_fls(size); + sl = tlsf_cast(int, size >> (fl - SL_INDEX_COUNT_LOG2)) ^ (1 << SL_INDEX_COUNT_LOG2); + fl -= (FL_INDEX_SHIFT - 1); + } + *fli = fl; + *sli = sl; +} + +/* ---------------------------------------------------------------- + * End of the environment necessary to compile and link the patch + * defined below + * ---------------------------------------------------------------- */ + +typedef bool (*poison_check_pfunc_t)(void *start, size_t size, bool is_free, bool print_errors); +static poison_check_pfunc_t s_poison_check_region = NULL; + +void tlsf_poison_check_pfunc_set(void *pfunc) +{ + s_poison_check_region = (poison_check_pfunc_t)pfunc; +} + +#define tlsf_insist_no_assert(x) { if (!(x)) { status--; } } + +int tlsf_check(void* tlsf) +{ + int i, j; + + control_t* control = tlsf_cast(control_t*, tlsf); + int status = 0; + + /* Check that the free lists and bitmaps are accurate. */ + for (i = 0; i < FL_INDEX_COUNT; ++i) + { + for (j = 0; j < SL_INDEX_COUNT; ++j) + { + const int fl_map = control->fl_bitmap & (1 << i); + const int sl_list = control->sl_bitmap[i]; + const int sl_map = sl_list & (1 << j); + const block_header_t* block = control->blocks[i][j]; + + /* Check that first- and second-level lists agree. */ + if (!fl_map) + { + tlsf_insist_no_assert(!sl_map && "second-level map must be null"); + } + + if (!sl_map) + { + tlsf_insist_no_assert(block == &control->block_null && "block list must be null"); + continue; + } + + /* Check that there is at least one free block. */ + tlsf_insist_no_assert(sl_list && "no free blocks in second-level map"); + tlsf_insist_no_assert(block != &control->block_null && "block should not be null"); + + while (block != &control->block_null) + { + int fli, sli; + const bool is_block_free = block_is_free(block); + tlsf_insist_no_assert(is_block_free && "block should be free"); + tlsf_insist_no_assert(!block_is_prev_free(block) && "blocks should have coalesced"); + tlsf_insist_no_assert(!block_is_free(block_next(block)) && "blocks should have coalesced"); + tlsf_insist_no_assert(block_is_prev_free(block_next(block)) && "block should be free"); + tlsf_insist_no_assert(block_size(block) >= block_size_min && "block not minimum size"); + + mapping_insert(block_size(block), &fli, &sli); + tlsf_insist_no_assert(fli == i && sli == j && "block size indexed in wrong list"); + block = block->next_free; + + /* block_size(block) returns the size of the usable memory when the block is allocated. + * As the block under test is free, we need to subtract to the block size the next_free + * and prev_free fields of the block header as they are not a part of the usable memory + * when the block is free. In addition, we also need to subtract the size of prev_phys_block + * as this field is in fact part of the current free block and not part of the next (allocated) + * block. Check the comments in block_split function for more details. + */ + const size_t actual_free_block_size = block_size(block) + - offsetof(block_header_t, next_free) + - block_header_overhead; + + if (s_poison_check_region != NULL) { + tlsf_insist_no_assert(s_poison_check_region((char *)block + sizeof(block_header_t), + actual_free_block_size, is_block_free, true /* print errors */)); + } + + + } + } + } + + return status; +} + +#undef tlsf_insist_no_assert diff --git a/components/heap/multi_heap_poisoning.c b/components/heap/multi_heap_poisoning.c index f59ef6766d..399d5e30bf 100644 --- a/components/heap/multi_heap_poisoning.c +++ b/components/heap/multi_heap_poisoning.c @@ -25,8 +25,8 @@ #include "tlsf.h" #else /* Header containing the declaration of tlsf_poison_fill_pfunc_set() - * used to register multi_heap_internal_poison_fill_region() as a - * callback to fill memory region with given patterns in the heap + * and tlsf_poison_check_pfunc_set() used to register callbacks to + * fill and check memory region with given patterns in the heap * components. */ #include "rom/tlsf.h" @@ -361,6 +361,7 @@ multi_heap_handle_t multi_heap_register(void *start, size_t size) #endif #ifdef CONFIG_HEAP_TLSF_USE_ROM_IMPL tlsf_poison_fill_pfunc_set(multi_heap_internal_poison_fill_region); + tlsf_poison_check_pfunc_set(multi_heap_internal_check_block_poisoning); #endif return multi_heap_register_impl(start, size); } From 860232bdafabbf136130405cdcaea67210b513ee Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Wed, 10 Aug 2022 16:19:04 +0200 Subject: [PATCH 3/4] heap: Add test to check that the corruption of free memory is detected This commit extends the heap test set by adding a test to check corruption detection in free memory block. For each byte of the free block memory, the test changes the value of the byte, call multi_heap_check(), make sure that the function returns 'corruption detected' only when comprehensive poisoning is set, restore the good value of the byte, calls multi_heap_check() again and make sure that it returns 'OK'. --- components/heap/test/test_corruption_check.c | 64 +++++++++++++++++++ components/heap/test_multi_heap_host/Makefile | 2 +- .../test_multi_heap_host/test_multi_heap.cpp | 64 +++++++++++++++++++ 3 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 components/heap/test/test_corruption_check.c diff --git a/components/heap/test/test_corruption_check.c b/components/heap/test/test_corruption_check.c new file mode 100644 index 0000000000..25ca70977d --- /dev/null +++ b/components/heap/test/test_corruption_check.c @@ -0,0 +1,64 @@ +/* + * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Unlicense OR CC0-1.0 + */ +#include "unity.h" +#include "stdio.h" + +#include "esp_heap_caps.h" + +/* executing multi_heap_internal_check_block_poisoning() + * takes longer on external RAM and therefore the timeout + * in the test of 30 seconds is exceeded. Execute the test + * on a smaller memory chunk + */ +#ifdef CONFIG_SPIRAM +const size_t MALLOC_SIZE = 16; +#else +const size_t MALLOC_SIZE = 64; +#endif +const uint8_t CORRUPTED_VALUE = 0xaa; + +/* This test will corrupt the memory of a free block in the heap and check + * that in the case of comprehensive poisoning the heap corruption is detected + * by heap_caps_check_integrity(). For light poisoning and no poisoning, the test will + * check that heap_caps_check_integrity() does not report the corruption. + */ +TEST_CASE("multi_heap poisoning detection", "[heap]") +{ + /* malloc some memory to get a pointer */ + uint8_t *ptr = heap_caps_malloc(MALLOC_SIZE, MALLOC_CAP_8BIT); + + /* free the memory to free the block but keep the pointer in mind */ + heap_caps_free(ptr); + + /* variable used in the test */ + uint8_t original_value = 0x00; + + for (size_t i = 0; i < MALLOC_SIZE; i++) + { + /* keep the good value in store in order to check that when we set the byte back + * to its original value, heap_caps_check_integrity() no longer returns the + * heap corruption. */ + original_value = ptr[i]; + + /* set corrupted value in the free memory*/ + ptr[i] = CORRUPTED_VALUE; + + bool is_heap_ok = heap_caps_check_integrity(MALLOC_CAP_8BIT, true); +#ifdef CONFIG_HEAP_POISONING_COMPREHENSIVE + /* check that heap_caps_check_integrity() detects the corruption */ + TEST_ASSERT_FALSE(is_heap_ok); +#else + /* the comprehensive corruption is not checked in the heap_caps_check_integrity() */ + TEST_ASSERT_TRUE(is_heap_ok); +#endif + /* fix the corruption by restoring the original value at ptr + i */ + ptr[i] = original_value; + + /* check that heap_caps_check_integrity() stops reporting the corruption */ + is_heap_ok = heap_caps_check_integrity(MALLOC_CAP_8BIT, true); + TEST_ASSERT_TRUE(is_heap_ok); + } +} diff --git a/components/heap/test_multi_heap_host/Makefile b/components/heap/test_multi_heap_host/Makefile index 6c10f8035a..7d97cbcc6f 100644 --- a/components/heap/test_multi_heap_host/Makefile +++ b/components/heap/test_multi_heap_host/Makefile @@ -17,7 +17,7 @@ INCLUDE_FLAGS = -I../include -I../../../tools/catch -I../tlsf GCOV ?= gcov -CPPFLAGS += $(INCLUDE_FLAGS) -D CONFIG_LOG_DEFAULT_LEVEL -g -fstack-protector-all -m32 -DCONFIG_HEAP_POISONING_COMPREHENSIVE +CPPFLAGS += $(INCLUDE_FLAGS) -D CONFIG_LOG_DEFAULT_LEVEL -g -fstack-protector-all -m32 CFLAGS += -Wall -Werror -fprofile-arcs -ftest-coverage CXXFLAGS += -std=c++11 -Wall -Werror -fprofile-arcs -ftest-coverage LDFLAGS += -lstdc++ -fprofile-arcs -ftest-coverage -m32 diff --git a/components/heap/test_multi_heap_host/test_multi_heap.cpp b/components/heap/test_multi_heap_host/test_multi_heap.cpp index e85cceddc6..c3cac1cad1 100644 --- a/components/heap/test_multi_heap_host/test_multi_heap.cpp +++ b/components/heap/test_multi_heap_host/test_multi_heap.cpp @@ -2,6 +2,8 @@ #include "multi_heap.h" #include "../multi_heap_config.h" +#include "../tlsf/tlsf_common.h" +#include "../tlsf/tlsf_block_functions.h" #include #include @@ -523,3 +525,65 @@ TEST_CASE("multi_heap allocation overhead", "[multi_heap]") multi_heap_free(heap, x); } + +/* This test will corrupt the memory of a free block in the heap and check + * that in the case of comprehensive poisoning the heap corruption is detected + * by multi_heap_check(). For light poisoning and no poisoning, the test will + * check that multi_heap_check() does not report the corruption. + */ +TEST_CASE("multi_heap poisoning detection", "[multi_heap]") +{ + const size_t HEAP_SIZE = 4 * 1024; + + /* define heap related data */ + uint8_t heap_mem[HEAP_SIZE]; + memset(heap_mem, 0x00, HEAP_SIZE); + + /* register the heap memory. One free block only will be available */ + multi_heap_handle_t heap = multi_heap_register(heap_mem, HEAP_SIZE); + + /* offset in memory at which to find the first free memory byte */ + const size_t free_memory_offset = sizeof(multi_heap_info_t) + sizeof(control_t) + block_header_overhead; + + /* block header of the free block under test in the heap () */ + const block_header_t* block = (block_header_t*)(heap_mem + free_memory_offset - sizeof(block_header_t)); + + /* actual number of bytes potentially filled with the free pattern in the free block under test */ + const size_t effective_free_size = block_size(block) - block_header_overhead - offsetof(block_header_t, next_free); + + /* variable used in the test */ + size_t affected_byte = 0x00; + uint8_t original_value = 0x00; + uint8_t corrupted_value = 0x00; + + /* repeat the corruption a few times to cover more of the free memory */ + for (size_t i = 0; i < effective_free_size; i++) + { + /* corrupt random bytes in the heap (it needs to be bytes from free memory in + * order to check that the comprehensive poisoning is doing its job) */ + affected_byte = free_memory_offset + i; + corrupted_value = (rand() % UINT8_MAX) | 1; + + /* keep the good value in store in order to check that when we set the byte back + * to its original value, multi_heap_check() no longer returns the heap corruption. */ + original_value = heap_mem[affected_byte]; + + /* make sure we are not replacing the original value with the same value */ + heap_mem[affected_byte] ^= corrupted_value; + + bool is_heap_ok = multi_heap_check(heap, true); +#ifdef CONFIG_HEAP_POISONING_COMPREHENSIVE + /* check that multi_heap_check() detects the corruption */ + REQUIRE(is_heap_ok == false); +#else + /* the comprehensive corruption is not checked in the multi_heap_check() */ + REQUIRE(is_heap_ok == true); +#endif + /* fix the corruption */ + heap_mem[affected_byte] = original_value; + + /* check that multi_heap_check() stops reporting the corruption */ + is_heap_ok = multi_heap_check(heap, true); + REQUIRE(is_heap_ok == true); + } +} From 2e897c2e74ac2a9a37e97ccea75dbc90b2fce8c1 Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Mon, 29 Aug 2022 19:46:49 +0800 Subject: [PATCH 4/4] TLSF: fix the patch for tlsf_check function in ROM tlsf_check in the patch was not called because the the TLSF functions table in ROM was still pointing to the ROM implementation. --- components/esp_rom/CMakeLists.txt | 11 ++- components/esp_rom/patches/esp_rom_tlsf.c | 97 +++++++++++++++++--- components/heap/multi_heap.c | 4 +- components/heap/multi_heap_poisoning.c | 4 +- components/heap/test/test_corruption_check.c | 10 +- 5 files changed, 103 insertions(+), 23 deletions(-) diff --git a/components/esp_rom/CMakeLists.txt b/components/esp_rom/CMakeLists.txt index 5fe5dc2f50..de4858f7e1 100644 --- a/components/esp_rom/CMakeLists.txt +++ b/components/esp_rom/CMakeLists.txt @@ -21,7 +21,8 @@ else() "patches/esp_rom_regi2c.c" "patches/esp_rom_efuse.c") - if(CONFIG_ESP_ROM_HAS_HEAP_TLSF AND CONFIG_ESP_ROM_TLSF_CHECK_PATCH) + if(CONFIG_HEAP_TLSF_USE_ROM_IMPL AND CONFIG_ESP_ROM_TLSF_CHECK_PATCH) + # This file shall be included in the build if TLSF in ROM is activated list(APPEND sources "patches/esp_rom_tlsf.c") endif() @@ -217,6 +218,14 @@ else() # Regular app build endif() if(CONFIG_HEAP_TLSF_USE_ROM_IMPL) + # After registering the component, set the tlsf_set_rom_patches symbol as undefined + # to force the linker to integrate the whole `esp_rom_tlsf.c` object file inside the + # final binary. This is necessary because tlsf_set_rom_patches is a constructor, thus, + # there as no explicit reference/call to it in IDF. + if(CONFIG_ESP_ROM_TLSF_CHECK_PATCH) + target_link_libraries(${COMPONENT_LIB} PRIVATE "-u tlsf_set_rom_patches") + endif() + rom_linker_script("heap") endif() endif() diff --git a/components/esp_rom/patches/esp_rom_tlsf.c b/components/esp_rom/patches/esp_rom_tlsf.c index aaf9d2dc17..541b9a0b82 100644 --- a/components/esp_rom/patches/esp_rom_tlsf.c +++ b/components/esp_rom/patches/esp_rom_tlsf.c @@ -14,10 +14,18 @@ #include #include +#include #include "esp_rom_caps.h" #include "rom/tlsf.h" +/*! + * @brief Opaque types for TLSF implementation + */ +typedef void* tlsf_t; +typedef void* pool_t; +typedef void* tlsf_walker; + /* ---------------------------------------------------------------- * Bring certain inline functions, macro and structures from the * tlsf ROM implementation to be able to compile the patch. @@ -166,7 +174,7 @@ void tlsf_poison_check_pfunc_set(void *pfunc) #define tlsf_insist_no_assert(x) { if (!(x)) { status--; } } -int tlsf_check(void* tlsf) +int tlsf_check(tlsf_t tlsf) { int i, j; @@ -211,25 +219,24 @@ int tlsf_check(void* tlsf) mapping_insert(block_size(block), &fli, &sli); tlsf_insist_no_assert(fli == i && sli == j && "block size indexed in wrong list"); - block = block->next_free; - /* block_size(block) returns the size of the usable memory when the block is allocated. - * As the block under test is free, we need to subtract to the block size the next_free - * and prev_free fields of the block header as they are not a part of the usable memory - * when the block is free. In addition, we also need to subtract the size of prev_phys_block - * as this field is in fact part of the current free block and not part of the next (allocated) - * block. Check the comments in block_split function for more details. - */ - const size_t actual_free_block_size = block_size(block) - - offsetof(block_header_t, next_free) - - block_header_overhead; + /* block_size(block) returns the size of the usable memory when the block is allocated. + * As the block under test is free, we need to subtract to the block size the next_free + * and prev_free fields of the block header as they are not a part of the usable memory + * when the block is free. In addition, we also need to subtract the size of prev_phys_block + * as this field is in fact part of the current free block and not part of the next (allocated) + * block. Check the comments in block_split function for more details. + */ + const size_t actual_free_block_size = block_size(block) + - offsetof(block_header_t, next_free) + - block_header_overhead; if (s_poison_check_region != NULL) { - tlsf_insist_no_assert(s_poison_check_region((char *)block + sizeof(block_header_t), - actual_free_block_size, is_block_free, true /* print errors */)); + tlsf_insist_no_assert(s_poison_check_region((char *)block + sizeof(block_header_t), + actual_free_block_size, is_block_free, true /* print errors */)); } - + block = block->next_free; } } } @@ -238,3 +245,63 @@ int tlsf_check(void* tlsf) } #undef tlsf_insist_no_assert + +/* Set up the TLSF ROM patches here */ + +/*! + * @brief Structure to store all the functions of a TLSF implementation. + * The goal of this table is to change any of the address here in order + * to let the ROM code call another function implementation than the one + * in ROM. + */ +struct heap_tlsf_stub_table_t { + tlsf_t (*tlsf_create)(void* mem); + tlsf_t (*tlsf_create_with_pool)(void* mem, size_t bytes); + pool_t (*tlsf_get_pool)(tlsf_t tlsf); + pool_t (*tlsf_add_pool)(tlsf_t tlsf, void* mem, size_t bytes); + void (*tlsf_remove_pool)(tlsf_t tlsf, pool_t pool); + + void* (*tlsf_malloc)(tlsf_t tlsf, size_t size); + void* (*tlsf_memalign)(tlsf_t tlsf, size_t align, size_t size); + void* (*tlsf_memalign_offs)(tlsf_t tlsf, size_t align, size_t size, size_t offset); + void* (*tlsf_realloc)(tlsf_t tlsf, void* ptr, size_t size); + void (*tlsf_free)(tlsf_t tlsf, void* ptr); + + size_t (*tlsf_block_size)(void* ptr); + size_t (*tlsf_size)(void); + size_t (*tlsf_align_size)(void); + size_t (*tlsf_block_size_min)(void); + size_t (*tlsf_block_size_max)(void); + size_t (*tlsf_pool_overhead)(void); + size_t (*tlsf_alloc_overhead)(void); + + void (*tlsf_walk_pool)(pool_t pool, tlsf_walker walker, void* user); + + int (*tlsf_check)(tlsf_t tlsf); + int (*tlsf_check_pool)(pool_t pool); +}; + +/* We need the original table from the ROM */ +extern struct heap_tlsf_stub_table_t* heap_tlsf_table_ptr; + +/* We will copy the ROM table and modify the functions we patch */ +struct heap_tlsf_stub_table_t heap_tlsf_patch_table_ptr; + +/*! + * @brief Setup the TLSF ROM patches. + * This function must be called when setting up the heap. It will put in place the function patched + * from the ROM implementation. + * This function must not be defined as static, as it is marked as "undefined" in the linker flags + * (to force the linker to integrate the functions of this file inside the final binary) + */ +void __attribute__((constructor)) tlsf_set_rom_patches(void) +{ + /* Copy the ROM default table inside our table */ + memcpy(&heap_tlsf_patch_table_ptr, heap_tlsf_table_ptr, sizeof(struct heap_tlsf_stub_table_t)); + + /* Set the patched function here */ + heap_tlsf_patch_table_ptr.tlsf_check = tlsf_check; + + /* Set our table as the one to use in the ROM code */ + heap_tlsf_table_ptr = &heap_tlsf_patch_table_ptr; +} diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c index 9431a23771..8481268d55 100644 --- a/components/heap/multi_heap.c +++ b/components/heap/multi_heap.c @@ -81,7 +81,7 @@ typedef struct multi_heap_info { void* heap_data; } heap_t; -#ifdef CONFIG_HEAP_TLSF_USE_ROM_IMPL +#if CONFIG_HEAP_TLSF_USE_ROM_IMPL void _multi_heap_lock(void *lock) { @@ -103,7 +103,7 @@ void multi_heap_in_rom_init(void) multi_heap_os_funcs_init(&multi_heap_os_funcs); } -#else //#ifndef CONFIG_HEAP_TLSF_USE_ROM_IMPL +#else // CONFIG_HEAP_TLSF_USE_ROM_IMPL /* Return true if this block is free. */ static inline bool is_free(const block_header_t *block) diff --git a/components/heap/multi_heap_poisoning.c b/components/heap/multi_heap_poisoning.c index 399d5e30bf..9cb42b95d4 100644 --- a/components/heap/multi_heap_poisoning.c +++ b/components/heap/multi_heap_poisoning.c @@ -359,10 +359,10 @@ multi_heap_handle_t multi_heap_register(void *start, size_t size) memset(start, FREE_FILL_PATTERN, size); } #endif -#ifdef CONFIG_HEAP_TLSF_USE_ROM_IMPL +#if CONFIG_HEAP_TLSF_USE_ROM_IMPL tlsf_poison_fill_pfunc_set(multi_heap_internal_poison_fill_region); tlsf_poison_check_pfunc_set(multi_heap_internal_check_block_poisoning); -#endif +#endif // CONFIG_HEAP_TLSF_USE_ROM_IMPL return multi_heap_register_impl(start, size); } diff --git a/components/heap/test/test_corruption_check.c b/components/heap/test/test_corruption_check.c index 25ca70977d..9c4dd47eb2 100644 --- a/components/heap/test/test_corruption_check.c +++ b/components/heap/test/test_corruption_check.c @@ -47,15 +47,19 @@ TEST_CASE("multi_heap poisoning detection", "[heap]") ptr[i] = CORRUPTED_VALUE; bool is_heap_ok = heap_caps_check_integrity(MALLOC_CAP_8BIT, true); -#ifdef CONFIG_HEAP_POISONING_COMPREHENSIVE + + /* fix the corruption by restoring the original value at ptr + i. + * We need to do this before the ASSERT because they may print a message. + * Using print allocates memory on the heap, so the heap has to be fixed. */ + ptr[i] = original_value; + +#if CONFIG_HEAP_POISONING_COMPREHENSIVE /* check that heap_caps_check_integrity() detects the corruption */ TEST_ASSERT_FALSE(is_heap_ok); #else /* the comprehensive corruption is not checked in the heap_caps_check_integrity() */ TEST_ASSERT_TRUE(is_heap_ok); #endif - /* fix the corruption by restoring the original value at ptr + i */ - ptr[i] = original_value; /* check that heap_caps_check_integrity() stops reporting the corruption */ is_heap_ok = heap_caps_check_integrity(MALLOC_CAP_8BIT, true);