Merge branch 'feature/mem-corruption-check-when-comprehensif-poisoning-v4.4' into 'release/v4.4'

heap: Provide memory corruption check when poisoning is active (V4.4)

See merge request espressif/esp-idf!19778

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;
 			}
 		}

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;
     }

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);
+    }
+}

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

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 <string.h>
 #include <assert.h>
@@ -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);
+    }
+}