From 4edd48c7cfba6a6f7218d9a63f4a4ad935a9d5c9 Mon Sep 17 00:00:00 2001 From: Emil Popov Date: Fri, 28 Apr 2023 09:19:44 -0400 Subject: [PATCH 1/3] Defines ipUDP_PAYLOAD_IP_TYPE_OFFSET as an offset dependent on the IPv6 and UDP headers. Calculates ipIP_TYPE_OFFSET automatically based on the sizes it depends on instead of using a hardcoded number. Removes the definitions of ipIP_TYPE_OFFSET and ipUDP_PAYLOAD_IP_TYPE_OFFSET from FreeRTOS_IPv6_Private.h because they are already defined in FreeRTOS_IPv4_Private.h Makes ipIP_TYPE_OFFSET define signed so asserts can properly check for negative values. Adds an assert to ensure that storing of the IP-Type for IPv4 frames does not result in overwriting the ethernet header which would be the case if ipIP_TYPE_OFFSET somehow became negative or zero. Adds a comment to the code storing of the IP-Type byte for IPv6 frames emphasizing that it is not required and only used for debugging. --- source/FreeRTOS_IP.c | 8 ++++---- source/FreeRTOS_IP_Utils.c | 6 ++++++ source/include/FreeRTOS_IPv4_Private.h | 22 +++++++++++++++++----- source/include/FreeRTOS_IPv6_Private.h | 6 ------ 4 files changed, 27 insertions(+), 15 deletions(-) diff --git a/source/FreeRTOS_IP.c b/source/FreeRTOS_IP.c index c860f1fc1..865d0e44b 100644 --- a/source/FreeRTOS_IP.c +++ b/source/FreeRTOS_IP.c @@ -1824,9 +1824,9 @@ static eFrameProcessingResult_t prvProcessIPPacket( const IPPacket_t * pxIPPacke /* coverity[misra_c_2012_rule_11_3_violation] */ eReturn = prvAllowIPPacketIPv6( ( ( const IPHeader_IPv6_t * ) &( pxIPPacket->xIPHeader ) ), pxNetworkBuffer, uxHeaderLength ); - /* The IP-header type is copied to a location 6 bytes before the messages - * starts. It might be needed later on when a UDP-payload buffer is being - * used. */ + /* The IP-header type is copied to a special reserved location a few bytes before the message + * starts. In the case of IPv6, this value is never actually used and the line below can safely be removed + * with no ill effects. We only store it to help with debugging. */ pxNetworkBuffer->pucEthernetBuffer[ 0 - ( BaseType_t ) ipIP_TYPE_OFFSET ] = pxIPHeader_IPv6->ucVersionTrafficClass; } break; @@ -1854,7 +1854,7 @@ static eFrameProcessingResult_t prvProcessIPPacket( const IPPacket_t * pxIPPacke eReturn = prvAllowIPPacketIPv4( pxIPPacket, pxNetworkBuffer, uxHeaderLength ); { - /* The IP-header type is copied to a location 6 bytes before the + /* The IP-header type is copied to a special reserved location a few bytes before the * messages starts. It might be needed later on when a UDP-payload * buffer is being used. */ pxNetworkBuffer->pucEthernetBuffer[ 0 - ( BaseType_t ) ipIP_TYPE_OFFSET ] = pxIPHeader->ucVersionHeaderLength; diff --git a/source/FreeRTOS_IP_Utils.c b/source/FreeRTOS_IP_Utils.c index 8425295ed..7cf2d138c 100644 --- a/source/FreeRTOS_IP_Utils.c +++ b/source/FreeRTOS_IP_Utils.c @@ -996,6 +996,12 @@ void vPreCheckConfigs( void ) } #endif /* LCOV_EXCL_BR_STOP */ + + /* ipIP_TYPE_OFFSET is used like so: + * pxNetworkBuffer->pucEthernetBuffer[ 0 - ( BaseType_t ) ipIP_TYPE_OFFSET ] = IP-Version-Byte + * It's value MUST be > 0. Otherwise, storing the IPv4 version byte + * will overwrite the Ethernet header. */ + configASSERT( ipIP_TYPE_OFFSET > 0 ); } #endif /* if ( configASSERT_DEFINED == 1 ) */ } diff --git a/source/include/FreeRTOS_IPv4_Private.h b/source/include/FreeRTOS_IPv4_Private.h index eb83b5a47..d00b36fd3 100644 --- a/source/include/FreeRTOS_IPv4_Private.h +++ b/source/include/FreeRTOS_IPv4_Private.h @@ -45,11 +45,23 @@ /* The offset into a UDP packet at which the UDP data (payload) starts. */ #define ipUDP_PAYLOAD_OFFSET_IPv4 ( sizeof( UDPPacket_t ) ) /* The value of 'ipUDP_PAYLOAD_IP_TYPE_OFFSET' is 42 + 6 = 48 bytes. */ -/* __HT_ just to get it compiled. */ -#if !defined( ipIP_TYPE_OFFSET ) - #define ipIP_TYPE_OFFSET ( 6U ) -#endif -#define ipUDP_PAYLOAD_IP_TYPE_OFFSET ( sizeof( UDPPacket_t ) + ipIP_TYPE_OFFSET ) +#define ipUDP_PAYLOAD_IP_TYPE_OFFSET ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) ) + +/* ipIP_TYPE_OFFSET is involved in some sorcery. pxUDPPayloadBuffer_to_NetworkBuffer() must be able to convert + * a payload pointer ( like for example a pointer to the DNS payload of a packet ) back to a NetworkBufferDescriptor_t. + * This must work for both IPv4 and IPv6 packets. The pointer conversion is done by subtracting a constant from the payload pointer. + * For IPv6, this magic number is ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) ) which equals 48 bytes. + * If however we use that same constant for an IPv4 packet, we end up somewhere in front of the Ethernet header. + * In order to accommodate that, the Ethernet frame buffer gets allocated a bit larger than needed. + * For IPv4 frames, prvProcessIPPacket() stores the version header field at a negative offset, a few bytes before the start + * of the Ethernet header. That IPv4 version field MUST be stored the same distance from the payload as in IPv6. + * ipIP_TYPE_OFFSET must be equal to: sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) - ( sizeof( UDPPacket_t ) + * In most situations, ipIP_TYPE_OFFSET will end up being equal to 6. If the Ethernet header is enlarged to include VLAN + * tag support, ipIP_TYPE_OFFSET will shrink to 2. With the current design, the Ethernet header cannot be expanded to contain + * more than one VLAN tag or ipIP_TYPE_OFFSET will become less than zero. ipIP_TYPE_OFFSET should never be allowed to be <= 0 + * or storing of the IPv4 version byte will overwrite the Ethernet header of the frame. + */ +#define ipIP_TYPE_OFFSET ( ( int32_t ) sizeof( UDPHeader_t ) + ( int32_t ) sizeof( IPHeader_IPv6_t ) - ( int32_t ) sizeof( UDPPacket_t ) ) #include "pack_struct_start.h" struct xIP_HEADER diff --git a/source/include/FreeRTOS_IPv6_Private.h b/source/include/FreeRTOS_IPv6_Private.h index 6851fe8a4..669a36478 100644 --- a/source/include/FreeRTOS_IPv6_Private.h +++ b/source/include/FreeRTOS_IPv6_Private.h @@ -55,10 +55,6 @@ #undef TCP_PACKET_SIZE #define TCP_PACKET_SIZE ( sizeof( TCPPacket_IPv6_t ) ) -/* The offset from the UDP payload where the IP type will be stored. - * For IPv4 packets, this it located 6 bytes before pucEthernetBuffer. - * For IPv6 packets, this it located in the usual 'ucVersionTrafficClass'. */ -#define ipIP_TYPE_OFFSET ( 6U ) /* The offset into an IP packet into which the IP data (payload) starts. */ #define ipIPv6_PAYLOAD_OFFSET ( sizeof( IPPacket_IPv6_t ) ) /* MISRA Ref 20.5.1 [Use of undef] */ @@ -69,8 +65,6 @@ #define ipMAX_UDP_PAYLOAD_LENGTH ( ( ipconfigNETWORK_MTU - ipSIZE_OF_IPv6_HEADER ) - ipSIZE_OF_UDP_HEADER ) /* The offset into a UDP packet at which the UDP data (payload) starts. */ #define ipUDP_PAYLOAD_OFFSET_IPv6 ( sizeof( UDPPacket_IPv6_t ) ) -/* The value of 'ipUDP_PAYLOAD_IP_TYPE_OFFSET' is 42 + 6 = 48 bytes. */ -#define ipUDP_PAYLOAD_IPv6_TYPE_OFFSET ( sizeof( UDPPacket_IPv6_t ) + ipIP_TYPE_OFFSET ) #if ( ipconfigBYTE_ORDER == pdFREERTOS_LITTLE_ENDIAN ) From 4404b182e9e7a279131aef22ebab4a492586bee6 Mon Sep 17 00:00:00 2001 From: GitHub Action Date: Mon, 24 Jul 2023 21:00:26 +0000 Subject: [PATCH 2/3] Uncrustify: triggered by comment. --- source/FreeRTOS_IP_Utils.c | 2 +- source/include/FreeRTOS_IPv4_Private.h | 8 ++++---- source/include/FreeRTOS_IPv6_Private.h | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/source/FreeRTOS_IP_Utils.c b/source/FreeRTOS_IP_Utils.c index 7cf2d138c..ba895859b 100644 --- a/source/FreeRTOS_IP_Utils.c +++ b/source/FreeRTOS_IP_Utils.c @@ -997,7 +997,7 @@ void vPreCheckConfigs( void ) #endif /* LCOV_EXCL_BR_STOP */ - /* ipIP_TYPE_OFFSET is used like so: + /* ipIP_TYPE_OFFSET is used like so: * pxNetworkBuffer->pucEthernetBuffer[ 0 - ( BaseType_t ) ipIP_TYPE_OFFSET ] = IP-Version-Byte * It's value MUST be > 0. Otherwise, storing the IPv4 version byte * will overwrite the Ethernet header. */ diff --git a/source/include/FreeRTOS_IPv4_Private.h b/source/include/FreeRTOS_IPv4_Private.h index d00b36fd3..a22d762e8 100644 --- a/source/include/FreeRTOS_IPv4_Private.h +++ b/source/include/FreeRTOS_IPv4_Private.h @@ -36,14 +36,14 @@ #include "FreeRTOS_IP_Private.h" /* The maximum UDP payload length. */ -#define ipMAX_UDP_PAYLOAD_LENGTH ( ( ipconfigNETWORK_MTU - ipSIZE_OF_IPv4_HEADER ) - ipSIZE_OF_UDP_HEADER ) +#define ipMAX_UDP_PAYLOAD_LENGTH ( ( ipconfigNETWORK_MTU - ipSIZE_OF_IPv4_HEADER ) - ipSIZE_OF_UDP_HEADER ) -#define TCP_PACKET_SIZE ( sizeof( TCPPacket_t ) ) +#define TCP_PACKET_SIZE ( sizeof( TCPPacket_t ) ) /* The offset into an IP packet into which the IP data (payload) starts. */ -#define ipIP_PAYLOAD_OFFSET ( sizeof( IPPacket_t ) ) +#define ipIP_PAYLOAD_OFFSET ( sizeof( IPPacket_t ) ) /* The offset into a UDP packet at which the UDP data (payload) starts. */ -#define ipUDP_PAYLOAD_OFFSET_IPv4 ( sizeof( UDPPacket_t ) ) +#define ipUDP_PAYLOAD_OFFSET_IPv4 ( sizeof( UDPPacket_t ) ) /* The value of 'ipUDP_PAYLOAD_IP_TYPE_OFFSET' is 42 + 6 = 48 bytes. */ #define ipUDP_PAYLOAD_IP_TYPE_OFFSET ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) ) diff --git a/source/include/FreeRTOS_IPv6_Private.h b/source/include/FreeRTOS_IPv6_Private.h index 669a36478..16802aff5 100644 --- a/source/include/FreeRTOS_IPv6_Private.h +++ b/source/include/FreeRTOS_IPv6_Private.h @@ -62,9 +62,9 @@ /* coverity[misra_c_2012_rule_20_5_violation] */ /* The maximum UDP payload length. */ #undef ipMAX_UDP_PAYLOAD_LENGTH -#define ipMAX_UDP_PAYLOAD_LENGTH ( ( ipconfigNETWORK_MTU - ipSIZE_OF_IPv6_HEADER ) - ipSIZE_OF_UDP_HEADER ) +#define ipMAX_UDP_PAYLOAD_LENGTH ( ( ipconfigNETWORK_MTU - ipSIZE_OF_IPv6_HEADER ) - ipSIZE_OF_UDP_HEADER ) /* The offset into a UDP packet at which the UDP data (payload) starts. */ -#define ipUDP_PAYLOAD_OFFSET_IPv6 ( sizeof( UDPPacket_IPv6_t ) ) +#define ipUDP_PAYLOAD_OFFSET_IPv6 ( sizeof( UDPPacket_IPv6_t ) ) #if ( ipconfigBYTE_ORDER == pdFREERTOS_LITTLE_ENDIAN ) From 7b04e7cfd07276e54887781368631f365ee9152d Mon Sep 17 00:00:00 2001 From: ActoryOu Date: Wed, 26 Jul 2023 17:36:02 +0800 Subject: [PATCH 3/3] Correct the comment of ipUDP_PAYLOAD_IP_TYPE_OFFSET. --- source/include/FreeRTOS_IPv4_Private.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/include/FreeRTOS_IPv4_Private.h b/source/include/FreeRTOS_IPv4_Private.h index a22d762e8..2dbe15810 100644 --- a/source/include/FreeRTOS_IPv4_Private.h +++ b/source/include/FreeRTOS_IPv4_Private.h @@ -44,7 +44,7 @@ #define ipIP_PAYLOAD_OFFSET ( sizeof( IPPacket_t ) ) /* The offset into a UDP packet at which the UDP data (payload) starts. */ #define ipUDP_PAYLOAD_OFFSET_IPv4 ( sizeof( UDPPacket_t ) ) -/* The value of 'ipUDP_PAYLOAD_IP_TYPE_OFFSET' is 42 + 6 = 48 bytes. */ +/* The value of 'ipUDP_PAYLOAD_IP_TYPE_OFFSET' is 8 + 40 = 48 bytes. */ #define ipUDP_PAYLOAD_IP_TYPE_OFFSET ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) ) /* ipIP_TYPE_OFFSET is involved in some sorcery. pxUDPPayloadBuffer_to_NetworkBuffer() must be able to convert