diff --git a/portable/MSVC-MingW/port.c b/portable/MSVC-MingW/port.c index 40458682b80..2c62986ae1f 100644 --- a/portable/MSVC-MingW/port.c +++ b/portable/MSVC-MingW/port.c @@ -637,7 +637,11 @@ void vPortGenerateSimulatedInterrupt( uint32_t ulInterruptNumber ) void vPortGenerateSimulatedInterruptFromWindowsThread( uint32_t ulInterruptNumber ) { - if( xPortRunning == pdTRUE ) + /* Reject out-of-range interrupt numbers before the shift below. Mirrors the + * bounds check the task-context sibling vPortGenerateSimulatedInterrupt already + * performs: ( 1UL << ulInterruptNumber ) is undefined when ulInterruptNumber is + * >= portMAX_INTERRUPTS (the width of ulPendingInterrupts). */ + if( ( xPortRunning == pdTRUE ) && ( ulInterruptNumber < portMAX_INTERRUPTS ) ) { /* Can't proceed if in a critical section as pvInterruptEventMutex won't * be available. */ diff --git a/portable/MemMang/heap_1.c b/portable/MemMang/heap_1.c index f697c907ca9..6e3b6e47bf1 100644 --- a/portable/MemMang/heap_1.c +++ b/portable/MemMang/heap_1.c @@ -53,6 +53,17 @@ /* A few bytes might be lost to byte aligning the heap start address. */ #define configADJUSTED_HEAP_SIZE ( configTOTAL_HEAP_SIZE - portBYTE_ALIGNMENT ) +/* configTOTAL_HEAP_SIZE must be larger than portBYTE_ALIGNMENT, otherwise the + * configADJUSTED_HEAP_SIZE subtraction above underflows. The "enough room left" + * check in pvPortMalloc() compares an unsigned size_t index against + * configADJUSTED_HEAP_SIZE, so an underflowed (very large) value silently + * defeats that check and pvPortMalloc() can return a pointer outside the ucHeap + * array. Reject such a nonsensical, too-small heap at compile time. This is a + * compiler (not preprocessor) check so that it still works when + * configTOTAL_HEAP_SIZE is defined with a cast, e.g. ( ( size_t ) 0x2000 ). */ +typedef char heapCHECK_configTOTAL_HEAP_SIZE_EXCEEDS_portBYTE_ALIGNMENT + [ ( ( configTOTAL_HEAP_SIZE ) > ( portBYTE_ALIGNMENT ) ) ? 1 : -1 ]; + /* Max value that fits in a size_t type. */ #define heapSIZE_MAX ( ~( ( size_t ) 0 ) ) diff --git a/stream_buffer.c b/stream_buffer.c index a4c6d268d11..087838c5ec8 100644 --- a/stream_buffer.c +++ b/stream_buffer.c @@ -820,10 +820,12 @@ size_t xStreamBufferSend( StreamBufferHandle_t xStreamBuffer, TickType_t xTicksToWait ) { StreamBuffer_t * const pxStreamBuffer = xStreamBuffer; - size_t xReturn, xSpace = 0; + size_t xReturn = 0; + size_t xSpace = 0; size_t xRequiredSpace = xDataLengthBytes; TimeOut_t xTimeOut; size_t xMaxReportedSpace = 0; + BaseType_t xSendAllowed = pdTRUE; traceENTER_xStreamBufferSend( xStreamBuffer, pvTxData, xDataLengthBytes, xTicksToWait ); @@ -845,9 +847,22 @@ size_t xStreamBufferSend( StreamBufferHandle_t xStreamBuffer, /* Overflow? */ configASSERT( xRequiredSpace > xDataLengthBytes ); + /* Enforce the overflow check at runtime as well, so that a message + * length whose required-space addition wraps size_t is rejected even + * when configASSERT() is compiled out. Without this the wrapped + * (smaller) xRequiredSpace would pass the space checks below and the + * send would proceed to copy xDataLengthBytes bytes, causing an + * out-of-bounds write. Reject the send instead: leave xReturn 0, skip + * blocking and the write, and exit through the single return below. */ + if( xRequiredSpace <= xDataLengthBytes ) + { + xSendAllowed = pdFALSE; + xTicksToWait = ( TickType_t ) 0; + } + /* If this is a message buffer then it must be possible to write the * whole message. */ - if( xRequiredSpace > xMaxReportedSpace ) + else if( xRequiredSpace > xMaxReportedSpace ) { /* The message would not fit even if the entire buffer was empty, * so don't wait for space. */ @@ -921,7 +936,10 @@ size_t xStreamBufferSend( StreamBufferHandle_t xStreamBuffer, mtCOVERAGE_TEST_MARKER(); } - xReturn = prvWriteMessageToBuffer( pxStreamBuffer, pvTxData, xDataLengthBytes, xSpace, xRequiredSpace ); + if( xSendAllowed == pdTRUE ) + { + xReturn = prvWriteMessageToBuffer( pxStreamBuffer, pvTxData, xDataLengthBytes, xSpace, xRequiredSpace ); + } if( xReturn > ( size_t ) 0 ) { @@ -955,8 +973,10 @@ size_t xStreamBufferSendFromISR( StreamBufferHandle_t xStreamBuffer, BaseType_t * const pxHigherPriorityTaskWoken ) { StreamBuffer_t * const pxStreamBuffer = xStreamBuffer; - size_t xReturn, xSpace; + size_t xReturn = 0; + size_t xSpace; size_t xRequiredSpace = xDataLengthBytes; + BaseType_t xSendAllowed = pdTRUE; traceENTER_xStreamBufferSendFromISR( xStreamBuffer, pvTxData, xDataLengthBytes, pxHigherPriorityTaskWoken ); @@ -973,14 +993,31 @@ size_t xStreamBufferSendFromISR( StreamBufferHandle_t xStreamBuffer, /* Overflow? */ configASSERT( xRequiredSpace > xDataLengthBytes ); + + /* Enforce the overflow check at runtime as well (see xStreamBufferSend), + * so a wrapping message length is rejected when configASSERT() is + * compiled out rather than proceeding to an out-of-bounds write. Reject + * the send by leaving xReturn 0 and skipping the write below, so the + * function still exits through its single return. */ + if( xRequiredSpace <= xDataLengthBytes ) + { + xSendAllowed = pdFALSE; + } + else + { + mtCOVERAGE_TEST_MARKER(); + } } else { mtCOVERAGE_TEST_MARKER(); } - xSpace = xStreamBufferSpacesAvailable( pxStreamBuffer ); - xReturn = prvWriteMessageToBuffer( pxStreamBuffer, pvTxData, xDataLengthBytes, xSpace, xRequiredSpace ); + if( xSendAllowed == pdTRUE ) + { + xSpace = xStreamBufferSpacesAvailable( pxStreamBuffer ); + xReturn = prvWriteMessageToBuffer( pxStreamBuffer, pvTxData, xDataLengthBytes, xSpace, xRequiredSpace ); + } if( xReturn > ( size_t ) 0 ) { @@ -1028,16 +1065,21 @@ static size_t prvWriteMessageToBuffer( StreamBuffer_t * const pxStreamBuffer, /* Ensure the data length given fits within configMESSAGE_BUFFER_LENGTH_TYPE. */ configASSERT( ( size_t ) xMessageLength == xDataLengthBytes ); - if( xSpace >= xRequiredSpace ) + if( ( ( size_t ) xMessageLength == xDataLengthBytes ) && ( xSpace >= xRequiredSpace ) ) { - /* There is enough space to write both the message length and the message + /* The message length fits within configMESSAGE_BUFFER_LENGTH_TYPE and + * there is enough space to write both the message length and the message * itself into the buffer. Start by writing the length of the data, the data * itself will be written later in this function. */ xNextHead = prvWriteBytesToBuffer( pxStreamBuffer, ( const uint8_t * ) &( xMessageLength ), sbBYTES_TO_STORE_MESSAGE_LENGTH, xNextHead ); } else { - /* Not enough space, so do not write data to the buffer. */ + /* Either there is not enough space, or the message length cannot be + * represented by configMESSAGE_BUFFER_LENGTH_TYPE without truncation. + * Writing a truncated length header would corrupt the message + * framing at the receiver, so do not write any data to the buffer. When asserts are + * disabled this runtime check is the only guard against truncation. */ xDataLengthBytes = 0; } } diff --git a/tasks.c b/tasks.c index 461271fcffb..8f69fff1951 100644 --- a/tasks.c +++ b/tasks.c @@ -1653,77 +1653,95 @@ STATIC void prvAddNewTaskToReadyList( TCB_t * pxNewTCB ) PRIVILEGED_FUNCTION; { TCB_t * pxNewTCB; - /* If the stack grows down then allocate the stack then the TCB so the stack - * does not grow into the TCB. Likewise if the stack grows up then allocate - * the TCB then the stack. */ - #if ( portSTACK_GROWTH > 0 ) - { - /* Allocate space for the TCB. Where the memory comes from depends on - * the implementation of the port malloc function and whether or not static - * allocation is being used. */ - /* MISRA Ref 11.5.1 [Malloc memory assignment] */ - /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-115 */ - /* coverity[misra_c_2012_rule_11_5_violation] */ - pxNewTCB = ( TCB_t * ) pvPortMalloc( sizeof( TCB_t ) ); - - if( pxNewTCB != NULL ) + /* Guard against the stack size calculation overflowing. The stack is + * allocated as ( ( size_t ) uxStackDepth ) * sizeof( StackType_t ). If a + * caller-supplied uxStackDepth is large enough that this product wraps + * size_t, the allocation is far smaller than requested while + * prvInitialiseNewTask() still computes the top of stack from the full + * uxStackDepth, causing out-of-bounds writes when the initial stack frame + * is written. The wrap is detected by multiplying and dividing back: + * if dividing the byte size by sizeof( StackType_t ) does not recover + * uxStackDepth then the multiplication overflowed. Leave pxNewTCB NULL + * and skip allocation instead of under-allocating, so the failure is + * reported through the single return at the end of the function. */ + if( ( ( size_t ) uxStackDepth ) != ( ( ( ( size_t ) uxStackDepth ) * sizeof( StackType_t ) ) / sizeof( StackType_t ) ) ) + { + pxNewTCB = NULL; + } + else + { + /* If the stack grows down then allocate the stack then the TCB so the stack + * does not grow into the TCB. Likewise if the stack grows up then allocate + * the TCB then the stack. */ + #if ( portSTACK_GROWTH > 0 ) { - ( void ) memset( ( void * ) pxNewTCB, 0x00, sizeof( TCB_t ) ); - - /* Allocate space for the stack used by the task being created. - * The base of the stack memory stored in the TCB so the task can - * be deleted later if required. */ + /* Allocate space for the TCB. Where the memory comes from depends on + * the implementation of the port malloc function and whether or not static + * allocation is being used. */ /* MISRA Ref 11.5.1 [Malloc memory assignment] */ /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-115 */ /* coverity[misra_c_2012_rule_11_5_violation] */ - pxNewTCB->pxStack = ( StackType_t * ) pvPortMallocStack( ( ( ( size_t ) uxStackDepth ) * sizeof( StackType_t ) ) ); + pxNewTCB = ( TCB_t * ) pvPortMalloc( sizeof( TCB_t ) ); - if( pxNewTCB->pxStack == NULL ) + if( pxNewTCB != NULL ) { - /* Could not allocate the stack. Delete the allocated TCB. */ - vPortFree( pxNewTCB ); - pxNewTCB = NULL; - } - } - } - #else /* portSTACK_GROWTH */ - { - StackType_t * pxStack; + ( void ) memset( ( void * ) pxNewTCB, 0x00, sizeof( TCB_t ) ); - /* Allocate space for the stack used by the task being created. */ - /* MISRA Ref 11.5.1 [Malloc memory assignment] */ - /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-115 */ - /* coverity[misra_c_2012_rule_11_5_violation] */ - pxStack = ( StackType_t * ) pvPortMallocStack( ( ( ( size_t ) uxStackDepth ) * sizeof( StackType_t ) ) ); + /* Allocate space for the stack used by the task being created. + * The base of the stack memory stored in the TCB so the task can + * be deleted later if required. */ + /* MISRA Ref 11.5.1 [Malloc memory assignment] */ + /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-115 */ + /* coverity[misra_c_2012_rule_11_5_violation] */ + pxNewTCB->pxStack = ( StackType_t * ) pvPortMallocStack( ( ( ( size_t ) uxStackDepth ) * sizeof( StackType_t ) ) ); - if( pxStack != NULL ) + if( pxNewTCB->pxStack == NULL ) + { + /* Could not allocate the stack. Delete the allocated TCB. */ + vPortFree( pxNewTCB ); + pxNewTCB = NULL; + } + } + } + #else /* portSTACK_GROWTH */ { - /* Allocate space for the TCB. */ + StackType_t * pxStack; + + /* Allocate space for the stack used by the task being created. */ /* MISRA Ref 11.5.1 [Malloc memory assignment] */ /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-115 */ /* coverity[misra_c_2012_rule_11_5_violation] */ - pxNewTCB = ( TCB_t * ) pvPortMalloc( sizeof( TCB_t ) ); + pxStack = ( StackType_t * ) pvPortMallocStack( ( ( ( size_t ) uxStackDepth ) * sizeof( StackType_t ) ) ); - if( pxNewTCB != NULL ) + if( pxStack != NULL ) { - ( void ) memset( ( void * ) pxNewTCB, 0x00, sizeof( TCB_t ) ); + /* Allocate space for the TCB. */ + /* MISRA Ref 11.5.1 [Malloc memory assignment] */ + /* More details at: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/MISRA.md#rule-115 */ + /* coverity[misra_c_2012_rule_11_5_violation] */ + pxNewTCB = ( TCB_t * ) pvPortMalloc( sizeof( TCB_t ) ); - /* Store the stack location in the TCB. */ - pxNewTCB->pxStack = pxStack; + if( pxNewTCB != NULL ) + { + ( void ) memset( ( void * ) pxNewTCB, 0x00, sizeof( TCB_t ) ); + + /* Store the stack location in the TCB. */ + pxNewTCB->pxStack = pxStack; + } + else + { + /* The stack cannot be used as the TCB was not created. Free + * it again. */ + vPortFreeStack( pxStack ); + } } else { - /* The stack cannot be used as the TCB was not created. Free - * it again. */ - vPortFreeStack( pxStack ); + pxNewTCB = NULL; } } - else - { - pxNewTCB = NULL; - } + #endif /* portSTACK_GROWTH */ } - #endif /* portSTACK_GROWTH */ if( pxNewTCB != NULL ) {