Merge branch 'bugfix/heap_caps_int_overflows_v3.2' into 'release/v3.2'

heap: Add integer overflow checks on MALLOC_CAP_32BIT & MALLOC_CAP_EXEC (v3.2)

See merge request idf/esp-idf!4569

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
+    uintptr_t dstart = (uintptr_t)addr; //First word
-    uint32_t dend = ((int)addr) + len - 4; //Last word
+    uintptr_t dend = dstart + len - 4; //Last word
-    assert(dstart >= SOC_DIRAM_DRAM_LOW);
+    assert(esp_ptr_in_diram_dram((void *)dstart));
-    assert(dend <= SOC_DIRAM_DRAM_HIGH);
+    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");

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

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.

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]")

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

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