diff --git a/components/esp_ringbuf/include/freertos/ringbuf.h b/components/esp_ringbuf/include/freertos/ringbuf.h index 67919750724..127399b2fff 100644 --- a/components/esp_ringbuf/include/freertos/ringbuf.h +++ b/components/esp_ringbuf/include/freertos/ringbuf.h @@ -73,6 +73,7 @@ typedef struct xSTATIC_RINGBUFFER { * @param[in] xBufferType Type of ring buffer, see documentation. * * @note xBufferSize of no-split/allow-split buffers will be rounded up to the nearest 32-bit aligned size. + * The aligned size must be large enough to hold two item headers. * * @return A handle to the created ring buffer, or NULL in case of error. */ @@ -87,6 +88,9 @@ RingbufHandle_t xRingbufferCreate(size_t xBufferSize, RingbufferType_t xBufferTy * @param[in] xItemSize Size of each item to be put into the ring buffer * @param[in] xItemNum Maximum number of items the buffer needs to hold simultaneously * + * @note Returns NULL if xItemNum is 0, the calculated buffer size overflows, + * or the resulting buffer is too small to hold two item headers. + * * @return A RingbufHandle_t handle to the created ring buffer, or NULL in case of error. */ RingbufHandle_t xRingbufferCreateNoSplit(size_t xItemSize, size_t xItemNum); @@ -102,8 +106,9 @@ RingbufHandle_t xRingbufferCreateNoSplit(size_t xItemSize, size_t xItemNum); * which will be used to hold the ring buffer's data structure * * @note xBufferSize of no-split/allow-split buffers MUST be 32-bit aligned. + * The aligned size must be large enough to hold two item headers. * - * @return A handle to the created ring buffer + * @return A handle to the created ring buffer, or NULL in case of error. */ RingbufHandle_t xRingbufferCreateStatic(size_t xBufferSize, RingbufferType_t xBufferType, @@ -528,6 +533,10 @@ BaseType_t xRingbufferGetStaticBuffer(RingbufHandle_t xRingbuffer, uint8_t **ppu * * @note A queue created using this function must only be deleted using * vRingbufferDeleteWithCaps() + * @note xBufferSize of no-split/allow-split buffers will be rounded up to the + * nearest 32-bit aligned size. The aligned size must be large enough to hold + * two item headers. + * * @param[in] xBufferSize Size of the buffer in bytes * @param[in] xBufferType Type of ring buffer, see documentation. * @param[in] uxMemoryCaps Memory capabilities of the queue's memory (see diff --git a/components/esp_ringbuf/ringbuf.c b/components/esp_ringbuf/ringbuf.c index f205f9afad3..d4b4b69052a 100644 --- a/components/esp_ringbuf/ringbuf.c +++ b/components/esp_ringbuf/ringbuf.c @@ -91,6 +91,11 @@ static void prvInitializeNewRingbuffer(size_t xBufferSize, Ringbuffer_t *pxNewRingbuffer, uint8_t *pucRingbufferStorage); +//Validate and align buffer size for dynamically allocated ring buffers +static BaseType_t prvGetAlignedBufferSize(size_t xBufferSize, + RingbufferType_t xBufferType, + size_t *pxAlignedBufferSize); + //Calculate current amount of free space (in bytes) in the ring buffer static size_t prvGetFreeSize(Ringbuffer_t *pxRingbuffer); @@ -204,6 +209,29 @@ static BaseType_t prvReceiveGenericFromISR(Ringbuffer_t *pxRingbuffer, // ------------------------------------------------ Static Functions --------------------------------------------------- +static BaseType_t prvGetAlignedBufferSize(size_t xBufferSize, + RingbufferType_t xBufferType, + size_t *pxAlignedBufferSize) +{ + if (xBufferType >= RINGBUF_TYPE_MAX || xBufferSize == 0) { + return pdFALSE; + } + + //No-split/allow-split buffers must be large enough to avoid underflowing xMaxItemSize + if (xBufferType != RINGBUF_TYPE_BYTEBUF) { + if (xBufferSize > SIZE_MAX - rbALIGN_MASK) { + return pdFALSE; //Alignment would overflow + } + xBufferSize = rbALIGN_SIZE(xBufferSize); + if (xBufferSize < rbHEADER_SIZE * 2) { + return pdFALSE; + } + } + + *pxAlignedBufferSize = xBufferSize; + return pdTRUE; +} + static void prvInitializeNewRingbuffer(size_t xBufferSize, RingbufferType_t xBufferType, Ringbuffer_t *pxNewRingbuffer, @@ -944,10 +972,11 @@ RingbufHandle_t xRingbufferCreate(size_t xBufferSize, RingbufferType_t xBufferTy configASSERT(xBufferSize > 0); configASSERT(xBufferType < RINGBUF_TYPE_MAX); - //Allocate memory - if (xBufferType != RINGBUF_TYPE_BYTEBUF) { - xBufferSize = rbALIGN_SIZE(xBufferSize); //xBufferSize is rounded up for no-split/allow-split buffers + if (prvGetAlignedBufferSize(xBufferSize, xBufferType, &xBufferSize) != pdTRUE) { + return NULL; } + + //Allocate memory Ringbuffer_t *pxNewRingbuffer = calloc(1, sizeof(Ringbuffer_t)); uint8_t *pucRingbufferStorage = malloc(xBufferSize); if (pxNewRingbuffer == NULL || pucRingbufferStorage == NULL) { @@ -966,7 +995,16 @@ err: RingbufHandle_t xRingbufferCreateNoSplit(size_t xItemSize, size_t xItemNum) { - return xRingbufferCreate((rbALIGN_SIZE(xItemSize) + rbHEADER_SIZE) * xItemNum, RINGBUF_TYPE_NOSPLIT); + //Guard the (aligned item size + header) * item count computation against overflow + size_t xItemSizeWithHeader; + size_t xBufferSize; + if (xItemNum == 0 || xItemSize > SIZE_MAX - rbALIGN_MASK || + __builtin_add_overflow(rbALIGN_SIZE(xItemSize), rbHEADER_SIZE, &xItemSizeWithHeader) || + __builtin_mul_overflow(xItemSizeWithHeader, xItemNum, &xBufferSize)) { + return NULL; + } + + return xRingbufferCreate(xBufferSize, RINGBUF_TYPE_NOSPLIT); } RingbufHandle_t xRingbufferCreateStatic(size_t xBufferSize, @@ -978,9 +1016,15 @@ RingbufHandle_t xRingbufferCreateStatic(size_t xBufferSize, configASSERT(xBufferSize > 0); configASSERT(xBufferType < RINGBUF_TYPE_MAX); configASSERT(pucRingbufferStorage != NULL && pxStaticRingbuffer != NULL); + if (xBufferType >= RINGBUF_TYPE_MAX || xBufferSize == 0) { + return NULL; + } if (xBufferType != RINGBUF_TYPE_BYTEBUF) { - //No-split/allow-split buffer sizes must be 32-bit aligned + //No-split/allow-split buffer sizes must be 32-bit aligned and large enough to avoid underflowing xMaxItemSize configASSERT(rbCHECK_ALIGNED(xBufferSize)); + if (!rbCHECK_ALIGNED(xBufferSize) || xBufferSize < rbHEADER_SIZE * 2) { + return NULL; + } } Ringbuffer_t *pxNewRingbuffer = (Ringbuffer_t *)pxStaticRingbuffer; @@ -1464,11 +1508,11 @@ RingbufHandle_t xRingbufferCreateWithCaps(size_t xBufferSize, RingbufferType_t x StaticRingbuffer_t *pxStaticRingbuffer; uint8_t *pucRingbufferStorage; - //Allocate memory - if (xBufferType != RINGBUF_TYPE_BYTEBUF) { - xBufferSize = rbALIGN_SIZE(xBufferSize); //xBufferSize is rounded up for no-split/allow-split buffers + if (prvGetAlignedBufferSize(xBufferSize, xBufferType, &xBufferSize) != pdTRUE) { + return NULL; } + //Allocate memory pxStaticRingbuffer = heap_caps_malloc(sizeof(StaticRingbuffer_t), (uint32_t)uxMemoryCaps); pucRingbufferStorage = heap_caps_malloc(xBufferSize, (uint32_t)uxMemoryCaps); diff --git a/components/esp_ringbuf/test_apps/main/test_ringbuf_common.c b/components/esp_ringbuf/test_apps/main/test_ringbuf_common.c index 028671dbe09..0c982c91e70 100644 --- a/components/esp_ringbuf/test_apps/main/test_ringbuf_common.c +++ b/components/esp_ringbuf/test_apps/main/test_ringbuf_common.c @@ -10,6 +10,7 @@ */ #include "sdkconfig.h" +#include #include #include #include @@ -19,6 +20,7 @@ #include "freertos/semphr.h" #include "freertos/ringbuf.h" #include "unity.h" +#include "esp_heap_caps.h" #include "esp_rom_sys.h" #include "test_functions.h" @@ -176,6 +178,85 @@ void receive_check_and_return_item_byte_buffer(RingbufHandle_t handle, const uin } } +static void check_too_small_ringbuffer_send_rejected(RingbufferType_t buffer_type) +{ + const size_t too_small_size = ITEM_HDR_SIZE; + + TEST_ASSERT_TRUE(heap_caps_check_integrity_all(true)); + + RingbufHandle_t handle = xRingbufferCreate(too_small_size, buffer_type); + BaseType_t ret = pdFALSE; + if (handle != NULL) { + if (buffer_type == RINGBUF_TYPE_NOSPLIT) { + void *item = NULL; + ret = xRingbufferSendAcquire(handle, &item, SMALL_ITEM_SIZE, 0); + if (ret == pdTRUE) { + memcpy(item, small_item, SMALL_ITEM_SIZE); + } + } else { + ret = xRingbufferSend(handle, small_item, SMALL_ITEM_SIZE, 0); + } + } + + TEST_ASSERT_TRUE(heap_caps_check_integrity_all(true)); + TEST_ASSERT_EQUAL(pdFALSE, ret); + + if (handle != NULL && ret == pdFALSE) { + vRingbufferDelete(handle); + } +} + +static void check_too_small_static_ringbuffer_rejected(RingbufferType_t buffer_type) +{ + static StaticRingbuffer_t ringbuffer_struct; + //Aligned so the no-split/allow-split alignment assert is satisfied and only the size guard rejects + static uint8_t ringbuffer_storage[ITEM_HDR_SIZE] __attribute__((aligned(sizeof(size_t)))); + + TEST_ASSERT_TRUE(heap_caps_check_integrity_all(true)); + + RingbufHandle_t handle = xRingbufferCreateStatic(sizeof(ringbuffer_storage), buffer_type, + ringbuffer_storage, &ringbuffer_struct); + + TEST_ASSERT_TRUE(heap_caps_check_integrity_all(true)); + TEST_ASSERT_NULL(handle); +} + +static void check_too_small_ringbuffer_with_caps_rejected(RingbufferType_t buffer_type) +{ + const size_t too_small_size = ITEM_HDR_SIZE; + + TEST_ASSERT_TRUE(heap_caps_check_integrity_all(true)); + + //The size guard rejects before any allocation, so the specific caps are irrelevant here + RingbufHandle_t handle = xRingbufferCreateWithCaps(too_small_size, buffer_type, MALLOC_CAP_8BIT); + + TEST_ASSERT_TRUE(heap_caps_check_integrity_all(true)); + TEST_ASSERT_NULL(handle); +} + +TEST_CASE("Ringbuffer prevents heap corruption when size can underflow max item size", "[esp_ringbuf]") +{ + check_too_small_ringbuffer_send_rejected(RINGBUF_TYPE_NOSPLIT); + check_too_small_ringbuffer_send_rejected(RINGBUF_TYPE_ALLOWSPLIT); + + check_too_small_static_ringbuffer_rejected(RINGBUF_TYPE_NOSPLIT); + check_too_small_static_ringbuffer_rejected(RINGBUF_TYPE_ALLOWSPLIT); + + check_too_small_ringbuffer_with_caps_rejected(RINGBUF_TYPE_NOSPLIT); + check_too_small_ringbuffer_with_caps_rejected(RINGBUF_TYPE_ALLOWSPLIT); + + RingbufHandle_t byte_buffer = xRingbufferCreate(ITEM_HDR_SIZE, RINGBUF_TYPE_BYTEBUF); + TEST_ASSERT_NOT_NULL(byte_buffer); + vRingbufferDelete(byte_buffer); +} + +TEST_CASE("Ringbuffer fixed-size no-split creation rejects size overflow", "[esp_ringbuf][linux]") +{ + const size_t overflowing_item_count = (SIZE_MAX / SMALL_ITEM_SIZE) + 2; + + TEST_ASSERT_NULL(xRingbufferCreateNoSplit(SMALL_ITEM_SIZE, overflowing_item_count)); +} + /* ----------------- Basic ring buffer behavior tests cases -------------------- * The following set of test cases will test basic send, receive, wrap around and buffer full * behavior of each type of ring buffer.