From 6536e51a32c81ef916a6df747c1a1e97941daef8 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Thu, 25 Aug 2022 13:12:53 +0200 Subject: [PATCH 1/2] heap: Provide tlsf_check_hook() mechanism to check for memory corruption Add a call to tlsf_check_hook() in tlsf_check() that calls multi_heap_internal_check_block_poisoning() and check the memory of every free blocks when heap poisoning is active. --- components/heap/heap_tlsf.c | 26 ++++++++++++++++++++++++-- components/heap/multi_heap.c | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/components/heap/heap_tlsf.c b/components/heap/heap_tlsf.c index e5f826011f..8251796339 100644 --- a/components/heap/heap_tlsf.c +++ b/components/heap/heap_tlsf.c @@ -493,7 +493,7 @@ typedef struct integrity_t int status; } integrity_t; -#define tlsf_insist(x) { tlsf_assert(x); if (!(x)) { status--; } } +#define tlsf_insist(x) { if (!(x)) { status--; } } static void integrity_walker(void* ptr, size_t size, int used, void* user) { @@ -512,6 +512,10 @@ static void integrity_walker(void* ptr, size_t size, int used, void* user) integ->status += status; } +#ifdef MULTI_HEAP_POISONING +extern bool tlsf_check_hook(void *start, size_t size, bool is_free); +#endif + int tlsf_check(tlsf_t tlsf) { int i, j; @@ -548,7 +552,8 @@ int tlsf_check(tlsf_t tlsf) while (block != &control->block_null) { int fli, sli; - tlsf_insist(block_is_free(block) && "block should be free"); + const bool is_block_free = block_is_free(block); + tlsf_insist(is_block_free && "block should be free"); tlsf_insist(!block_is_prev_free(block) && "blocks should have coalesced"); tlsf_insist(!block_is_free(block_next(block)) && "blocks should have coalesced"); tlsf_insist(block_is_prev_free(block_next(block)) && "block should be free"); @@ -556,6 +561,23 @@ int tlsf_check(tlsf_t tlsf) mapping_insert(block_size(block), &fli, &sli); tlsf_insist(fli == i && sli == j && "block size indexed in wrong list"); + +#ifdef MULTI_HEAP_POISONING + /* 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; + + tlsf_insist(tlsf_check_hook((void*)block + sizeof(block_header_t), + actual_free_block_size, is_block_free)); +#endif + block = block->next_free; } } diff --git a/components/heap/multi_heap.c b/components/heap/multi_heap.c index 43790389b7..01775d98a7 100644 --- a/components/heap/multi_heap.c +++ b/components/heap/multi_heap.c @@ -290,13 +290,47 @@ 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; } From 55a2b55bdb58a5f84fa4043d3a15571db1cd9197 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Wed, 10 Aug 2022 16:19:04 +0200 Subject: [PATCH 2/2] 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 0827e34695..9945f5409c 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 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..4c45634fb4 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 "../heap_tlsf.h" +#include "../heap_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); + } +}