From f13e25d156945abebcda5b64d0256cd3bd04be4e Mon Sep 17 00:00:00 2001 From: Mahavir Jain Date: Thu, 24 Mar 2022 12:25:37 +0530 Subject: [PATCH 1/2] heap: Fix regression in `heap_caps_add_region` API related to address range checks Regression was introduced in 32408b718f7c7699c0ef4af1fc768a696e2d6b74, which disallowed addition of heap region with following condition: `new_start < start && new_end == start` This caused issues in Bluetooth APIs `esp_bt_mem_release` or `esp_bt_controller_mem_release`. This commit fixes the problem and also adds API documentation for supported memory address ranges in heap add region APIs. --- components/heap/heap_caps_init.c | 41 +++++++++++--------- components/heap/include/esp_heap_caps_init.h | 28 +++++++++++++ 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/components/heap/heap_caps_init.c b/components/heap/heap_caps_init.c index 6b770650c2..6c28af1a3d 100644 --- a/components/heap/heap_caps_init.c +++ b/components/heap/heap_caps_init.c @@ -162,15 +162,8 @@ esp_err_t heap_caps_add_region(intptr_t start, intptr_t end) return ESP_ERR_NOT_FOUND; } -esp_err_t heap_caps_add_region_with_caps(const uint32_t caps[], intptr_t start, intptr_t end) +bool heap_caps_check_add_region_allowed(intptr_t heap_start, intptr_t heap_end, intptr_t start, intptr_t end) { - esp_err_t err = ESP_FAIL; - if (caps == NULL || start == 0 || end == 0 || end <= start) { - return ESP_ERR_INVALID_ARG; - } - - //Check if region overlaps the start and/or end of an existing region. If so, the - //region is invalid (or maybe added twice) /* * We assume that in any region, the "start" must be stictly less than the end. * Specially, the 3rd scenario can be allowed. For example, allocate memory from heap, @@ -183,27 +176,39 @@ esp_err_t heap_caps_add_region_with_caps(const uint32_t caps[], intptr_t start, * the existing heap region s(tart) e(nd) * |----------------------| * - * 1.add region (e1start; + * 1.add region (e1=s) |-----------------| wrong: bool condition_2 = start < heap->start && end >= heap->start; + * 2.add region (s2s) |-----------------| wrong: bool condition_2 = start < heap_start && end > heap_start; * |---------------------------------| wrong * - * 3.add region (s3>=s && e3= heap->start && end < heap->end; + * 3.add region (s3>=s && e3= heap_start && end < heap_end; * |--------------| correct * - * 4.add region (s4=e) |----------------------| wrong: bool condition_4 = start < heap->end && end >= heap->end; + * 4.add region (s4e) |------------------------| wrong: bool condition_4 = start < heap_end && end > heap_end; * |---------------------| wrong * - * 5.add region (s5>=e) |----| correct: bool condition_5 = start >= heap->end; + * 5.add region (s5>=e) |----| correct: bool condition_5 = start >= heap_end; */ + bool condition_2 = start < heap_start && end > heap_start; // if true then region not allowed + bool condition_4 = start < heap_end && end > heap_end; // if true then region not allowed + + return (condition_2 || condition_4) ? false: true; +} + +esp_err_t heap_caps_add_region_with_caps(const uint32_t caps[], intptr_t start, intptr_t end) +{ + esp_err_t err = ESP_FAIL; + if (caps == NULL || start == 0 || end == 0 || end <= start) { + return ESP_ERR_INVALID_ARG; + } + + //Check if region overlaps the start and/or end of an existing region. If so, the + //region is invalid (or maybe added twice) heap_t *heap; SLIST_FOREACH(heap, ®istered_heaps, next) { - bool condition_2 = start < heap->start && end >= heap->start; //if 1, wrong - bool condition_4 = start < heap->end && end >= heap->end; //if 1, wrong - bool wrong_region = condition_2 || condition_4; - - if (wrong_region) { + if (!heap_caps_check_add_region_allowed(heap->start, heap->end, start, end)) { + ESP_EARLY_LOGD(TAG, "invalid overlap detected with existing heap region"); return ESP_FAIL; } } diff --git a/components/heap/include/esp_heap_caps_init.h b/components/heap/include/esp_heap_caps_init.h index 50aab38493..b730b5fa71 100644 --- a/components/heap/include/esp_heap_caps_init.h +++ b/components/heap/include/esp_heap_caps_init.h @@ -49,6 +49,20 @@ void heap_caps_enable_nonos_stack_heaps(void); * * Use heap_caps_add_region_with_caps() to register a region with custom capabilities. * + * @note Please refer to following example for memory regions allowed for addition to heap based on an existing region + * (address range for demonstration purpose only): + @verbatim + Existing region: 0x1000 <-> 0x3000 + New region: 0x1000 <-> 0x3000 (Allowed) + New region: 0x1000 <-> 0x2000 (Allowed) + New region: 0x0000 <-> 0x1000 (Allowed) + New region: 0x3000 <-> 0x4000 (Allowed) + New region: 0x0000 <-> 0x2000 (NOT Allowed) + New region: 0x0000 <-> 0x4000 (NOT Allowed) + New region: 0x1000 <-> 0x4000 (NOT Allowed) + New region: 0x2000 <-> 0x4000 (NOT Allowed) + @endverbatim + * * @param start Start address of new region. * @param end End address of new region. * @@ -63,6 +77,20 @@ esp_err_t heap_caps_add_region(intptr_t start, intptr_t end); * * Similar to heap_caps_add_region(), only custom memory capabilities are specified by the caller. * + * @note Please refer to following example for memory regions allowed for addition to heap based on an existing region + * (address range for demonstration purpose only): + @verbatim + Existing region: 0x1000 <-> 0x3000 + New region: 0x1000 <-> 0x3000 (Allowed) + New region: 0x1000 <-> 0x2000 (Allowed) + New region: 0x0000 <-> 0x1000 (Allowed) + New region: 0x3000 <-> 0x4000 (Allowed) + New region: 0x0000 <-> 0x2000 (NOT Allowed) + New region: 0x0000 <-> 0x4000 (NOT Allowed) + New region: 0x1000 <-> 0x4000 (NOT Allowed) + New region: 0x2000 <-> 0x4000 (NOT Allowed) + @endverbatim + * * @param caps Ordered array of capability masks for the new region, in order of priority. Must have length * SOC_MEMORY_TYPE_NO_PRIOS. Does not need to remain valid after the call returns. * @param start Start address of new region. From 98b8ca6475fc75e7892bef0e7228bbb0b169822d Mon Sep 17 00:00:00 2001 From: Mahavir Jain Date: Thu, 24 Mar 2022 19:55:11 +0530 Subject: [PATCH 2/2] heap: add test case for region overlap check condition --- components/heap/heap_caps_init.c | 1 + components/heap/test/test_runtime_heap_reg.c | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/components/heap/heap_caps_init.c b/components/heap/heap_caps_init.c index 6c28af1a3d..1b6892bb10 100644 --- a/components/heap/heap_caps_init.c +++ b/components/heap/heap_caps_init.c @@ -162,6 +162,7 @@ esp_err_t heap_caps_add_region(intptr_t start, intptr_t end) return ESP_ERR_NOT_FOUND; } +/* This API is used for internal test purpose and hence its not marked as static */ bool heap_caps_check_add_region_allowed(intptr_t heap_start, intptr_t heap_end, intptr_t start, intptr_t end) { /* diff --git a/components/heap/test/test_runtime_heap_reg.c b/components/heap/test/test_runtime_heap_reg.c index 305177bcd5..c7a9dc493c 100644 --- a/components/heap/test/test_runtime_heap_reg.c +++ b/components/heap/test/test_runtime_heap_reg.c @@ -71,3 +71,20 @@ TEST_CASE("Add .bss memory to heap region runtime", "[heap][ignore]") /* Twice add must be failed */ TEST_ASSERT( (heap_caps_add_region((intptr_t)s_buffer, (intptr_t)s_buffer + BUF_SZ) != ESP_OK) ); } + +extern esp_err_t heap_caps_check_add_region_allowed(intptr_t heap_start, intptr_t heap_end, intptr_t start, intptr_t end); + +TEST_CASE("Add heap region address range checks", "[heap]") +{ + const intptr_t heap_start = 0x1000; + const intptr_t heap_end = 0x3000; + + TEST_ASSERT_TRUE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x0, 0x1000)); + TEST_ASSERT_TRUE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x1000, 0x2000)); + TEST_ASSERT_TRUE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x1000, 0x3000)); + TEST_ASSERT_TRUE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x3000, 0x4000)); + TEST_ASSERT_FALSE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x0, 0x2000)); + TEST_ASSERT_FALSE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x0, 0x4000)); + TEST_ASSERT_FALSE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x1000, 0x4000)); + TEST_ASSERT_FALSE(heap_caps_check_add_region_allowed(heap_start, heap_end, 0x2000, 0x4000)); +}