Preparing library to use loopback device

[htibosch]

This PR prepares the IP-stack to use loopback addresses 127.0.0.0 and ::1.

The issue was raised on the FreeRTOS forum here by ozanagma.

Description

● Routing.c :

It now recognises a new IP type for loopback: "eIPv6_Loopback"

Removed a minor bug in FreeRTOS_AddNetworkInterface(), and also FreeRTOS_AddEndPoint().

When adding the following interfaces:

    FreeRTOS_AddNetworkInterface( &iface_a );
    FreeRTOS_AddNetworkInterface( &iface_b );
    FreeRTOS_AddNetworkInterface( &iface_a );
    FreeRTOS_AddNetworkInterface( &iface_c );

In the above example, "iface_b" will become an orphan because of this piece of code:

    if( pxInterface != NULL )
    {
        pxInterface->pxNext = NULL;
        pxInterface->pxEndPoint = NULL;

● FreeRTOS_IPv6.h :

Added xBadIPv6Loopback()
Added xIsIPv6Loopback()

● FreeRTOS_IPv4.h :

Added xIsIPv4Loopback()
Added xBadIPv4Loopback()

Communication with a loopback device always takes place within a system, and from a loopback IP address to another loopback IP.

Here is how I implemented the XOR operation:

    BaseType_t xReturn = pdFALSE;
    if( pxEndPoint != NULL )
    {
        BaseType_t x1 = ( xIsIPv4Loopback( pxIPHeader->ulDestinationIPAddress ) != 0 ) ? pdTRUE : pdFALSE;
        BaseType_t x2 = ( xIsIPv4Loopback( pxIPHeader->ulSourceIPAddress ) != 0 ) ? pdTRUE : pdFALSE;

        if( x1 != x2 )
        {
            /* Either the source or the destination address is a loopback address. */
            xReturn = pdTRUE;
        }
    }

● FreeRTOS_ND.c :

The function FreeRTOS_CreateIPv6Address() will allow prefix address length of 128 bits, which is necessary to allow IPv6 loopback addresses.

● FreeRTOS_IP.c :

and the most important change:

-    else if( ( pxUDPPacket->xEthernetHeader.usFrameType == ipIPv4_FRAME_TYPE ) &&
-             ( ipFIRST_LOOPBACK_IPv4 <= ( FreeRTOS_ntohl( pxUDPPacket->xIPHeader.ulDestinationIPAddress ) ) ) &&
-             ( ( FreeRTOS_ntohl( pxUDPPacket->xIPHeader.ulDestinationIPAddress ) ) < ipLAST_LOOPBACK_IPv4 ) )
+    else if( ( pxUDPPacket->xEthernetHeader.usFrameType == ipIPv4_FRAME_TYPE ) &&
+             ( xBadIPv4Loopback( &( pxUDPPacket->xIPHeader ) ) != pdFALSE ) )
+    {
+        /* The local loopback addresses must never appear outside a host. See RFC 1122
+         * section 3.2.1.3. */
+        eReturn = eReleaseBuffer;
+    }

● FreeRTOS_IPv6.c :

a similar changes as in FreeRTOS_IP.c here above.

Test Steps

In my next PR I will present the loopback network interface.

Also I developed a module that starts 4 tasks to do TCPv4, TCPv6, UDPv4, and UDPv6. Each task will create to sockets that will exchange data through the loopback device.

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]

/bot run formatting

[htibosch]

/bot run formatting

[htibosch]

/bot run formatting

[moninom1]

Yeah i think this needs to be removed in the case the endpoint is already present on the list.

[moninom1]

But i think we should also add pxEndPoint->pxNext = NULL; in line 262 in case there is no endpoint in the list

[moninom1]

Also can we move commit as a separate PR?

[htibosch]

But i think we should also add pxEndPoint->pxNext = NULL; in line 262 in case there is no endpoint in the list

That is 10 points for you, Monika, thanks! Although we assume that endpoints have been cleared, we can not count on it.

[shubnil]

This logic is not very clear.Isn't it enough to check only for destination address.
In my understanding, We should only process packets for which the destination address is loopback. Source address can only be loopback if there is a bridging/routing supported in the device and as of now our stack does not support bridging/routing. And from an external device only the destination address can be loopback. Please suggest if I am missing anything here.

[htibosch]

@shubnil wrote:

+    if( pxEndPoint != NULL )
+    {
+        BaseType_t x1 = ( xIsIPv4Loopback( pxIPHeader->ulDestinationIPAddress ) != 0 ) ? pdTRUE : pdFALSE;
+        BaseType_t x2 = ( xIsIPv4Loopback( pxIPHeader->ulSourceIPAddress ) != 0 ) ? pdTRUE : pdFALSE;

This logic is not very clear. Isn't it enough to check only for destination address?

The check is done in xBadIPv4Loopback(), which is called from prvProcessUDPPacket(), which will be called for any incoming UDP packet.

It will return pdTRUE in case either the source or the destination address is a loopback address.

As described in RFC1122, Section 3.2.1.3, addresses within the entire 127.0.0.0/8 block do not legitimately appear on any network anywhere.

Before we had a loopback interface, loopback addresses had to be dropped:

    else if( ( pxUDPPacket->xEthernetHeader.usFrameType == ipIPv4_FRAME_TYPE ) &&
             ( ipFIRST_LOOPBACK_IPv4 <= ( FreeRTOS_ntohl( pxUDPPacket->xIPHeader.ulDestinationIPAddress ) ) ) &&
             ( ( FreeRTOS_ntohl( pxUDPPacket->xIPHeader.ulDestinationIPAddress ) ) < ipLAST_LOOPBACK_IPv4 ) )
    {
        eReturn = eReleaseBuffer;
    }

which would be the same as :

    else if( ( pxUDPPacket->xEthernetHeader.usFrameType == ipIPv4_FRAME_TYPE ) &&
               xIsIPv4Loopback( pxUDPPacket->xIPHeader.ulDestinationIPAddress ) )
    {
        /* Drop all packets travelling from or to a loopback address. */
        eReturn = eReleaseBuffer;
    }

I think that we should have tested for both ulDestinationIPAddress and ulSourceIPAddress because 127.x.x.x was not yet implemented.

Now we have added a loopback device, and so 127.x.x.x addresses must be allowed, with the exception of a packet leaving or entering the host. Loopback packets may only travel internally. So that is why:

    if( x1 != x2 )
    {
        /* Either the source or the destination address is an IPv4 loopback address. */
        xReturn = pdTRUE;
    }

when either the destination or the source address is a loopback address, the packet must be dropped.
All other packets may be processed.

[shubnil]

I would like understand a little more on Packets will loopback as Source address. What will be the use case for this? Is this for the packets originated from the loopback interface? In that case the packet should not go out of the system. Please suggest if this understanding is correct.

[shubnil]

Line no 65 and 243 both have a constant IPv6 address, however one is just const and other is static const. We should ideally have same for both.

[htibosch]

Line no 65 and 243 both have a constant IPv6 address, however one is just const and other is static const. We should ideally have same for both.

Good point. xIPv6UnspecifiedAddress[] is quite new and it is put halfway the file among the functions. The unspecified address is the same as the ANY address FreeRTOS_in6addr_any[], which already exists for a long time, and which is global. Let's use that in stead.

     const struct xIPv6_Address FreeRTOS_in6addr_any = { 0 };
     const struct xIPv6_Address FreeRTOS_in6addr_loopback = { { 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 1U } };
-    static const struct xIPv6_Address xIPv6UnspecifiedAddress = { { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 } };

[shubnil]

This logic is not very clear.Isn't it enough to check only for destination address.
In my understanding, We should only process packets for which the destination address is loopback. Source address can only be loopback if there is a bridging/routing supported in the device and as of now our stack does not support bridging/routing. And from an external device only the destination address can be loopback. Please suggest if I am missing anything here.

[htibosch]

I already answered to this question here above.

[shubnil]

Codewise it makes more sense to have xBadIPv6Loopback and xBadIPv4Loopback checks at the same level. Here, xBadIPv4Loopback check is happening at FreeRTOS_IP.c but xBadIPv6Loopback check is in IPv6 specfic file. Can we re-arrange the code.

[htibosch]

I answered this and rearranged the code like this:

prvAllowIPPacketIPv4() calls xBadIPv4Loopback()
prvAllowIPPacketIPv6() calls xBadIPv6Loopback()

[shubnil]

Overall, I see 2 main concerns:

1. The V4 and V6 loopback check arrangement is not uniform. V4 loopback check is in 'FreeRTOS_IP.c while V6 loopback check is inside FreeRTOS_IPv6.c. Both should be at same level, otherwise the code readability and protocol separation gets disturbed.
2. Inside the loopback checks isn't it enough to have loopback check only for destination address. Do we really need to check for source addresses as well to be loopback. If yes, Please give an example of such a used case.

[htibosch]

Thanks for your review, it makes sense. I will do one more integration test after applying the mentioned changed.

[htibosch]

Line no 65 and 243 both have a constant IPv6 address, however one is just const and other is static const. We should ideally have same for both.

Good point. xIPv6UnspecifiedAddress[] is quite new and it is put halfway the file among the functions. The unspecified address is the same as the ANY address FreeRTOS_in6addr_any[], which already exists for a long time, and which is global. Let's use that in stead.

     const struct xIPv6_Address FreeRTOS_in6addr_any = { 0 };
     const struct xIPv6_Address FreeRTOS_in6addr_loopback = { { 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 1U } };
-    static const struct xIPv6_Address xIPv6UnspecifiedAddress = { { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 } };

[htibosch]

I already answered to this question here above.

[htibosch]

I answered this and rearranged the code like this:

prvAllowIPPacketIPv4() calls xBadIPv4Loopback()
prvAllowIPPacketIPv6() calls xBadIPv6Loopback()

[htibosch]

/bot run formatting

[htibosch]

/bot run formatting

[shubnil]

Thanks Hein. I am fine with the changes.

[ActoryOu]

Left 2 minor comments.

[tony-josi-aws]

pxIPPacket->xEthernetHeader.usFrameType == ipIPv4_FRAME_TYPE

This checks seems to be redundant. prvAllowIPPacketIPv4 is only called when pxIPPacket->xEthernetHeader.usFrameType is ipIPv4_FRAME_TYPE from the prvProcessIPPacket().

[moninom1]

+1 for this, usFrameType can be removed. It is validated in prvProcessIPPacket and called from no where else.

[htibosch]

Yes good idea, it was tested already. I also adapted the test in FreeRTOS_IPv4_utest.c.

[htibosch]

Hm, the last change affects more ut's than I expected. I will first solve that locally.

[htibosch]

/bot run formatting