Skip to content

Add integer overflow checks to buffer allocation APIs #1017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 6, 2023
Merged

Add integer overflow checks to buffer allocation APIs #1017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
038c18c
Add checks to verify integer overflows doesnt occur during buffer all…
tony-josi-aws Jun 23, 2023
e540aa7
Merge branch 'main' into test_buff_alloc
tony-josi-aws Sep 4, 2023
ddd10f2
Merge branch 'main' into test_buff_alloc
Skptak Sep 6, 2023
6ae36b2
Uncrustify: triggered by comment
actions-user Sep 6, 2023
b2b63b7
updating review feedback
tony-josi-aws Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 113 additions & 52 deletions source/portable/BufferManagement/BufferAllocation_2.c
View file
Open in desktop
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 );
Copy link
Contributor

@htibosch htibosch Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tony-josi-aws,

pucGetNetworkBuffer() allocates a buffer for pucEthernetBuffer
pxGetNetworkBufferWithDescriptor() also allocates a buffer for pucEthernetBuffer

Both do length checks and add the necessary bytes.

Now I wonder why do everything double and slightly different?

Why doesn't pxGetNetworkBufferWithDescriptor() call pucGetNetworkBuffer() to obtain a buffer?

Copy link
Member Author

@tony-josi-aws tony-josi-aws Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pucGetNetworkBuffer doesnt need to do the +2 as its only called for resizing existing buffers.


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
Open in desktop
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