From 1fac58deb7f086a1f9fb1fa48a7d955f54c374d9 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Mon, 11 Mar 2019 10:49:51 +1100 Subject: [PATCH] heap: Add integer overflow checks on MALLOC_CAP_32BIT & MALLOC_CAP_EXEC --- components/heap/heap_caps.c | 39 ++++++++++--------- components/heap/heap_caps_init.c | 4 +- components/heap/heap_private.h | 2 + components/heap/test/test_malloc.c | 7 +++- components/soc/esp32/include/soc/soc.h | 2 + .../soc/include/soc/soc_memory_layout.h | 8 ++++ 6 files changed, 42 insertions(+), 20 deletions(-) diff --git a/components/heap/heap_caps.c b/components/heap/heap_caps.c index 90b8821f5..3ab376115 100644 --- a/components/heap/heap_caps.c +++ b/components/heap/heap_caps.c @@ -33,26 +33,21 @@ possible. This should optimize the amount of RAM accessible to the code without /* This takes a memory chunk in a region that can be addressed as both DRAM as well as IRAM. It will convert it to - IRAM in such a way that it can be later freed. It assumes both the address as wel as the length to be word-aligned. + IRAM in such a way that it can be later freed. It assumes both the address as well as the length to be word-aligned. It returns a region that's 1 word smaller than the region given because it stores the original Dram address there. - - In theory, we can also make this work by prepending a struct that looks similar to the block link struct used by the - heap allocator itself, which will allow inspection tools relying on any block returned from any sort of malloc to - have such a block in front of it, work. We may do this later, if/when there is demand for it. For now, a simple - pointer is used. */ IRAM_ATTR static void *dram_alloc_to_iram_addr(void *addr, size_t len) { - uint32_t dstart = (int)addr; //First word - uint32_t dend = ((int)addr) + len - 4; //Last word - assert(dstart >= SOC_DIRAM_DRAM_LOW); - assert(dend <= SOC_DIRAM_DRAM_HIGH); + uintptr_t dstart = (uintptr_t)addr; //First word + uintptr_t dend = dstart + len - 4; //Last word + assert(esp_ptr_in_diram_dram((void *)dstart)); + assert(esp_ptr_in_diram_dram((void *)dend)); assert((dstart & 3) == 0); assert((dend & 3) == 0); uint32_t istart = SOC_DIRAM_IRAM_LOW + (SOC_DIRAM_DRAM_HIGH - dend); uint32_t *iptr = (uint32_t *)istart; *iptr = dstart; - return (void *)(iptr + 1); + return iptr + 1; } bool heap_caps_match(const heap_t *heap, uint32_t caps) @@ -67,6 +62,12 @@ IRAM_ATTR void *heap_caps_malloc( size_t size, uint32_t caps ) { void *ret = NULL; + if (size > HEAP_SIZE_MAX) { + // Avoids int overflow when adding small numbers to size, or + // calculating 'end' from start+size, by limiting 'size' to the possible range + return NULL; + } + if (caps & MALLOC_CAP_EXEC) { //MALLOC_CAP_EXEC forces an alloc from IRAM. There is a region which has both this as well as the following //caps, but the following caps are not possible for IRAM. Thus, the combination is impossible and we return @@ -82,7 +83,7 @@ IRAM_ATTR void *heap_caps_malloc( size_t size, uint32_t caps ) /* 32-bit accessible RAM should allocated in 4 byte aligned sizes * (Future versions of ESP-IDF should possibly fail if an invalid size is requested) */ - size = (size + 3) & (~3); + size = (size + 3) & (~3); // int overflow checked above } for (int prio = 0; prio < SOC_MEMORY_TYPE_NO_PRIOS; prio++) { @@ -97,13 +98,13 @@ IRAM_ATTR void *heap_caps_malloc( size_t size, uint32_t caps ) //doesn't cover, see if they're available in other prios. if ((get_all_caps(heap) & caps) == caps) { //This heap can satisfy all the requested capabilities. See if we can grab some memory using it. - if ((caps & MALLOC_CAP_EXEC) && heap->start >= SOC_DIRAM_DRAM_LOW && heap->start < SOC_DIRAM_DRAM_HIGH) { + if ((caps & MALLOC_CAP_EXEC) && esp_ptr_in_diram_dram((void *)heap->start)) { //This is special, insofar that what we're going to get back is a DRAM address. If so, //we need to 'invert' it (lowest address in DRAM == highest address in IRAM and vice-versa) and //add a pointer to the DRAM equivalent before the address we're going to return. - ret = multi_heap_malloc(heap->heap, size + 4); + ret = multi_heap_malloc(heap->heap, size + 4); // int overflow checked above if (ret != NULL) { - return dram_alloc_to_iram_addr(ret, size + 4); + return dram_alloc_to_iram_addr(ret, size + 4); // int overflow checked above } } else { //Just try to alloc, nothing special. @@ -250,13 +251,11 @@ IRAM_ATTR static heap_t *find_containing_heap(void *ptr ) IRAM_ATTR void heap_caps_free( void *ptr) { - intptr_t p = (intptr_t)ptr; - if (ptr == NULL) { return; } - if ((p >= SOC_DIRAM_IRAM_LOW) && (p <= SOC_DIRAM_IRAM_HIGH)) { + if (esp_ptr_in_diram_iram(ptr)) { //Memory allocated here is actually allocated in the DRAM alias region and //cannot be de-allocated as usual. dram_alloc_to_iram_addr stores a pointer to //the equivalent DRAM address, though; free that. @@ -280,6 +279,10 @@ IRAM_ATTR void *heap_caps_realloc( void *ptr, size_t size, int caps) return NULL; } + if (size > HEAP_SIZE_MAX) { + return NULL; + } + heap_t *heap = find_containing_heap(ptr); assert(heap != NULL && "realloc() pointer is outside heap areas"); diff --git a/components/heap/heap_caps_init.c b/components/heap/heap_caps_init.c index 55d40d9c6..30b9dc299 100644 --- a/components/heap/heap_caps_init.c +++ b/components/heap/heap_caps_init.c @@ -31,7 +31,9 @@ struct registered_heap_ll registered_heaps; static void register_heap(heap_t *region) { - region->heap = multi_heap_register((void *)region->start, region->end - region->start); + size_t heap_size = region->end - region->start; + assert(heap_size <= HEAP_SIZE_MAX); + region->heap = multi_heap_register((void *)region->start, heap_size); if (region->heap != NULL) { ESP_EARLY_LOGD(TAG, "New heap initialised at %p", region->heap); } diff --git a/components/heap/heap_private.h b/components/heap/heap_private.h index a8a0ac9fd..f5a61f327 100644 --- a/components/heap/heap_private.h +++ b/components/heap/heap_private.h @@ -28,6 +28,8 @@ extern "C" { for heap_caps_init.c to share heap information with heap_caps.c */ +#define HEAP_SIZE_MAX (SOC_MAX_CONTIGUOUS_RAM_SIZE) + /* Type for describing each registered heap */ typedef struct heap_t_ { uint32_t caps[SOC_MEMORY_TYPE_NO_PRIOS]; ///< Capabilities for the type of memory in this heap (as a prioritised set). Copied from soc_memory_types so it's in RAM not flash. diff --git a/components/heap/test/test_malloc.c b/components/heap/test/test_malloc.c index 52dc5d171..703270c9a 100644 --- a/components/heap/test/test_malloc.c +++ b/components/heap/test/test_malloc.c @@ -109,13 +109,18 @@ void* test_calloc_wrapper(size_t count, size_t size) TEST_CASE("alloc overflows should all fail", "[heap]") { - /* allocates 8 bytes */ + /* allocates 8 bytes if size_t overflows */ TEST_ASSERT_NULL(test_calloc_wrapper(SIZE_MAX / 2 + 4, 2)); /* will overflow if any poisoning is enabled (should fail for sensible OOM reasons, otherwise) */ TEST_ASSERT_NULL(test_malloc_wrapper(SIZE_MAX - 1)); TEST_ASSERT_NULL(test_calloc_wrapper(SIZE_MAX - 1, 1)); + + /* will overflow when the size is rounded up to word align it */ + TEST_ASSERT_NULL(heap_caps_malloc(SIZE_MAX-1, MALLOC_CAP_32BIT)); + + TEST_ASSERT_NULL(heap_caps_malloc(SIZE_MAX-1, MALLOC_CAP_EXEC)); } TEST_CASE("unreasonable allocs should all fail", "[heap]") diff --git a/components/soc/esp32/include/soc/soc.h b/components/soc/esp32/include/soc/soc.h index 64a0fab3f..96dea8023 100644 --- a/components/soc/esp32/include/soc/soc.h +++ b/components/soc/esp32/include/soc/soc.h @@ -72,6 +72,8 @@ #define SOC_EXTRAM_DATA_LOW 0x3F800000 #define SOC_EXTRAM_DATA_HIGH 0x3FC00000 +#define SOC_MAX_CONTIGUOUS_RAM_SIZE 0x400000 ///< Largest span of contiguous memory (DRAM or IRAM) in the address space + #define DR_REG_DPORT_BASE 0x3ff00000 #define DR_REG_AES_BASE 0x3ff01000 diff --git a/components/soc/include/soc/soc_memory_layout.h b/components/soc/include/soc/soc_memory_layout.h index 68c87623c..fd7132cfa 100644 --- a/components/soc/include/soc/soc_memory_layout.h +++ b/components/soc/include/soc/soc_memory_layout.h @@ -189,3 +189,11 @@ inline static bool IRAM_ATTR esp_ptr_in_drom(const void *p) { inline static bool IRAM_ATTR esp_ptr_in_dram(const void *p) { return ((intptr_t)p >= SOC_DRAM_LOW && (intptr_t)p < SOC_DRAM_HIGH); } + +inline static bool IRAM_ATTR esp_ptr_in_diram_dram(const void *p) { + return ((intptr_t)p >= SOC_DIRAM_DRAM_LOW && (intptr_t)p < SOC_DIRAM_DRAM_HIGH); +} + +inline static bool IRAM_ATTR esp_ptr_in_diram_iram(const void *p) { + return ((intptr_t)p >= SOC_DIRAM_IRAM_LOW && (intptr_t)p < SOC_DIRAM_IRAM_HIGH); +}