Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

FreeRTOS+TCP: Let xDataLength always mean "total number of bytes" #1513

Merged
merged 4 commits into from
Nov 25, 2019
Merged

FreeRTOS+TCP: Let xDataLength always mean "total number of bytes" #1513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04e1c60
FreeRTOS+TCP: Let xDataLength always mean the same, total number of b…
Nov 8, 2019
3248a18
NetworkBuffer.xDataLength also corrected for the received UDP packets
Nov 9, 2019
0fa525c
Replaced 2 magic numbers with a sizeof statement
Nov 12, 2019
67e86d2
Changes comments about xDataLength in downstream UDP packets
Nov 12, 2019
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
10 changes: 5 additions & 5 deletions libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_ARP.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,11 +605,11 @@ void vARPGenerateRequestPacket( NetworkBufferDescriptor_t * const pxNetworkBuffe
{
ARPPacket_t *pxARPPacket;

/* Buffer allocation ensures that buffers always have space
for an ARP packet. See buffer allocation implementations 1
and 2 under portable/BufferManagement. */
configASSERT( pxNetworkBuffer );
configASSERT( pxNetworkBuffer->xDataLength >= sizeof(ARPPacket_t) );
/* Buffer allocation ensures that buffers always have space
for an ARP packet. See buffer allocation implementations 1
and 2 under portable/BufferManagement. */
configASSERT( pxNetworkBuffer );
configASSERT( pxNetworkBuffer->xDataLength >= sizeof(ARPPacket_t) );

pxARPPacket = ( ARPPacket_t * ) pxNetworkBuffer->pucEthernetBuffer;

Expand Down
24 changes: 10 additions & 14 deletions libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,15 +912,16 @@ for testing purposes, by the module iot_test_freertos_tcp.c
uint32_t ulDNSHandlePacket( NetworkBufferDescriptor_t *pxNetworkBuffer )
{
DNSMessage_t *pxDNSMessageHeader;
size_t uxPayloadSize = pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t );

if( pxNetworkBuffer->xDataLength >= sizeof( DNSMessage_t ) )
if( uxPayloadSize >= sizeof( DNSMessage_t ) )
{
pxDNSMessageHeader =
( DNSMessage_t * ) ( pxNetworkBuffer->pucEthernetBuffer + sizeof( UDPPacket_t ) );

/* The parameter pdFALSE indicates that the reply was not expected. */
prvParseDNSReply( ( uint8_t * ) pxDNSMessageHeader,
pxNetworkBuffer->xDataLength,
uxPayloadSize,
pdFALSE );
}

Expand All @@ -935,11 +936,12 @@ DNSMessage_t *pxDNSMessageHeader;
{
UDPPacket_t *pxUDPPacket = ( UDPPacket_t * ) pxNetworkBuffer->pucEthernetBuffer;
uint8_t *pucUDPPayloadBuffer = pxNetworkBuffer->pucEthernetBuffer + sizeof( UDPPacket_t );
size_t uxPayloadSize = pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t );

/* The network buffer data length has already been set to the
length of the UDP payload. */
prvTreatNBNS( pucUDPPayloadBuffer,
pxNetworkBuffer->xDataLength,
uxPayloadSize,
pxUDPPacket->xIPHeader.ulSourceIPAddress );

/* The packet was not consumed. */
Expand Down Expand Up @@ -1168,11 +1170,9 @@ BaseType_t xDoStore = xExpected;
{
BaseType_t xDataLength = uxBufferLength + sizeof( UDPHeader_t ) + sizeof( EthernetHeader_t ) + sizeof( IPHeader_t );

/* The field xDataLength was set to the length of the UDP payload.
The answer (reply) will be longer than the request, so the packet
must be duplicaed into a bigger buffer */
/* Set the size of the outgoing packet. */
pxNetworkBuffer->xDataLength = xDataLength;
pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, xDataLength + 16 );
pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, xDataLength + sizeof( LLMNRAnswer_t ) );

if( pxNewBuffer != NULL )
{
Expand Down Expand Up @@ -1326,14 +1326,10 @@ BaseType_t xDoStore = xExpected;
if( ( xBufferAllocFixedSize == pdFALSE ) && ( pxNetworkBuffer != NULL ) )
{
NetworkBufferDescriptor_t *pxNewBuffer;
BaseType_t xDataLength = pxNetworkBuffer->xDataLength + sizeof( UDPHeader_t ) +
sizeof( EthernetHeader_t ) + sizeof( IPHeader_t );

/* The field xDataLength was set to the length of the UDP payload.
The answer (reply) will be longer than the request, so the packet
must be duplicated into a bigger buffer */
pxNetworkBuffer->xDataLength = xDataLength;
pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, xDataLength + 16 );
/* The field xDataLength was set to the total length of the UDP packet,
i.e. the payload size plus sizeof( UDPPacket_t ). */
pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, pxNetworkBuffer->xDataLength + sizeof( NBNSAnswer_t ) );

if( pxNewBuffer != NULL )
{
Expand Down
30 changes: 16 additions & 14 deletions libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1122,7 +1122,8 @@ void FreeRTOS_SetAddressConfiguration( const uint32_t *pulIPAddress, const uint3
pxNetworkBuffer->pucEthernetBuffer[ ipSOCKET_OPTIONS_OFFSET ] = FREERTOS_SO_UDPCKSUM_OUT;
pxNetworkBuffer->ulIPAddress = ulIPAddress;
pxNetworkBuffer->usPort = ipPACKET_CONTAINS_ICMP_DATA;
pxNetworkBuffer->xDataLength = xNumberOfBytesToSend + sizeof( ICMPHeader_t );
/* xDataLength is the size of the total packet, including the Ethernet header. */
pxNetworkBuffer->xDataLength = xNumberOfBytesToSend + sizeof( ICMPPacket_t );

/* Send to the stack. */
xStackTxEvent.pvData = pxNetworkBuffer;
Expand Down Expand Up @@ -1611,23 +1612,24 @@ uint8_t ucProtocol;

/* Only proceed if the payload length indicated in the header
appears to be valid. */
if ( pxNetworkBuffer->xDataLength >= sizeof( UDPPacket_t ) )
if ( ( pxNetworkBuffer->xDataLength >= sizeof( UDPPacket_t ) ) && ( FreeRTOS_ntohs( pxUDPPacket->xUDPHeader.usLength ) >= sizeof( UDPHeader_t ) ) )
{
/* Ensure that downstream UDP packet handling has the lesser
* of: the actual network buffer Ethernet frame length, or
* the sender's UDP packet header payload length, minus the
* size of the UDP header.
*
* The size of the UDP packet structure in this implementation
* includes the size of the Ethernet header, the size of
* the IP header, and the size of the UDP header.
size_t uxPayloadSize_1, uxPayloadSize_2;
/* The UDP payload size can be calculated by subtracting the
* header size from `xDataLength`.
* However, the `xDataLength` may be longer that expected,
* e.g. when a small packet is padded with zero's.
* The UDP header contains a field `usLength` reflecting
* the payload size plus the UDP header ( 8 bytes ).
* Set `xDataLength` to the size of the headers,
* plus the lower of the two calculated payload sizes.
*/

pxNetworkBuffer->xDataLength -= sizeof( UDPPacket_t );
if( ( FreeRTOS_ntohs( pxUDPPacket->xUDPHeader.usLength ) - sizeof( UDPHeader_t ) ) <
pxNetworkBuffer->xDataLength )
uxPayloadSize_1 = pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t );
uxPayloadSize_2 = FreeRTOS_ntohs( pxUDPPacket->xUDPHeader.usLength ) - sizeof( UDPHeader_t );
dan4thewin marked this conversation as resolved.
Show resolved Hide resolved
if( uxPayloadSize_1 > uxPayloadSize_2 )
{
pxNetworkBuffer->xDataLength = FreeRTOS_ntohs( pxUDPPacket->xUDPHeader.usLength ) - sizeof( UDPHeader_t );
pxNetworkBuffer->xDataLength = uxPayloadSize_2 + sizeof( UDPPacket_t );
Copy link
Contributor

Choose a reason for hiding this comment

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

This line does not appear to be consistent with the big block comment above. Can you please confirm (i.e. especially since is probably an important block of code for mitigating buffer overflow risk, and it's not obvious to read)?

}

/* Fields in pxNetworkBuffer (usPort, ulIPAddress) are network order. */
Expand Down
11 changes: 7 additions & 4 deletions libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,9 +702,11 @@ EventBits_t xEventBits = ( EventBits_t ) 0;
}
taskEXIT_CRITICAL();

/* The returned value is the data length, which may have been capped to
the receive buffer size. */
lReturn = ( int32_t ) pxNetworkBuffer->xDataLength;
/* The returned value is the length of the payload data, which is
calculated at the total packet size minus the headers.
The validity of `xDataLength` prvProcessIPPacket has been confirmed
in 'prvProcessIPPacket()'. */
lReturn = ( int32_t ) ( pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t ) );

if( pxSourceAddress != NULL )
{
Expand Down Expand Up @@ -833,7 +835,8 @@ FreeRTOS_Socket_t *pxSocket;

if( pxNetworkBuffer != NULL )
{
pxNetworkBuffer->xDataLength = xTotalDataLength;
/* xDataLength is the size of the total packet, including the Ethernet header. */
pxNetworkBuffer->xDataLength = xTotalDataLength + sizeof( UDPPacket_t );
pxNetworkBuffer->usPort = pxDestinationAddress->sin_port;
pxNetworkBuffer->usBoundPort = ( uint16_t ) socketGET_SOCKET_PORT( pxSocket );
pxNetworkBuffer->ulIPAddress = pxDestinationAddress->sin_addr;
Expand Down
20 changes: 15 additions & 5 deletions libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_UDP_IP.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,22 @@ UDPPacket_t *pxUDPPacket;
IPHeader_t *pxIPHeader;
eARPLookupResult_t eReturned;
uint32_t ulIPAddress = pxNetworkBuffer->ulIPAddress;
size_t uxPayloadSize;

/* Map the UDP packet onto the start of the frame. */
pxUDPPacket = ( UDPPacket_t * ) pxNetworkBuffer->pucEthernetBuffer;

#if ipconfigSUPPORT_OUTGOING_PINGS == 1
if( pxNetworkBuffer->usPort == ipPACKET_CONTAINS_ICMP_DATA )
{
uxPayloadSize = pxNetworkBuffer->xDataLength - sizeof( ICMPPacket_t );
}
else
#endif
{
uxPayloadSize = pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t );
}

/* Determine the ARP cache status for the requested IP address. */
eReturned = eARPGetCacheEntry( &( ulIPAddress ), &( pxUDPPacket->xEthernetHeader.xDestinationAddress ) );

Expand Down Expand Up @@ -109,7 +121,7 @@ uint32_t ulIPAddress = pxNetworkBuffer->ulIPAddress;

pxUDPHeader->usDestinationPort = pxNetworkBuffer->usPort;
pxUDPHeader->usSourcePort = pxNetworkBuffer->usBoundPort;
pxUDPHeader->usLength = ( uint16_t ) ( pxNetworkBuffer->xDataLength + sizeof( UDPHeader_t ) );
pxUDPHeader->usLength = ( uint16_t ) ( uxPayloadSize + sizeof( UDPHeader_t ) );
pxUDPHeader->usLength = FreeRTOS_htons( pxUDPHeader->usLength );
pxUDPHeader->usChecksum = 0u;
}
Expand Down Expand Up @@ -143,16 +155,14 @@ uint32_t ulIPAddress = pxNetworkBuffer->ulIPAddress;
if( pxNetworkBuffer->usPort == ipPACKET_CONTAINS_ICMP_DATA )
{
pxIPHeader->ucProtocol = ipPROTOCOL_ICMP;
pxIPHeader->usLength = ( uint16_t ) ( pxNetworkBuffer->xDataLength + sizeof( IPHeader_t ) );
pxIPHeader->usLength = ( uint16_t ) ( uxPayloadSize + sizeof( IPHeader_t ) + sizeof( ICMPHeader_t ) );
}
else
#endif /* ipconfigSUPPORT_OUTGOING_PINGS */
{
pxIPHeader->usLength = ( uint16_t ) ( pxNetworkBuffer->xDataLength + sizeof( IPHeader_t ) + sizeof( UDPHeader_t ) );
pxIPHeader->usLength = ( uint16_t ) ( uxPayloadSize + sizeof( IPHeader_t ) + sizeof( UDPHeader_t ) );
}

/* The total transmit size adds on the Ethernet header. */
pxNetworkBuffer->xDataLength = pxIPHeader->usLength + sizeof( EthernetHeader_t );
pxIPHeader->usLength = FreeRTOS_htons( pxIPHeader->usLength );
/* HT:endian: changed back to network endian */
pxIPHeader->ulDestinationIPAddress = pxNetworkBuffer->ulIPAddress;
Expand Down