Improve frame filtering

[HTRamsey]

Improve frame filtering

Description

Expand the checks in eConsiderFrameForProcessing to drop invalid frames earlier

Test Steps

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[htibosch]

That looks good, clean expressions. Thank you.

[htibosch]

Before this comment, I would expect the following conditional break:

        default:
            if( FreeRTOS_ntohs( pxEthernetHeader->usFrameType ) <= 0x600U )
            {
                #if ipconfigIS_ENABLED( ipconfigFILTER_OUT_NON_ETHERNET_II_FRAMES )
                    eReturn = eReleaseBuffer;
                #endif
            }
            else
            {
                #if ipconfigIS_DISABLED( ipconfigPROCESS_CUSTOM_ETHERNET_FRAMES )
                    eReturn = eReleaseBuffer;
                #endif
            }
            break;
    }    
+   if( eReturn == eReleaseBuffer )
+   {
+       /* No use to do more testing. */
+       break;
+   }

[HTRamsey]

Do you prefer that or using an if statement with breaks? I generally avoid places with multiple layers of breaks like switches in whiles.

[evpopov]

This is fairly expensive search for an end-point and wasn't it already done in the network driver?

[tony-josi-aws]

Yes, I believe either we need to assign the endpoint to the network buffer here or just use FreeRTOS_FindEndPointOnMAC as a preliminary check to match the MAC address along with LLMNR and MDNS MAC addresses if checks.
It doesn't make much sense to ignore the endpoint result here.

[HTRamsey]

Well if FreeRTOS_MatchingEndpoint is called from the NetworkInterface and ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 0, then neither should be done. If ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 1, the previous call to FreeRTOS_FindEndPointOnMAC is pretty much wasted work when it is half of the work done in FreeRTOS_MatchingEndpoint. But you have no way of assigning the pxEndpoint result unless you change the parameter or you are able to use pxPacketBuffer_to_NetworkBuffer.

[evpopov]

I am a bit confused. We cannot "assign the endpoint" in eConsiderFrameForProcessing() because:

• eConsiderFrameForProcessing() is an optional function. It's presence is controlled by ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES so if ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES is enabled, the endoint needs to be assigned elswhere.
• eConsiderFrameForProcessing() does not have access to the network buffer, so we cannot assign an endpoint in it.

I do agree that we should use FreeRTOS_FindEndPointOnMAC() to check if we need to drop the frame, however since eConsiderFrameForProcessing() doesn not have a pointer to the network interface in used to call:

pxEndPoint = FreeRTOS_FindEndPointOnMAC( &( pxEthernetHeader->xDestinationAddress ), NULL );

which is a bit useless as it can match the MAC of a different network interface.

My original comment was due to this:
The network driver ( or at least the DriverSAM that I'm familiar with ) already did:

pxNextNetworkBufferDescriptor->pxInterface = pxMyInterface;
pxNextNetworkBufferDescriptor->pxEndPoint = FreeRTOS_MatchingEndpoint( pxMyInterface, pxNextNetworkBufferDescriptor->pucEthernetBuffer );

if( pxNextNetworkBufferDescriptor->pxEndPoint == NULL )
{
    FreeRTOS_printf( ( "NetworkInterface: can not find a proper endpoint\n" ) );
    xRelease = pdTRUE;
}

Therefor, if we ever reached eConsiderFrameForProcessing(), the frame already has a good endpoint assigned.
As a matter of fact, calling

pxEndPoint = FreeRTOS_MatchingEndpoint( NULL, pucEthernetBuffer );

actually creates a problem of its own: FreeRTOS_MatchingEndpoint() internally will call pxEasyFit() with a NULL for an interface pointer. That causes pxEasyFit() to search on ALL interfaces and potentially return a match on IF_X even though the frame was received on IF_Y

I hope the above is clear, or maybe I'm missing something.

[evpopov]

@HTRamsey,
FreeRTOS_MatchingEndpoint() does not guarantee that the frame's destination MAC matches one of our end-point's MAC addresses. FreeRTOS_MatchingEndpoint() and pxEasyFit() could select an end-point base on IP-version or destination IP address, so with ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 0 the ONLY place that ever verifies the destination MAC of a frame against the MAC addresses of our endpoints is when eConsiderFrameForProcessing() does this:

pxEndPoint = FreeRTOS_FindEndPointOnMAC( &( pxEthernetHeader->xDestinationAddress ), NULL );

if( pxEndPoint != NULL )
{
    /* The packet was directed to this node - process it. */
    eReturn = eProcessBuffer;
}

I believe the code above is required, but if I were to ever try to improve eConsiderFrameForProcessing(), I would change it's parameter to pxNetworkBuffer and change the call above to:

pxEndPoint = FreeRTOS_FindEndPointOnMAC( &( pxEthernetHeader->xDestinationAddress ), pxNetworkBuffer );

I agree that calling FreeRTOS_FindEndPointOnMAC() does work that was already done in FreeRTOS_MatchingEndpoint() however, FreeRTOS_MatchingEndpoint() does not store information on whether it found a MAC match. If it could store that info somehow, then we wouldn't have to call FreeRTOS_FindEndPointOnMAC()

[HTRamsey]

I missed the prioritized methods in pxEasyFit, so you're right it could just check rMATCH_IP_ADDR. The other methods in pxEasyFit are checked in prvAllowIPPacketIPv4 & prvAllowIPPacketIPv6 where it calls FreeRTOS_FindEndPointOnIP_IPv4, FreeRTOS_FindEndPointOnMAC, etc. Briefly looking at pxEasyFit, I thought it checked all of them rather than going in some order.

I think this is where it would be beneficial to implement what I had mentioned here. I don't know how much it affects performance, but there are a few things that are checked multiple times, like endpoints and the IP header, etc.

Also if you had the pxNetworkBufferparameter, you could at least check data length too.

Edit: @evpopov I just realized calling eConsiderFrameForProcessing is inconsistent in the NetworkInterfaces. I don't think it's supposed to be optional as in 'some' Network Interface this is defined:

#if ( ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 0 )
    #define ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer )    eProcessBuffer
#else
    #define ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer ) \
    eConsiderFrameForProcessing( ( pucEthernetBuffer ) )
#endif

and in FreeRTOS_IP.c it is the inverse:

#if ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 0
    #define ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer )    eConsiderFrameForProcessing( ( pucEthernetBuffer ) )
#else
    #define ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer )    eProcessBuffer
#endif

So it looks like if it isn't called in the NetworkInterface it is supposed to be called in prvProcessEthernetPacket. But ATSAME5x does it backwards and maybe some others. I don't know it doesn't seem consistent but I think it isn't intended to be optional.

Ideally all these checks would be handled only once in the network interface in some manner like:
Check Frame -> Find Endpoint -> Check Packet -> Send to IP Task -> Process Packet

[evpopov]

Oh wow! I had not seen this. I just searched for ipCONSIDER_FRAME_FOR_PROCESSING in all the network drivers and you're right. It is defined all over the place.
BTW, from my prespective, the same5x driver defines it exactly as FreeRTOS_IP.c and every other network driver that defines ipCONSIDER_FRAME_FOR_PROCESSING is backwards.

I don't want to speculate on what those drivers are doing. It would be nice to get some insight from someone with more long-term experience but I think this macro shouldn't be defined in the network drivers.

[HTRamsey]

Maybe it can be placed before actually queuing the eNetworkRxEvent outside of the network interface. The less that's required in the network interface, the better in my opinion. That's also why I had previously tried to pull vNetworkInterfaceAllocateRAMToBuffers outside of the network drivers, because it's basically identical for every driver except for the location to store the packets.

[htibosch]

@HTRamsey wrote:

But you have no way of assigning the pxEndpoint result unless you change the parameter or you are able to use pxPacketBuffer_to_NetworkBuffer.

As discussed earlier, it is proposed that eConsiderFrameForProcessing() gets the packet descriptor as a parameter.

I am not a fan of using pxPacketBuffer_to_NetworkBuffer unless really necessary.

[htibosch]

First of all, thank you for the interesting discussion: @HTRamsey, @evpopov, and @tony-josi-aws .

The reception of packets has three important moments:

1. Before passing it to the IP-task for processing, we want to attach an endpoint to it.
   We have made this obligatory. This attached endpoint must be used everywhere in the reception path, up until calling the pfOutput() method.

2. Filtering at Ethernet level: eConsiderFrameForProcessing()

3. Before passing it to the protocol stacks, we want protocol checks: the IP-header, the length, the addresses

What happens where?

1. is performed by FreeRTOS_MatchingEndpoint() in the NetworkInterface

2. is either performed in the network interface or in the IP-task
   depending on ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES.

3. is performed by a chain of functions:

void prvProcessEthernetPacket( pxDescriptor )

   ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer );

   case ipARP_FRAME_TYPE:
        eARPProcessPacket()
   
   case ipIPv4_FRAME_TYPE:
   case ipIPv6_FRAME_TYPE:
        prvProcessIPPacket()
            prvAllowIPPacketIPv4/6()
            Check address fields
            Check checksum when necessary
            xCheckIPv[46]SizeFields()
            prvCheckIP4HeaderOptions/eHandleIPv6ExtensionHeaders()
            Check if ARP/ND resolution is needed
            Refresh ARP/ND address cache 
        case ipPROTOCOL_ICMP:
            ProcessICMPPacket() or prvProcessICMPMessage_IPv6()
        case ipPROTOCOL_UDP:
            prvProcessUDPPacket()
        case ipPROTOCOL_TCP:
            xProcessReceivedTCPPacket()
    default:
        eApplicationProcessCustomFrameHook() when not ARP or IPv4/6 frame type

I know there is still some filtering code in some network interfaces, that code should not be part of the network interface. I would just remove it because the IP-task already has good filtering.

@evpopov noticed that ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES is handled differently in FreeRTOS_IP.c and in a network interface.
If they were the same, eConsiderFrameForProcessing() would either be called in both modules, or it would never be called, depending on ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES.

The macro and the function have always lead to confusion and mistakes for end-users.

I would also like to see a new version of eConsiderFrameForProcessing(), and that also gets the packet descriptor as a parameter as Emil is suggesting:

eFrameProcessingResult_t eConsiderFrameForProcessing( NetworkBufferDescriptor_t * pxDescriptor );

The function will assume that the following are set:

pxDescriptor
pxDescriptor->pxInterface
pxDescriptor->pxEndPoint

@evpopov writes:

pxEndPoint = FreeRTOS_MatchingEndpoint( NULL, pucEthernetBuffer );
This is fairly expensive search for an end-point and wasn't it already done in the network driver?

Yes absolutely, and not necessary.
Although I see a call to FreeRTOS_FindEndPointOnMAC(), and not FreeRTOS_MatchingEndpoint().

@tony-josi-aws writes:

It doesn't make much sense to ignore the endpoint result here.

Yes indeed. eConsiderFrameForProcessing() must get the packet descriptor as a parameter, and use pxDescriptor->pxEndPoint.
I propose to use a new name and deprecate the current function name.

And maybe we can also check/reevaluate the use of ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES?

In FreeRTOS_IP.c, I would rather see this code, in stead of hiding it behind a macro:

#if( ipconfigIS_ENABLED( ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES ) )
    /* The network interface has checked the frame already. */
    eResult = eProcessBuffer;
#else
    /* Check if we must process this frame. */
    eResult = eConsiderFrameForProcessing();
#endif

The idea behind ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES was: shall we filter packet before or after the transfer to the IP-task. If there is an attack of bad/unknown frames, at least the IP message queue is not loaded.

[HTRamsey]

@htibosch personally I would remove ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES & ipconfigETHERNET_DRIVER_FILTERS_PACKETS because ideally you want to guarantee the filtering is done identically regardless of which NetworkInterface is used.

Then do steps 1, 2, & 3 that you listed all in a function like vReceivePacketFromNetworkInterface( NetworkBufferDescriptor_t * pxDescriptor ) that is called immediately after the NetworkInterface creates the buffer and attaches the datalength and interface. Only if the packet is completely valid do we then queue it, because queuing is where the most overhead is. This function handles xSendEventStructToIPTask and vReleaseNetworkBufferAndDescriptor. You've then abstracted away almost all of the TCP stack away from the NetworkInterface, just leaving the hardware portions that are actually unique per driver. Then when the IP task receives the buffer it can immediately go to the processing functions like xProcessReceivedTCPPacket.

Edit: Ideally the buffer is created after some filtering is done, so you don't have to immediately release it if the packet is invalid. Maybe you can pass data pointer, datalength, and interface as the parameters instead of a buffer then only create the buffer if you intend on passing the data to the queue. so something like vReceivePacketFromNetworkInterface( uint8_t * pucDMABuffer, size_t xDataLength, struct xNetworkInterface * pxInterface ). But that makes rx chaining more difficult/impossible.

[htibosch]

That looks good, clean code. I have two questions left. Thanks.

[htibosch]

@HTRamsey wrote:

But you have no way of assigning the pxEndpoint result unless you change the parameter or you are able to use pxPacketBuffer_to_NetworkBuffer.

As discussed earlier, it is proposed that eConsiderFrameForProcessing() gets the packet descriptor as a parameter.

I am not a fan of using pxPacketBuffer_to_NetworkBuffer unless really necessary.

[HTRamsey]

I don't know how much to do in this PR, or just wait. I fixed the current problems, if we want to move forward with the rest then I would do the following

eFrameProcessingResult_t eConsiderFrameForProcessing( const uint8_t * const pucEthernetBuffer )
{
    ( void ) pucEthernetBuffer;
    return eProcessBuffer;
}

BaseType_t xConsiderBufferForProcessing( NetworkBufferDescriptor_t * const pxNetworkBuffer )
{
    // Check to verify packet is completely valid
}

void vReceiveBufferFromNetworkInterface( NetworkBufferDescriptor_t * const pxNetworkBuffer )
{
    // Iterate through linked buffers like prvHandleEthernetPacket does
    iptraceNETWORK_INTERFACE_INPUT( pxNetworkBuffer->xDataLength, pxNetworkBuffer->pucEthernetBuffer );
    if( xConsiderBufferForProcessing( pxNetworkBuffer ) == pdTRUE )
    {
        // send to queue
    }
    else
    {
        // release buffer
    }
}

[tony-josi-aws]

NIT: This line is unnecessary as eReturn is already eReleaseBuffer here.

[tony-josi-aws]

MISRA 2012: else if constructs should be terminated with non-empty else case.

[HTRamsey]

If that else is reached, should we release the buffer? The destination address would be unsupported I guess.

Edit: Shouldn't we check for 0x01005E for IPv4 multicast, similar to how we do for IPv6?

[tony-josi-aws]

If that else is reached, should we release the buffer? The destination address would be unsupported I guess.

Yes, I believe its good to release the buffer.

Shouldn't we check for 0x01005E for IPv4 multicast, similar to how we do for IPv6?

For multicast support, this check should be required. It would be good to use the new multicast MAC address macros in vSetMultiCastIPv4MacAddress as well.

[evpopov]

Just a small side note:
In #1019 I've changed this check to allow all multicast MAC addresses if ipconfigSUPPORT_IP_MULTICAST is enabled. I might change that in the future, but with multicasts enabled and especially if ipconfigPROCESS_CUSTOM_ETHERNET_FRAMES is enabled, it really is too early to drop any multicasts at this stage.

[HTRamsey]

I'll change the multicast filtering part after that goes in, for now it just drops ipv6 multicasts if ipv6 is disabled and vice versa for ipv4

[tony-josi-aws]

@HTRamsey, thanks for the PR, this is a good improvement/cleanup. As the PR doesn't change finding endpoint logic, I believe the rest of the changes can be made as a separate PR.

[tony-josi-aws]

/bot run formatting