Improves the calculation of the offset at which we store the IP version value (FreeRTOS#979)

* 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.

* Uncrustify: triggered by comment.

* Correct the comment of ipUDP_PAYLOAD_IP_TYPE_OFFSET.

---------

Co-authored-by: Emil Popov <[email protected]>
Co-authored-by: GitHub Action <[email protected]>
Co-authored-by: ActoryOu <[email protected]>
Co-authored-by: Tony Josi <[email protected]>

Author: 5 people

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;

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 ) */
 }

source/include/FreeRTOS_IPv4_Private.h

@@ -37,20 +37,32 @@
 /* *INDENT-ON* */
 
 /* 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 ) )
-/* 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_OFFSET_IPv4       ( sizeof( UDPPacket_t ) )
+/* 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
+ * 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

source/include/FreeRTOS_IPv6_Private.h

@@ -56,22 +56,16 @@
 #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] */
 /* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-2051 */
 /* 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 ) )
-/* 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 )
+#define ipUDP_PAYLOAD_OFFSET_IPv6    ( sizeof( UDPPacket_IPv6_t ) )
 
 #if ( ipconfigBYTE_ORDER == pdFREERTOS_LITTLE_ENDIAN )