Skip to content

Commit

Permalink
Add integer overflow checks to buffer allocation APIs (FreeRTOS#1017)
Browse files Browse the repository at this point in the history 
* Add checks to verify integer overflows doesnt occur during buffer allocations

* Uncrustify: triggered by comment

* updating review feedback

---------

Co-authored-by: Soren Ptak <ptaksoren@gmail.com>
Co-authored-by: GitHub Action <action@github.com>
  • Loading branch information
@tony-josi-aws @Skptak @actions-user
3 people authored Sep 6, 2023
1 parent eed294c commit f590724
Show file tree
Hide file tree
Showing 2 changed files with 169 additions and 60 deletions.
165 changes: 113 additions & 52 deletions source/portable/BufferManagement/BufferAllocation_2.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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 )
{
Expand All @@ -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;
Expand Down Expand Up @@ -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 )
Expand All @@ -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 )
{
Expand Down Expand Up @@ -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;
Expand Down
64 changes: 56 additions & 8 deletions source/portable/NetworkInterface/pic32mzef/BufferAllocation_2.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 ) )
{
Expand All @@ -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();
Expand Down Expand Up @@ -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 )
Expand Down Expand Up @@ -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 )
{
Expand Down

0 comments on commit f590724

Please sign in to comment.