From 3b9450b59d9f4d24e2aed956487c4ec016401a26 Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Wed, 27 Sep 2023 15:04:26 +0200 Subject: [PATCH] fix(heap): Fixed integrity check on used blocks by the tlsf component This commit updates the tlsf submodule to include the modification made in the component aiming to perform integrity check on all blocks (not only the free ones). Added test to test the fix in test_apps/heap_tests. Fixes https://github.com/espressif/esp-idf/issues/12231 --- components/heap/heap_tlsf.c | 41 ++++++++-------- components/heap/test/test_corruption_check.c | 50 ++++++++++++++++++++ 2 files changed, 71 insertions(+), 20 deletions(-) diff --git a/components/heap/heap_tlsf.c b/components/heap/heap_tlsf.c index 334b88eb5ff..fec557a743d 100644 --- a/components/heap/heap_tlsf.c +++ b/components/heap/heap_tlsf.c @@ -533,6 +533,10 @@ typedef struct integrity_t #define tlsf_insist(x) { if (!(x)) { status--; } } +#ifdef MULTI_HEAP_POISONING +extern bool tlsf_check_hook(void *start, size_t size, bool is_free); +#endif + static void integrity_walker(void* ptr, size_t size, int used, void* user) { block_header_t* block = block_from_ptr(ptr); @@ -546,14 +550,27 @@ static void integrity_walker(void* ptr, size_t size, int used, void* user) tlsf_insist(integ->prev_status == this_prev_status && "prev status incorrect"); tlsf_insist(size == this_block_size && "block size incorrect"); +#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 = used ? this_block_size : + this_block_size - offsetof(block_header_t, next_free)- block_header_overhead; + + void* ptr_block = used ? (void*)block + block_start_offset : + (void*)block + sizeof(block_header_t); + + tlsf_insist(tlsf_check_hook(ptr_block, actual_free_block_size, !used)); +#endif // MULTI_HEAP_POISONING + integ->prev_status = this_status; 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; @@ -600,22 +617,6 @@ int tlsf_check(tlsf_t tlsf) mapping_insert(control, 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/test/test_corruption_check.c b/components/heap/test/test_corruption_check.c index 25ca70977d3..323b0a7663c 100644 --- a/components/heap/test/test_corruption_check.c +++ b/components/heap/test/test_corruption_check.c @@ -62,3 +62,53 @@ TEST_CASE("multi_heap poisoning detection", "[heap]") TEST_ASSERT_TRUE(is_heap_ok); } } + +#if !defined(CONFIG_HEAP_TLSF_USE_ROM_IMPL) + +#ifdef CONFIG_HEAP_TASK_TRACKING +#define HEAD_CANARY_OFFSET 3 // head canary | task tracking | allocated size +#else +#define HEAD_CANARY_OFFSET 2 // head canary | allocated size +#endif // CONFIG_HEAP_TASK_TRACKING + +#define TAIL_CANARY_OFFSET 1 + +/* This test will corrupt the canary of a allocated memory block and call the + * heap_caps_check_integrity() function to check that the corruption is detected. + */ +TEST_CASE("canary corruption in light or comprehensive poisoning mode", "[heap]") +{ + const uint8_t allocation_size = 1 * sizeof(uint32_t); + /* malloc some memory to get a pointer */ + uint32_t *ptr = heap_caps_malloc(allocation_size, MALLOC_CAP_DEFAULT); + TEST_ASSERT_NOT_NULL(ptr); + + /* corrupt the head canary */ + uint32_t canary = ptr[-HEAD_CANARY_OFFSET]; + ptr[-HEAD_CANARY_OFFSET] = 0xdeadbeef; + + /* call the integrity check function and verify that it returns 0 (corruption detected) */ + bool is_corrupted = !heap_caps_check_integrity(MALLOC_CAP_DEFAULT, false); + TEST_ASSERT_TRUE(is_corrupted); + + /* fix the head canary */ + ptr[-HEAD_CANARY_OFFSET] = canary; + + /* re run the corruption check to make sure the function returns no corruption */ + is_corrupted = !heap_caps_check_integrity(MALLOC_CAP_DEFAULT, false); + TEST_ASSERT_FALSE(is_corrupted); + + /* corrupt tail canary */ + canary = ptr[TAIL_CANARY_OFFSET]; + ptr[TAIL_CANARY_OFFSET] = 0xdeadbeef; + + /* call the integrity check function and verify that it returns 0 (corruption detected) */ + is_corrupted = !heap_caps_check_integrity(MALLOC_CAP_DEFAULT, false); + TEST_ASSERT_TRUE(is_corrupted); + + /* clear the corruption and free the pointer before returning */ + ptr[TAIL_CANARY_OFFSET] = canary; + heap_caps_free(ptr); +} + +#endif // !CONFIG_HEAP_TLSF_USE_ROM_IMPL