From f590724b4793b0f072c12aa6eb1b678e95254be0 Mon Sep 17 00:00:00 2001 From: Tony Josi Date: Wed, 6 Sep 2023 16:52:00 +0530 Subject: [PATCH 1/2] Add integer overflow checks to buffer allocation APIs (#1017) * Add checks to verify integer overflows doesnt occur during buffer allocations * Uncrustify: triggered by comment * updating review feedback --------- Co-authored-by: Soren Ptak Co-authored-by: GitHub Action --- .../BufferManagement/BufferAllocation_2.c | 165 ++++++++++++------ .../pic32mzef/BufferAllocation_2.c | 64 ++++++- 2 files changed, 169 insertions(+), 60 deletions(-) diff --git a/source/portable/BufferManagement/BufferAllocation_2.c b/source/portable/BufferManagement/BufferAllocation_2.c index 4a7e2476e0..95ae9fb695 100644 --- a/source/portable/BufferManagement/BufferAllocation_2.c +++ b/source/portable/BufferManagement/BufferAllocation_2.c @@ -63,6 +63,10 @@ #define baMINIMAL_BUFFER_SIZE sizeof( ARPPacket_t ) #endif /* ipconfigUSE_TCP == 1 */ +#define baALIGNMENT_BYTES ( sizeof( size_t ) ) +#define baALIGNMENT_MASK ( baALIGNMENT_BYTES - 1U ) +#define baADD_WILL_OVERFLOW( a, b ) ( ( a ) > ( SIZE_MAX - ( b ) ) ) + /* Compile time assertion with zero runtime effects * it will assert on 'e' not being zero, as it tries to divide by it, * will also print the line where the error occurred in case of failure */ @@ -175,8 +179,11 @@ BaseType_t xNetworkBuffersInitialise( void ) uint8_t * pucGetNetworkBuffer( size_t * pxRequestedSizeBytes ) { - uint8_t * pucEthernetBuffer; + uint8_t * pucEthernetBuffer = NULL; + size_t uxMaxAllowedBytes = ( SIZE_MAX >> 1 ); size_t xSize = *pxRequestedSizeBytes; + size_t xBytesRequiredForAlignment, xAllocatedBytes; + BaseType_t xIntegerOverflowed = pdFALSE; if( xSize < baMINIMAL_BUFFER_SIZE ) { @@ -187,25 +194,46 @@ uint8_t * pucGetNetworkBuffer( size_t * pxRequestedSizeBytes ) /* Round up xSize to the nearest multiple of N bytes, * where N equals 'sizeof( size_t )'. */ - if( ( xSize & ( sizeof( size_t ) - 1U ) ) != 0U ) + if( ( xSize & baALIGNMENT_MASK ) != 0U ) { - xSize = ( xSize | ( sizeof( size_t ) - 1U ) ) + 1U; - } + xBytesRequiredForAlignment = baALIGNMENT_BYTES - ( xSize & baALIGNMENT_MASK ); - *pxRequestedSizeBytes = xSize; + if( baADD_WILL_OVERFLOW( xSize, xBytesRequiredForAlignment ) == 0 ) + { + xSize += xBytesRequiredForAlignment; + } + else + { + xIntegerOverflowed = pdTRUE; + } + } - /* Allocate a buffer large enough to store the requested Ethernet frame size - * and a pointer to a network buffer structure (hence the addition of - * ipBUFFER_PADDING bytes). */ - pucEthernetBuffer = ( uint8_t * ) pvPortMalloc( xSize + ipBUFFER_PADDING ); - configASSERT( pucEthernetBuffer != NULL ); + if( baADD_WILL_OVERFLOW( xSize, ipBUFFER_PADDING ) == 0 ) + { + xAllocatedBytes = xSize + ipBUFFER_PADDING; + } + else + { + xIntegerOverflowed = pdTRUE; + } - if( pucEthernetBuffer != NULL ) + if( ( xIntegerOverflowed == pdFALSE ) && ( xAllocatedBytes <= uxMaxAllowedBytes ) ) { - /* Enough space is left at the start of the buffer to place a pointer to - * the network buffer structure that references this Ethernet buffer. - * Return a pointer to the start of the Ethernet buffer itself. */ - pucEthernetBuffer += ipBUFFER_PADDING; + *pxRequestedSizeBytes = xSize; + + /* Allocate a buffer large enough to store the requested Ethernet frame size + * and a pointer to a network buffer structure (hence the addition of + * ipBUFFER_PADDING bytes). */ + pucEthernetBuffer = ( uint8_t * ) pvPortMalloc( xAllocatedBytes ); + configASSERT( pucEthernetBuffer != NULL ); + + if( pucEthernetBuffer != NULL ) + { + /* Enough space is left at the start of the buffer to place a pointer to + * the network buffer structure that references this Ethernet buffer. + * Return a pointer to the start of the Ethernet buffer itself. */ + pucEthernetBuffer += ipBUFFER_PADDING; + } } return pucEthernetBuffer; @@ -234,8 +262,51 @@ NetworkBufferDescriptor_t * pxGetNetworkBufferWithDescriptor( size_t xRequestedS size_t uxCount; size_t uxMaxAllowedBytes = ( SIZE_MAX >> 1 ); size_t xRequestedSizeBytesCopy = xRequestedSizeBytes; + size_t xBytesRequiredForAlignment, xAllocatedBytes; + BaseType_t xIntegerOverflowed = pdFALSE; + + if( ( xRequestedSizeBytesCopy < ( size_t ) baMINIMAL_BUFFER_SIZE ) ) + { + /* ARP packets can replace application packets, so the storage must be + * at least large enough to hold an ARP. */ + xRequestedSizeBytesCopy = baMINIMAL_BUFFER_SIZE; + } + + /* Add 2 bytes to xRequestedSizeBytesCopy and round up xRequestedSizeBytesCopy + * to the nearest multiple of N bytes, where N equals 'sizeof( size_t )'. */ + if( baADD_WILL_OVERFLOW( xRequestedSizeBytesCopy, 2 ) == 0 ) + { + xRequestedSizeBytesCopy += 2U; + } + else + { + xIntegerOverflowed = pdTRUE; + } + + if( ( xRequestedSizeBytesCopy & baALIGNMENT_MASK ) != 0U ) + { + xBytesRequiredForAlignment = baALIGNMENT_BYTES - ( xRequestedSizeBytesCopy & baALIGNMENT_MASK ); + + if( baADD_WILL_OVERFLOW( xRequestedSizeBytesCopy, xBytesRequiredForAlignment ) == 0 ) + { + xRequestedSizeBytesCopy += xBytesRequiredForAlignment; + } + else + { + xIntegerOverflowed = pdTRUE; + } + } + + if( baADD_WILL_OVERFLOW( xRequestedSizeBytesCopy, ipBUFFER_PADDING ) == 0 ) + { + xAllocatedBytes = xRequestedSizeBytesCopy + ipBUFFER_PADDING; + } + else + { + xIntegerOverflowed = pdTRUE; + } - if( ( xRequestedSizeBytesCopy <= uxMaxAllowedBytes ) && ( xNetworkBufferSemaphore != NULL ) ) + if( ( xIntegerOverflowed == pdFALSE ) && ( xAllocatedBytes <= uxMaxAllowedBytes ) && ( xNetworkBufferSemaphore != NULL ) ) { /* If there is a semaphore available, there is a network buffer available. */ if( xSemaphoreTake( xNetworkBufferSemaphore, xBlockTimeTicks ) == pdPASS ) @@ -261,25 +332,9 @@ NetworkBufferDescriptor_t * pxGetNetworkBufferWithDescriptor( size_t xRequestedS if( xRequestedSizeBytesCopy > 0U ) { - if( ( xRequestedSizeBytesCopy < ( size_t ) baMINIMAL_BUFFER_SIZE ) ) - { - /* ARP packets can replace application packets, so the storage must be - * at least large enough to hold an ARP. */ - xRequestedSizeBytesCopy = baMINIMAL_BUFFER_SIZE; - } - - /* Add 2 bytes to xRequestedSizeBytesCopy and round up xRequestedSizeBytesCopy - * to the nearest multiple of N bytes, where N equals 'sizeof( size_t )'. */ - xRequestedSizeBytesCopy += 2U; - - if( ( xRequestedSizeBytesCopy & ( sizeof( size_t ) - 1U ) ) != 0U ) - { - xRequestedSizeBytesCopy = ( xRequestedSizeBytesCopy | ( sizeof( size_t ) - 1U ) ) + 1U; - } - /* Extra space is obtained so a pointer to the network buffer can * be stored at the beginning of the buffer. */ - pxReturn->pucEthernetBuffer = ( uint8_t * ) pvPortMalloc( xRequestedSizeBytesCopy + ipBUFFER_PADDING ); + pxReturn->pucEthernetBuffer = ( uint8_t * ) pvPortMalloc( xAllocatedBytes ); if( pxReturn->pucEthernetBuffer == NULL ) { @@ -403,32 +458,38 @@ NetworkBufferDescriptor_t * pxResizeNetworkBufferWithDescriptor( NetworkBufferDe size_t uxSizeBytes = xNewSizeBytes; NetworkBufferDescriptor_t * pxNetworkBufferCopy = pxNetworkBuffer; - - xOriginalLength = pxNetworkBufferCopy->xDataLength + ipBUFFER_PADDING; - uxSizeBytes = uxSizeBytes + ipBUFFER_PADDING; - - pucBuffer = pucGetNetworkBuffer( &( uxSizeBytes ) ); - if( pucBuffer == NULL ) - { - /* In case the allocation fails, return NULL. */ - pxNetworkBufferCopy = NULL; - } - else + if( baADD_WILL_OVERFLOW( uxSizeBytes, ipBUFFER_PADDING ) == 0 ) { - pxNetworkBufferCopy->xDataLength = uxSizeBytes; + uxSizeBytes = uxSizeBytes + ipBUFFER_PADDING; - if( uxSizeBytes > xOriginalLength ) + pucBuffer = pucGetNetworkBuffer( &( uxSizeBytes ) ); + + if( pucBuffer == NULL ) { - uxSizeBytes = xOriginalLength; + /* In case the allocation fails, return NULL. */ + pxNetworkBufferCopy = NULL; } + else + { + pxNetworkBufferCopy->xDataLength = uxSizeBytes; - ( void ) memcpy( pucBuffer - ipBUFFER_PADDING, - pxNetworkBufferCopy->pucEthernetBuffer - ipBUFFER_PADDING, - uxSizeBytes ); - vReleaseNetworkBuffer( pxNetworkBufferCopy->pucEthernetBuffer ); - pxNetworkBufferCopy->pucEthernetBuffer = pucBuffer; + if( uxSizeBytes > xOriginalLength ) + { + uxSizeBytes = xOriginalLength; + } + + ( void ) memcpy( pucBuffer - ipBUFFER_PADDING, + pxNetworkBufferCopy->pucEthernetBuffer - ipBUFFER_PADDING, + uxSizeBytes ); + vReleaseNetworkBuffer( pxNetworkBufferCopy->pucEthernetBuffer ); + pxNetworkBufferCopy->pucEthernetBuffer = pucBuffer; + } + } + else + { + pxNetworkBufferCopy = NULL; } return pxNetworkBufferCopy; diff --git a/source/portable/NetworkInterface/pic32mzef/BufferAllocation_2.c b/source/portable/NetworkInterface/pic32mzef/BufferAllocation_2.c index 266b3ade04..ad185edeff 100644 --- a/source/portable/NetworkInterface/pic32mzef/BufferAllocation_2.c +++ b/source/portable/NetworkInterface/pic32mzef/BufferAllocation_2.c @@ -75,6 +75,10 @@ STATIC_ASSERT( ipconfigETHERNET_MINIMUM_PACKET_BYTES <= baMINIMAL_BUFFER_SIZE ); #endif +#define baALIGNMENT_BYTES ( sizeof( size_t ) ) +#define baALIGNMENT_MASK ( baALIGNMENT_BYTES - 1U ) +#define baADD_WILL_OVERFLOW( a, b ) ( ( a ) > ( SIZE_MAX - ( b ) ) ) + /* A list of free (available) NetworkBufferDescriptor_t structures. */ static List_t xFreeBuffersList; @@ -385,6 +389,8 @@ NetworkBufferDescriptor_t * pxGetNetworkBufferWithDescriptor( size_t xRequestedS { NetworkBufferDescriptor_t * pxReturn = NULL; size_t uxCount; + size_t xBytesRequiredForAlignment, xAllocatedBytes; + BaseType_t xIntegerOverflowed = pdFALSE; if( ( xRequestedSizeBytes != 0u ) && ( xRequestedSizeBytes < ( size_t ) baMINIMAL_BUFFER_SIZE ) ) { @@ -397,19 +403,49 @@ NetworkBufferDescriptor_t * pxGetNetworkBufferWithDescriptor( size_t xRequestedS if( xRequestedSizeBytes != 0u ) { #endif /* #ifdef PIC32_USE_ETHERNET */ - xRequestedSizeBytes += 2u; - if( ( xRequestedSizeBytes & ( sizeof( size_t ) - 1u ) ) != 0u ) + if( baADD_WILL_OVERFLOW( xRequestedSizeBytes, 2 ) == 0 ) + { + xRequestedSizeBytes += 2U; + } + else { - xRequestedSizeBytes = ( xRequestedSizeBytes | ( sizeof( size_t ) - 1u ) ) + 1u; + xIntegerOverflowed = pdTRUE; + } + + if( ( xRequestedSizeBytes & baALIGNMENT_MASK ) != 0U ) + { + xBytesRequiredForAlignment = baALIGNMENT_BYTES - ( xRequestedSizeBytes & baALIGNMENT_MASK ); + + if( baADD_WILL_OVERFLOW( xRequestedSizeBytes, xBytesRequiredForAlignment ) == 0 ) + { + xRequestedSizeBytes += xBytesRequiredForAlignment; + } + else + { + xIntegerOverflowed = pdTRUE; + } } + #ifdef PIC32_USE_ETHERNET + ( void ) xAllocatedBytes; + #else + if( baADD_WILL_OVERFLOW( xRequestedSizeBytes, ipBUFFER_PADDING ) == 0 ) + { + xAllocatedBytes = xRequestedSizeBytes + ipBUFFER_PADDING; + } + else + { + xIntegerOverflowed = pdTRUE; + } + #endif /* #ifndef PIC32_USE_ETHERNET */ + #ifdef PIC32_USE_ETHERNET } #endif /* #ifdef PIC32_USE_ETHERNET */ /* If there is a semaphore available, there is a network buffer available. */ - if( xSemaphoreTake( xNetworkBufferSemaphore, xBlockTimeTicks ) == pdPASS ) + if( ( xIntegerOverflowed == pdFALSE ) && ( xSemaphoreTake( xNetworkBufferSemaphore, xBlockTimeTicks ) == pdPASS ) ) { /* Protect the structure as it is accessed from tasks and interrupts. */ taskENTER_CRITICAL(); @@ -438,7 +474,7 @@ NetworkBufferDescriptor_t * pxGetNetworkBufferWithDescriptor( size_t xRequestedS #ifdef PIC32_USE_ETHERNET pxReturn->pucEthernetBuffer = NetworkBufferAllocate( xRequestedSizeBytes - sizeof( TCPIP_MAC_ETHERNET_HEADER ) ); #else - pxReturn->pucEthernetBuffer = ( uint8_t * ) pvPortMalloc( xRequestedSizeBytes + ipBUFFER_PADDING ); + pxReturn->pucEthernetBuffer = ( uint8_t * ) pvPortMalloc( xAllocatedBytes ); #endif /* #ifdef PIC32_USE_ETHERNET */ if( pxReturn->pucEthernetBuffer == NULL ) @@ -554,16 +590,28 @@ NetworkBufferDescriptor_t * pxResizeNetworkBufferWithDescriptor( NetworkBufferDe size_t xNewSizeBytes ) { size_t xOriginalLength; - uint8_t * pucBuffer; + uint8_t * pucBuffer = NULL; + BaseType_t xIntegerOverflowed = pdFALSE; #ifdef PIC32_USE_ETHERNET xOriginalLength = pxNetworkBuffer->xDataLength; #else xOriginalLength = pxNetworkBuffer->xDataLength + ipBUFFER_PADDING; - xNewSizeBytes = xNewSizeBytes + ipBUFFER_PADDING; + + if( baADD_WILL_OVERFLOW( xNewSizeBytes, ipBUFFER_PADDING ) == 0 ) + { + xNewSizeBytes = xNewSizeBytes + ipBUFFER_PADDING; + } + else + { + xIntegerOverflowed = pdTRUE; + } #endif /* #ifdef PIC32_USE_ETHERNET */ - pucBuffer = pucGetNetworkBuffer( &( xNewSizeBytes ) ); + if( xIntegerOverflowed == pdFALSE ) + { + pucBuffer = pucGetNetworkBuffer( &( xNewSizeBytes ) ); + } if( pucBuffer == NULL ) { From a91c3116ba800649d83252bac85a7a566610ec0d Mon Sep 17 00:00:00 2001 From: Peter R Herrmann <114958111+Peter-Herrmann@users.noreply.github.com> Date: Wed, 6 Sep 2023 21:08:37 -0700 Subject: [PATCH 2/2] DriverSAM/NetworkInterface.c warning cleanup - purely refactoring (#1016) * warning cleanup - purely refactoring * Incorporated PR feedback - Made vCheckBuffersAndQueue() static - Added uxLowestSemCount back into prvEMACHandlerTask, now modifying global instead of shadowing * Uncrustify: triggered by comment. * Fix formatting * Uncrustify: triggered by comment. * Un-doing uncrustify commit that breaks formatting rules * formatting fix * formatting fix --------- Co-authored-by: Soren Ptak Co-authored-by: GitHub Action Co-authored-by: Rahul Kar --- .../DriverSAM/NetworkInterface.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/source/portable/NetworkInterface/DriverSAM/NetworkInterface.c b/source/portable/NetworkInterface/DriverSAM/NetworkInterface.c index a2b19d7343..7ad92f940b 100644 --- a/source/portable/NetworkInterface/DriverSAM/NetworkInterface.c +++ b/source/portable/NetworkInterface/DriverSAM/NetworkInterface.c @@ -204,14 +204,17 @@ static void hand_tx_errors( void ); static uint16_t prvGenerateCRC16( const uint8_t * pucAddress ); static void prvAddMulticastMACAddress( const uint8_t * ucMacAddress ); +/* Checks IP queue, buffers, and semaphore and logs diagnostic info if configured */ +static void vCheckBuffersAndQueue( void ); + +/* return 'puc_buffer' to the pool of transmission buffers. */ +void returnTxBuffer( uint8_t * puc_buffer ); + /*-----------------------------------------------------------*/ /* A copy of PHY register 1: 'PHY_REG_01_BMSR' */ static BaseType_t xGMACSwitchRequired; -/* LLMNR multicast address. */ -static const uint8_t llmnr_mac_address[] = { 0x01, 0x00, 0x5E, 0x00, 0x00, 0xFC }; - /* The GMAC object as defined by the ASF drivers. */ static gmac_device_t gs_gmac_dev; @@ -454,8 +457,6 @@ static BaseType_t xPHY_Write( BaseType_t xAddress, static BaseType_t prvSAM_NetworkInterfaceInitialise( NetworkInterface_t * pxInterface ) { - const TickType_t x5_Seconds = 5000UL; - if( xEMACTaskHandle == NULL ) { prvGMACInit( pxInterface ); @@ -673,9 +674,7 @@ static BaseType_t prvSAM_NetworkInterfaceOutput( NetworkInterface_t * pxInterfac static BaseType_t prvGMACInit( NetworkInterface_t * pxInterface ) { - uint32_t ncfgr; NetworkEndPoint_t * pxEndPoint; - BaseType_t xEntry = 1; gmac_options_t gmac_option; @@ -1065,7 +1064,7 @@ volatile UBaseType_t uxLastMinBufferCount = 0; volatile UBaseType_t uxCurrentSemCount; volatile UBaseType_t uxLowestSemCount; -void vCheckBuffersAndQueue( void ) +static void vCheckBuffersAndQueue( void ) { static UBaseType_t uxCurrentCount; @@ -1126,14 +1125,14 @@ void vNetworkInterfaceAllocateRAMToBuffers( NetworkBufferDescriptor_t pxNetworkB static void prvEMACHandlerTask( void * pvParameters ) { UBaseType_t uxCount; - UBaseType_t uxLowestSemCount = GMAC_TX_BUFFERS + 1; + + uxLowestSemCount = GMAC_TX_BUFFERS + 1; #if ( ipconfigZERO_COPY_TX_DRIVER != 0 ) NetworkBufferDescriptor_t * pxBuffer; #endif uint8_t * pucBuffer; BaseType_t xResult = 0; - uint32_t xStatus; const TickType_t ulMaxBlockTime = pdMS_TO_TICKS( EMAC_MAX_BLOCK_TIME_MS ); uint32_t ulISREvents = 0U;