Improve frame filtering (#1100)

* add checks for frame types

* address review comments

* Fix multicast filtering and finding endpoint

* Remove ipCONSIDER_FRAME_FOR_PROCESSING macro

* add ipv4 multicast frame check

* make tests finish compiling

* fix tests

* use multicast macros

* improve test coverage

* Uncrustify: triggered by comment.

* Fix unit test to have full coverage

* Fix spell check and formatting

* Define macro to check ethernet frame type.

* Remove unnecessary declaration of xMDNS_MACAddressIPv6.

* Replace all usage of xMDNS_MACAddressIPv6 with xMDNS_MacAddressIPv6.

---------

Co-authored-by: GitHub Action <[email protected]>
Co-authored-by: ActoryOu <[email protected]>
Co-authored-by: Monika Singh <[email protected]>

Author: 4 people

source/FreeRTOS_DNS.c

@@ -57,6 +57,48 @@
 #include "FreeRTOS_DNS_Callback.h"
 
 
+/** @brief The MAC address used for LLMNR. */
+const MACAddress_t xLLMNR_MacAddress = { { 0x01, 0x00, 0x5e, 0x00, 0x00, 0xfc } };
+
+/** @brief The IPv6 link-scope multicast MAC address */
+const MACAddress_t xLLMNR_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x01, 0x00, 0x03 } };
+
+/** @brief The IPv6 link-scope multicast address */
+const IPv6_Address_t ipLLMNR_IP_ADDR_IPv6 =
+{
+    { /* ff02::1:3 */
+        0xff, 0x02,
+        0x00, 0x00,
+        0x00, 0x00,
+        0x00, 0x00,
+        0x00, 0x00,
+        0x00, 0x00,
+        0x00, 0x01,
+        0x00, 0x03,
+    }
+};
+
+/** @brief The MAC address used for MDNS. */
+const MACAddress_t xMDNS_MacAddress = { { 0x01, 0x00, 0x5e, 0x00, 0x00, 0xfb } };
+
+/** @brief The IPv6 multicast DNS MAC address. */
+const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB } };
+
+/** @brief multicast DNS IPv6 address */
+const IPv6_Address_t ipMDNS_IP_ADDR_IPv6 =
+{
+    { /* ff02::fb */
+        0xff, 0x02,
+        0x00, 0x00,
+        0x00, 0x00,
+        0x00, 0x00,
+        0x00, 0x00,
+        0x00, 0x00,
+        0x00, 0x00,
+        0x00, 0xfb,
+    }
+};
+
 /* Exclude the entire file if DNS is not enabled. */
 #if ( ipconfigUSE_DNS != 0 )
 
@@ -95,69 +137,7 @@
                                       struct freertos_addrinfo ** ppxAddressInfo,
                                       BaseType_t xFamily );
 
-    #if ( ipconfigUSE_LLMNR == 1 )
-    /** @brief The MAC address used for LLMNR. */
-    const MACAddress_t xLLMNR_MacAddress = { { 0x01, 0x00, 0x5e, 0x00, 0x00, 0xfc } };
-    #endif /* ipconfigUSE_LLMNR == 1 */
-
 /*-----------------------------------------------------------*/
-    #if ( ipconfigUSE_LLMNR == 1 ) && ( ipconfigUSE_IPv6 != 0 )
-
-/**
- * @brief The IPv6 link-scope multicast address
- */
-        const IPv6_Address_t ipLLMNR_IP_ADDR_IPv6 =
-        {
-            { /* ff02::1:3 */
-                0xff, 0x02,
-                0x00, 0x00,
-                0x00, 0x00,
-                0x00, 0x00,
-                0x00, 0x00,
-                0x00, 0x00,
-                0x00, 0x01,
-                0x00, 0x03,
-            }
-        };
-
-/**
- * @brief The IPv6 link-scope multicast MAC address
- */
-        const MACAddress_t xLLMNR_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x01, 0x00, 0x03 } };
-    #endif /* ipconfigUSE_LLMNR && ipconfigUSE_IPv6 */
-
-    #if ( ipconfigUSE_MDNS == 1 ) && ( ipconfigUSE_IPv6 != 0 )
-
-/**
- * @brief multicast DNS IPv6 address
- */
-        const IPv6_Address_t ipMDNS_IP_ADDR_IPv6 =
-        {
-            { /* ff02::fb */
-                0xff, 0x02,
-                0x00, 0x00,
-                0x00, 0x00,
-                0x00, 0x00,
-                0x00, 0x00,
-                0x00, 0x00,
-                0x00, 0x00,
-                0x00, 0xfb,
-            }
-        };
-
-/**
- * @brief The IPv6 multicast DNS MAC address.
- * The MAC-addresses are provided here in case a network
- * interface needs it.
- */
-        const MACAddress_t xMDNS_MacAddressIPv6 = { { 0x33, 0x33, 0x00, 0x00, 0x00, 0xFB } };
-    #endif /* ( ipconfigUSE_MDNS == 1 ) && ( ipconfigUSE_IPv6 != 0 ) */
-
-
-    #if ( ipconfigUSE_MDNS == 1 )
-    /** @brief The MAC address used for MDNS. */
-    const MACAddress_t xMDNS_MacAddress = { { 0x01, 0x00, 0x5e, 0x00, 0x00, 0xfb } };
-    #endif /* ipconfigUSE_MDNS == 1 */
 
 /** @brief This global variable is being used to indicate to the driver which IP type
  *         is preferred for name service lookup, either IPv6 or IPv4. */

source/FreeRTOS_IP.c

@@ -92,19 +92,11 @@
     #endif
 #endif
 
-/** @brief If ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES is set to 1, then the Ethernet
- * driver will filter incoming packets and only pass the stack those packets it
- * considers need processing.  In this case ipCONSIDER_FRAME_FOR_PROCESSING() can
- * be #-defined away.  If ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES is set to 0
- * then the Ethernet driver will pass all received packets to the stack, and the
- * stack must do the filtering itself.  In this case ipCONSIDER_FRAME_FOR_PROCESSING
- * needs to call eConsiderFrameForProcessing.
- */
-#if ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 0
-    #define ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer )    eConsiderFrameForProcessing( ( pucEthernetBuffer ) )
-#else
-    #define ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer )    eProcessBuffer
-#endif
+/** @brief The frame type field in the Ethernet header must have a value greater than 0x0600.
+ * If the configuration option ipconfigFILTER_OUT_NON_ETHERNET_II_FRAMES is enabled, the stack
+ * will discard packets with a frame type value less than or equal to 0x0600.
+ * However, if this option is disabled, the stack will continue to process these packets. */
+#define ipIS_ETHERNET_FRAME_TYPE_INVALID( usFrameType )    ( ( usFrameType ) <= 0x0600U )
 
 static void prvCallDHCP_RA_Handler( NetworkEndPoint_t * pxEndPoint );
 
@@ -1451,85 +1443,165 @@ BaseType_t xSendEventStructToIPTask( const IPStackEvent_t * pxEvent,
  */
 eFrameProcessingResult_t eConsiderFrameForProcessing( const uint8_t * const pucEthernetBuffer )
 {
-    eFrameProcessingResult_t eReturn = eProcessBuffer;
-    const EthernetHeader_t * pxEthernetHeader = NULL;
-    const NetworkEndPoint_t * pxEndPoint = NULL;
+    eFrameProcessingResult_t eReturn = eReleaseBuffer;
 
-    if( pucEthernetBuffer == NULL )
-    {
-        eReturn = eReleaseBuffer;
-    }
-    else
+    do
     {
-        /* Map the buffer onto Ethernet Header struct for easy access to fields. */
+        const EthernetHeader_t * pxEthernetHeader = NULL;
+        const NetworkEndPoint_t * pxEndPoint = NULL;
+        uint16_t usFrameType;
+
+        /* First, check the packet buffer is non-null. */
+        if( pucEthernetBuffer == NULL )
+        {
+            /* The packet buffer was null - release it. */
+            break;
+        }
 
+        /* Map the buffer onto Ethernet Header struct for easy access to fields. */
         /* MISRA Ref 11.3.1 [Misaligned access] */
         /* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-113 */
         /* coverity[misra_c_2012_rule_11_3_violation] */
         pxEthernetHeader = ( ( const EthernetHeader_t * ) pucEthernetBuffer );
+        usFrameType = pxEthernetHeader->usFrameType;
 
-        /* Examine the destination MAC from the Ethernet header to see if it matches
-         * that of an end point managed by FreeRTOS+TCP. */
+        /* Second, filter based on ethernet frame type. */
+        /* The frame type field in the Ethernet header must have a value greater than 0x0600. */
+        if( ipIS_ETHERNET_FRAME_TYPE_INVALID( FreeRTOS_ntohs( usFrameType ) ) )
+        {
+            /* The packet was not an Ethernet II frame */
+            #if ipconfigIS_ENABLED( ipconfigFILTER_OUT_NON_ETHERNET_II_FRAMES )
+                /* filtering is enabled - release it. */
+                break;
+            #else
+                /* filtering is disabled - continue filter checks. */
+            #endif
+        }
+        else if( usFrameType == ipARP_FRAME_TYPE )
+        {
+            /* The frame is an ARP type */
+            #if ipconfigIS_DISABLED( ipconfigUSE_IPv4 )
+                /* IPv4 is disabled - release it. */
+                break;
+            #else
+                /*  IPv4 is enabled - Continue filter checks. */
+            #endif
+        }
+        else if( usFrameType == ipIPv4_FRAME_TYPE )
+        {
+            /* The frame is an IPv4 type */
+            #if ipconfigIS_DISABLED( ipconfigUSE_IPv4 )
+                /* IPv4 is disabled - release it. */
+                break;
+            #else
+                /* IPv4 is enabled - Continue filter checks. */
+            #endif
+        }
+        else if( usFrameType == ipIPv6_FRAME_TYPE )
+        {
+            /* The frame is an IPv6 type */
+            #if ipconfigIS_DISABLED( ipconfigUSE_IPv6 )
+                /* IPv6 is disabled - release it. */
+                break;
+            #else
+                /* IPv6 is enabled - Continue filter checks. */
+            #endif
+        }
+        else
+        {
+            /* The frame is an unsupported Ethernet II type */
+            #if ipconfigIS_DISABLED( ipconfigPROCESS_CUSTOM_ETHERNET_FRAMES )
+                /* Processing custom ethernet frames is disabled - release it. */
+                break;
+            #else
+                /* Processing custom ethernet frames is enabled - Continue filter checks. */
+            #endif
+        }
+
+        /* Third, filter based on destination mac address. */
         pxEndPoint = FreeRTOS_FindEndPointOnMAC( &( pxEthernetHeader->xDestinationAddress ), NULL );
 
         if( pxEndPoint != NULL )
         {
-            /* The packet was directed to this node - process it. */
-            eReturn = eProcessBuffer;
+            /* A destination endpoint was found - Continue filter checks. */
         }
         else if( memcmp( xBroadcastMACAddress.ucBytes, pxEthernetHeader->xDestinationAddress.ucBytes, sizeof( MACAddress_t ) ) == 0 )
         {
-            /* The packet was a broadcast - process it. */
-            eReturn = eProcessBuffer;
+            /* The packet was a broadcast - Continue filter checks. */
         }
-        else
-        #if ( ( ipconfigUSE_LLMNR == 1 ) && ( ipconfigUSE_DNS != 0 ) )
-            if( memcmp( xLLMNR_MacAddress.ucBytes, pxEthernetHeader->xDestinationAddress.ucBytes, sizeof( MACAddress_t ) ) == 0 )
-            {
-                /* The packet is a request for LLMNR - process it. */
-                eReturn = eProcessBuffer;
-            }
-            else
-        #endif /* ipconfigUSE_LLMNR */
-        #if ( ( ipconfigUSE_MDNS == 1 ) && ( ipconfigUSE_DNS != 0 ) )
-            if( memcmp( xMDNS_MacAddress.ucBytes, pxEthernetHeader->xDestinationAddress.ucBytes, sizeof( MACAddress_t ) ) == 0 )
-            {
-                /* The packet is a request for MDNS - process it. */
-                eReturn = eProcessBuffer;
-            }
-            else
-        #endif /* ipconfigUSE_MDNS */
-        if( ( pxEthernetHeader->xDestinationAddress.ucBytes[ 0 ] == ipMULTICAST_MAC_ADDRESS_IPv6_0 ) &&
-            ( pxEthernetHeader->xDestinationAddress.ucBytes[ 1 ] == ipMULTICAST_MAC_ADDRESS_IPv6_1 ) )
+        else if( memcmp( xLLMNR_MacAddress.ucBytes, pxEthernetHeader->xDestinationAddress.ucBytes, sizeof( MACAddress_t ) ) == 0 )
         {
-            /* The packet is a request for LLMNR - process it. */
-            eReturn = eProcessBuffer;
+            /* The packet is a request for LLMNR using IPv4 */
+            #if ( ipconfigIS_DISABLED( ipconfigUSE_DNS ) || ipconfigIS_DISABLED( ipconfigUSE_LLMNR ) || ipconfigIS_DISABLED( ipconfigUSE_IPv4 ) )
+                /* DNS, LLMNR, or IPv4 is disabled - release it. */
+                break;
+            #else
+                /* DNS, LLMNR, and IPv4 are enabled - Continue filter checks. */
+            #endif
         }
-        else
+        else if( memcmp( xLLMNR_MacAddressIPv6.ucBytes, pxEthernetHeader->xDestinationAddress.ucBytes, sizeof( MACAddress_t ) ) == 0 )
         {
-            /* The packet was not a broadcast, or for this node, just release
-             * the buffer without taking any other action. */
-            eReturn = eReleaseBuffer;
+            /* The packet is a request for LLMNR using IPv6 */
+            #if ( ipconfigIS_DISABLED( ipconfigUSE_DNS ) || ipconfigIS_DISABLED( ipconfigUSE_LLMNR ) || ipconfigIS_DISABLED( ipconfigUSE_IPv6 ) )
+                /* DNS, LLMNR, or IPv6 is disabled - release it. */
+                break;
+            #else
+                /* DNS, LLMNR, and IPv6 are enabled - Continue filter checks. */
+            #endif
         }
-    }
-
-    #if ( ipconfigFILTER_OUT_NON_ETHERNET_II_FRAMES == 1 )
-    {
-        uint16_t usFrameType;
-
-        if( eReturn == eProcessBuffer )
+        else if( memcmp( xMDNS_MacAddress.ucBytes, pxEthernetHeader->xDestinationAddress.ucBytes, sizeof( MACAddress_t ) ) == 0 )
         {
-            usFrameType = pxEthernetHeader->usFrameType;
-            usFrameType = FreeRTOS_ntohs( usFrameType );
-
-            if( usFrameType <= 0x600U )
-            {
-                /* Not an Ethernet II frame. */
-                eReturn = eReleaseBuffer;
-            }
+            /* The packet is a request for MDNS using IPv4 */
+            #if ( ipconfigIS_DISABLED( ipconfigUSE_DNS ) || ipconfigIS_DISABLED( ipconfigUSE_MDNS ) || ipconfigIS_DISABLED( ipconfigUSE_IPv4 ) )
+                /* DNS, MDNS, or IPv4 is disabled - release it. */
+                break;
+            #else
+                /* DNS, MDNS, and IPv4 are enabled - Continue filter checks. */
+            #endif
+        }
+        else if( memcmp( xMDNS_MacAddressIPv6.ucBytes, pxEthernetHeader->xDestinationAddress.ucBytes, sizeof( MACAddress_t ) ) == 0 )
+        {
+            /* The packet is a request for MDNS using IPv6 */
+            #if ( ipconfigIS_DISABLED( ipconfigUSE_DNS ) || ipconfigIS_DISABLED( ipconfigUSE_MDNS ) || ipconfigIS_DISABLED( ipconfigUSE_IPv6 ) )
+                /* DNS, MDNS, or IPv6 is disabled - release it. */
+                break;
+            #else
+                /* DNS, MDNS, and IPv6 are enabled - Continue filter checks. */
+            #endif
+        }
+        else if( ( pxEthernetHeader->xDestinationAddress.ucBytes[ 0 ] == ipMULTICAST_MAC_ADDRESS_IPv4_0 ) &&
+                 ( pxEthernetHeader->xDestinationAddress.ucBytes[ 1 ] == ipMULTICAST_MAC_ADDRESS_IPv4_1 ) &&
+                 ( pxEthernetHeader->xDestinationAddress.ucBytes[ 2 ] == ipMULTICAST_MAC_ADDRESS_IPv4_2 ) &&
+                 ( pxEthernetHeader->xDestinationAddress.ucBytes[ 3 ] <= 0x7fU ) )
+        {
+            /* The packet is an IPv4 Multicast */
+            #if ipconfigIS_DISABLED( ipconfigUSE_IPv4 )
+                /* IPv4 is disabled - release it. */
+                break;
+            #else
+                /* IPv4 is enabled - Continue filter checks. */
+            #endif
+        }
+        else if( ( pxEthernetHeader->xDestinationAddress.ucBytes[ 0 ] == ipMULTICAST_MAC_ADDRESS_IPv6_0 ) &&
+                 ( pxEthernetHeader->xDestinationAddress.ucBytes[ 1 ] == ipMULTICAST_MAC_ADDRESS_IPv6_1 ) )
+        {
+            /* The packet is an IPv6 Multicast */
+            #if ipconfigIS_DISABLED( ipconfigUSE_IPv6 )
+                /* IPv6 is disabled - release it. */
+                break;
+            #else
+                /* IPv6 is enabled - Continue filter checks. */
+            #endif
+        }
+        else
+        {
+            /* The packet was not a broadcast, or for this node - release it */
+            break;
         }
-    }
-    #endif /* ipconfigFILTER_OUT_NON_ETHERNET_II_FRAMES == 1  */
+
+        /* All checks have been passed, process the packet. */
+        eReturn = eProcessBuffer;
+    } while( ipFALSE_BOOL );
 
     return eReturn;
 }
@@ -1575,8 +1647,6 @@ static void prvProcessEthernetPacket( NetworkBufferDescriptor_t * const pxNetwor
             break;
         }
 
-        eReturned = ipCONSIDER_FRAME_FOR_PROCESSING( pxNetworkBuffer->pucEthernetBuffer );
-
         /* Map the buffer onto the Ethernet Header struct for easy access to the fields. */
 
         /* MISRA Ref 11.3.1 [Misaligned access] */
@@ -1586,7 +1656,7 @@ static void prvProcessEthernetPacket( NetworkBufferDescriptor_t * const pxNetwor
 
         /* The condition "eReturned == eProcessBuffer" must be true. */
         #if ( ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 0 )
-            if( eReturned == eProcessBuffer )
+            if( eConsiderFrameForProcessing( pxNetworkBuffer->pucEthernetBuffer ) == eProcessBuffer )
         #endif
         {
             /* Interpret the received Ethernet packet. */

source/FreeRTOS_IPv4_Utils.c

@@ -59,9 +59,9 @@ void vSetMultiCastIPv4MacAddress( uint32_t ulIPAddress,
 {
     uint32_t ulIP = FreeRTOS_ntohl( ulIPAddress );
 
-    pxMACAddress->ucBytes[ 0 ] = ( uint8_t ) 0x01U;
-    pxMACAddress->ucBytes[ 1 ] = ( uint8_t ) 0x00U;
-    pxMACAddress->ucBytes[ 2 ] = ( uint8_t ) 0x5EU;
+    pxMACAddress->ucBytes[ 0 ] = ( uint8_t ) ipMULTICAST_MAC_ADDRESS_IPv4_0;
+    pxMACAddress->ucBytes[ 1 ] = ( uint8_t ) ipMULTICAST_MAC_ADDRESS_IPv4_1;
+    pxMACAddress->ucBytes[ 2 ] = ( uint8_t ) ipMULTICAST_MAC_ADDRESS_IPv4_2;
     pxMACAddress->ucBytes[ 3 ] = ( uint8_t ) ( ( ulIP >> 16 ) & 0x7fU ); /* Use 7 bits. */
     pxMACAddress->ucBytes[ 4 ] = ( uint8_t ) ( ( ulIP >> 8 ) & 0xffU );  /* Use 8 bits. */
     pxMACAddress->ucBytes[ 5 ] = ( uint8_t ) ( ( ulIP ) & 0xffU );       /* Use 8 bits. */