Kernel defense-in-depth hardening (heap_1, tasks, event_groups, stream_buffer, MSVC-MingW port)

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. */

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

stream_buffer.c

@@ -820,10 +820,12 @@
                           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 @@
         /* 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 @@
         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 @@
                                  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 @@
 
         /* 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 @@
         /* 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;
         }
     }

tasks.c

@@ -1653,77 +1653,95 @@
     {
         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 )
         {