Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preparing library to use loopback device #1020

Merged
merged 32 commits into from
Oct 6, 2023
Merged

Preparing library to use loopback device #1020

Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
1803d90
Preparing library to use loopback device
htibosch Sep 6, 2023
91badc9
Repaired FreeRTOS_AddEndPoint() as well
htibosch Sep 6, 2023
a57b099
Minor changes for Doxygen
htibosch Sep 7, 2023
e5a8f60
Uncrustify: triggered by comment.
actions-user Sep 7, 2023
e9f9126
Merge branch 'main' into prepare_for_loopback
moninom1 Sep 7, 2023
87c517d
Added IPv6.h and removed call to xIsIPv6Loopback
htibosch Sep 7, 2023
2422335
Conditional compilation of xIPv6_GetIPType()
htibosch Sep 7, 2023
9104b2e
Do not call xBadIPv4Loopback() when IPv4 is not enabled
htibosch Sep 8, 2023
f7d2e35
Repaired unit tests
htibosch Sep 8, 2023
83f34fd
Merge branch 'main' into prepare_for_loopback
htibosch Sep 10, 2023
5669fd8
In FreeRTOS_AddEndPoint(), set next to NULL
htibosch Sep 12, 2023
855d6d5
One more change in FreeRTOS_AddNetworkInterface()
htibosch Sep 13, 2023
085741c
FreeRTOS_FillEndPoint: save pxNext before clearing entire endpoint st…
htibosch Sep 13, 2023
35a61c1
Uncrustify: triggered by comment.
actions-user Sep 13, 2023
c706b49
Merge branch 'main' into prepare_for_loopback
tony-josi-aws Sep 13, 2023
364bc05
Changes after review by Shub
htibosch Sep 14, 2023
5b24042
Changes after review by Shub, part 2
htibosch Sep 14, 2023
a0b59ff
Uncrustify: triggered by comment.
actions-user Sep 14, 2023
3c97367
Merge branch 'main' into prepare_for_loopback
moninom1 Sep 19, 2023
7cdbef2
Replace pxUDPPacket with pxIPacket in function prvAllowIPPacketIPv4()
htibosch Sep 19, 2023
c29f055
utest: replace xIPv6UnspecifiedAddress with FreeRTOS_in6addr_any
htibosch Sep 19, 2023
5240680
Merge branch 'main' into prepare_for_loopback
htibosch Sep 22, 2023
31da177
Merge branch 'main' into prepare_for_loopback
htibosch Sep 26, 2023
c2cd991
Checked unit-tests and coverage
htibosch Sep 26, 2023
3b9a259
ut: Repaired GetIPType loopback test
htibosch Sep 27, 2023
d695740
Update test/unit-test/FreeRTOS_IPv6_ConfigDriverCheckChecksum/FreeRTO…
htibosch Sep 27, 2023
d7ee3c2
Update test/unit-test/FreeRTOS_IPv6/ut.cmake
htibosch Sep 27, 2023
c05141f
Remove test for 'ipIPv4_FRAME_TYPE'
htibosch Sep 28, 2023
a73a8c1
Repairing tu again
htibosch Sep 29, 2023
20f16c2
Merge branch 'main' into prepare_for_loopback
moninom1 Oct 4, 2023
1150510
Merge branch 'main' into prepare_for_loopback
htibosch Oct 5, 2023
16352be
Merge branch 'main' into prepare_for_loopback
moninom1 Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion source/FreeRTOS_ARP.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,12 @@ BaseType_t xCheckRequiresARPResolution( const NetworkBufferDescriptor_t * pxNetw
( ucNextHeader == ipPROTOCOL_UDP ) )
{
IPv6_Type_t eType = xIPv6_GetIPType( ( const IPv6_Address_t * ) pxIPAddress );
FreeRTOS_printf( ( "xCheckRequiresARPResolution: %pip type %s\n", ( void * ) pxIPAddress->ucBytes, ( eType == eIPv6_Global ) ? "Global" : ( eType == eIPv6_LinkLocal ) ? "LinkLocal" : "other" ) );
FreeRTOS_debug_printf( ( "xCheckRequiresARPResolution: %pip type %s\n",
( void * ) pxIPAddress->ucBytes,
( eType == eIPv6_Global ) ? "Global" :
( eType == eIPv6_LinkLocal ) ? "LinkLocal" :
( eType == eIPv6_Loopback ) ? "Loopback" :
"other" ) );

if( eType == eIPv6_LinkLocal )
{
Expand Down
26 changes: 11 additions & 15 deletions source/FreeRTOS_IP.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1702,21 +1702,17 @@ static eFrameProcessingResult_t prvProcessUDPPacket( NetworkBufferDescriptor_t *
/* Note the header values required prior to the checksum
* generation as the checksum pseudo header may clobber some of
* these values. */
if( ( pxUDPPacket->xEthernetHeader.usFrameType == ipIPv4_FRAME_TYPE ) &&
( usLength > ( FreeRTOS_ntohs( pxUDPPacket->xIPHeader.usLength ) - uxIPHeaderSizePacket( pxNetworkBuffer ) ) ) )
{
eReturn = eReleaseBuffer;
}
else if( ( pxUDPPacket->xEthernetHeader.usFrameType == ipIPv4_FRAME_TYPE ) &&
( ipFIRST_LOOPBACK_IPv4 <= ( FreeRTOS_ntohl( pxUDPPacket->xIPHeader.ulDestinationIPAddress ) ) ) &&
( ( FreeRTOS_ntohl( pxUDPPacket->xIPHeader.ulDestinationIPAddress ) ) < ipLAST_LOOPBACK_IPv4 ) )
{
/* The local loopback addresses must never appear outside a host. See RFC 1122
* section 3.2.1.3. */
eReturn = eReleaseBuffer;
}
else if( ( pxNetworkBuffer->xDataLength >= uxMinSize ) &&
( uxLength >= sizeof( UDPHeader_t ) ) )
#if ( ipconfigUSE_IPv4 != 0 )
shubnil marked this conversation as resolved.
Show resolved Hide resolved
if( ( pxUDPPacket->xEthernetHeader.usFrameType == ipIPv4_FRAME_TYPE ) &&
( usLength > ( FreeRTOS_ntohs( pxUDPPacket->xIPHeader.usLength ) - uxIPHeaderSizePacket( pxNetworkBuffer ) ) ) )
{
moninom1 marked this conversation as resolved.
Show resolved Hide resolved
eReturn = eReleaseBuffer;
}
else
#endif /* ( ipconfigUSE_IPv4 != 0 ) */

if( ( pxNetworkBuffer->xDataLength >= uxMinSize ) &&
( uxLength >= sizeof( UDPHeader_t ) ) )
{
size_t uxPayloadSize_1, uxPayloadSize_2;

Expand Down
58 changes: 58 additions & 0 deletions source/FreeRTOS_IPv4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,58 @@ BaseType_t xIsIPv4Multicast( uint32_t ulIPAddress )
}
/*-----------------------------------------------------------*/

/**
* @brief Check if the packet is an illegal loopback packet.
*
* @param[in] pxIPHeader The IP-header being checked.
*
* @return Returns pdTRUE if the packet should be stopped, because either the source
* or the target address is a loopback address.
*/
BaseType_t xBadIPv4Loopback( const IPHeader_t * const pxIPHeader )
{
BaseType_t xReturn = pdFALSE;
const NetworkEndPoint_t * pxEndPoint = FreeRTOS_FindEndPointOnIP_IPv4( pxIPHeader->ulSourceIPAddress, 3 );

/* Allow loopback packets from this node itself only. */
if( pxEndPoint != NULL )
{
BaseType_t x1 = ( xIsIPv4Loopback( pxIPHeader->ulDestinationIPAddress ) != 0 ) ? pdTRUE : pdFALSE;
BaseType_t x2 = ( xIsIPv4Loopback( pxIPHeader->ulSourceIPAddress ) != 0 ) ? pdTRUE : pdFALSE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

return xReturn;
}
/*-----------------------------------------------------------*/

/**
* @brief Is the IP address an IPv4 loopback address.
*
* @param[in] ulAddress The IP address being checked.
*
* @return pdTRUE if the IP address is a loopback address or else, pdFALSE.
*/
BaseType_t xIsIPv4Loopback( uint32_t ulAddress )
{
BaseType_t xReturn = pdFALSE;
uint32_t ulIP = FreeRTOS_ntohl( ulAddress );

if( ( ulIP >= ipFIRST_LOOPBACK_IPv4 ) &&
( ulIP < ipLAST_LOOPBACK_IPv4 ) )
{
xReturn = pdTRUE;
}

return xReturn;
}
/*-----------------------------------------------------------*/

/**
* @brief Check whether this IPv4 packet is to be allowed or to be dropped.
*
Expand Down Expand Up @@ -260,6 +312,12 @@ enum eFrameProcessingResult prvAllowIPPacketIPv4( const struct xIP_PACKET * cons
/* Can not handle, unknown or invalid header version. */
eReturn = eReleaseBuffer;
}
else if( xBadIPv4Loopback( &( pxIPPacket->xIPHeader ) ) == pdTRUE )
{
/* The local loopback addresses must never appear outside a host. See RFC 1122
* section 3.2.1.3. */
eReturn = eReleaseBuffer;
}
else if(
( FreeRTOS_FindEndPointOnIP_IPv4( ulDestinationIPAddress, 4 ) == NULL ) &&
( pxNetworkBuffer->pxEndPoint == NULL ) &&
Expand Down
68 changes: 40 additions & 28 deletions source/FreeRTOS_IPv6.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ const struct xIPv6_Address FreeRTOS_in6addr_any = { 0 };
/**
* This variable is initialized by the system to contain the loopback IPv6 address.
*/
const struct xIPv6_Address FreeRTOS_in6addr_loopback = { { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1 } };
const struct xIPv6_Address FreeRTOS_in6addr_loopback = { { 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 1U } };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 } };


#if ( ipconfigDRIVER_INCLUDED_RX_IP_CHECKSUM == 1 )
/* Check IPv6 packet length. */
Expand Down Expand Up @@ -237,19 +237,6 @@ const struct xIPv6_Address FreeRTOS_in6addr_loopback = { { 0, 0, 0, 0, 0, 0, 0,
#endif /* ( ipconfigDRIVER_INCLUDED_RX_IP_CHECKSUM == 1 ) */
/*-----------------------------------------------------------*/

/**
* This variable is initialized by the system to contain the unspecified IPv6 address.
*/
static const struct xIPv6_Address xIPv6UnspecifiedAddress = { { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 } };

#if ( ipconfigETHERNET_DRIVER_FILTERS_PACKETS == 0 )

/*
* Check if the packet is a loopback packet.
*/
static BaseType_t xIsIPv6Loopback( const IPHeader_IPv6_t * const pxIPv6Header );
#endif /* ipconfigETHERNET_DRIVER_FILTERS_PACKETS == 0 */

/**
* @brief Get the group ID and stored into IPv6_Address_t.
*
Expand All @@ -270,34 +257,60 @@ static void xGetIPv6MulticastGroupID( const IPv6_Address_t * pxIPv6Address,

/*-----------------------------------------------------------*/

/**
* @brief Check if the IP-address is an IPv6 loopback address.
*
* @param[in] pxAddress The IP-address being checked.
*
* @return pdTRUE if the IP-address is a loopback address or else, pdFALSE.
*/
BaseType_t xIsIPv6Loopback( const IPv6_Address_t * pxAddress )
{
BaseType_t xReturn = pdFALSE;

if( memcmp( pxAddress->ucBytes, FreeRTOS_in6addr_loopback.ucBytes, ipSIZE_OF_IPv6_ADDRESS ) == 0 )
{
xReturn = pdTRUE;
}

return xReturn;
}

#if ( ipconfigETHERNET_DRIVER_FILTERS_PACKETS == 0 )

/**
* @brief Check if the packet is a loopback packet.
* @brief Check if the packet is an illegal loopback packet.
*
* @param[in] pxIPv6Header The IP packet in pxNetworkBuffer.
* @param[in] pxIPv6Header The IP-header of the packet.
*
* @return Returns pdTRUE if it's a legal loopback packet, pdFALSE if not .
* @return Returns pdTRUE if the packet should be stopped, because either the source
* or the target address is a loopback address.
*/
/* MISRA Ref 8.9.1 [File scoped variables] */
/* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-89 */
/* coverity[misra_c_2012_rule_8_9_violation] */
/* coverity[single_use] */
static BaseType_t xIsIPv6Loopback( const IPHeader_IPv6_t * const pxIPv6Header )
BaseType_t xBadIPv6Loopback( const IPHeader_IPv6_t * const pxIPv6Header )
{
BaseType_t xReturn = pdFALSE;
const NetworkEndPoint_t * pxEndPoint = FreeRTOS_FindEndPointOnIP_IPv6( &( pxIPv6Header->xSourceAddress ) );

/* Allow loopback packets from this node itself only. */
if( ( pxEndPoint != NULL ) &&
( memcmp( pxIPv6Header->xDestinationAddress.ucBytes, FreeRTOS_in6addr_loopback.ucBytes, sizeof( IPv6_Address_t ) ) == 0 ) &&
( memcmp( pxIPv6Header->xSourceAddress.ucBytes, pxEndPoint->ipv6_settings.xIPAddress.ucBytes, sizeof( IPv6_Address_t ) ) == 0 ) )
if( pxEndPoint != NULL )
{
xReturn = pdTRUE;
BaseType_t x1 = ( xIsIPv6Loopback( &( pxIPv6Header->xDestinationAddress ) ) != 0 ) ? pdTRUE : pdFALSE;
BaseType_t x2 = ( xIsIPv6Loopback( &( pxIPv6Header->xSourceAddress ) ) != 0 ) ? pdTRUE : pdFALSE;

if( x1 != x2 )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already answered to this question here above.

{
/* Either source or the destination address is a loopback address. */
xReturn = pdTRUE;
}
}

return xReturn;
}

#endif /* ipconfigETHERNET_DRIVER_FILTERS_PACKETS == 0 */


Expand Down Expand Up @@ -332,7 +345,7 @@ BaseType_t xIsIPv6AllowedMulticast( const IPv6_Address_t * pxIPAddress )
* - ..
* - 0xFF0F:: */
else if( ( IPv6MC_GET_FLAGS_VALUE( pxIPAddress ) == 0U ) &&
( memcmp( xGroupIDAddress.ucBytes, xIPv6UnspecifiedAddress.ucBytes, sizeof( IPv6_Address_t ) ) == 0 ) )
( memcmp( xGroupIDAddress.ucBytes, FreeRTOS_in6addr_any.ucBytes, sizeof( IPv6_Address_t ) ) == 0 ) )
{
xReturn = pdFALSE;
}
Expand Down Expand Up @@ -462,8 +475,8 @@ eFrameProcessingResult_t prvAllowIPPacketIPv6( const IPHeader_IPv6_t * const pxI

/* Drop if packet has unspecified IPv6 address (defined in RFC4291 - sec 2.5.2)
* either in source or destination address. */
if( ( memcmp( pxDestinationIPAddress->ucBytes, xIPv6UnspecifiedAddress.ucBytes, sizeof( IPv6_Address_t ) ) == 0 ) ||
( memcmp( pxSourceIPAddress->ucBytes, xIPv6UnspecifiedAddress.ucBytes, sizeof( IPv6_Address_t ) ) == 0 ) )
if( ( memcmp( pxDestinationIPAddress->ucBytes, FreeRTOS_in6addr_any.ucBytes, sizeof( IPv6_Address_t ) ) == 0 ) ||
( memcmp( pxSourceIPAddress->ucBytes, FreeRTOS_in6addr_any.ucBytes, sizeof( IPv6_Address_t ) ) == 0 ) )
{
xHasUnspecifiedAddress = pdTRUE;
}
Expand All @@ -476,10 +489,9 @@ eFrameProcessingResult_t prvAllowIPPacketIPv6( const IPHeader_IPv6_t * const pxI
eReturn = eProcessBuffer;
}
/* Is it the legal multicast address? */
else if( ( xHasUnspecifiedAddress == pdFALSE ) &&
else if( ( ( xHasUnspecifiedAddress == pdFALSE ) &&
( xBadIPv6Loopback( pxIPv6Header ) == pdFALSE ) ) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I answered this and rearranged the code like this:

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

( ( xIsIPv6AllowedMulticast( pxDestinationIPAddress ) != pdFALSE ) ||
/* Is it loopback address sent from this node? */
( xIsIPv6Loopback( pxIPv6Header ) != pdFALSE ) ||
/* Or (during DHCP negotiation) we have no IP-address yet? */
( FreeRTOS_IsNetworkUp() == 0 ) ) )
{
Expand Down
3 changes: 2 additions & 1 deletion source/FreeRTOS_ND.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1254,7 +1254,8 @@

if( xResult == pdPASS )
{
configASSERT( ( uxPrefixLength > 0U ) && ( uxPrefixLength < ( 8U * ipSIZE_OF_IPv6_ADDRESS ) ) );
/* A loopback IP-address has a prefix of 128. */
configASSERT( ( uxPrefixLength > 0U ) && ( uxPrefixLength <= ( 8U * ipSIZE_OF_IPv6_ADDRESS ) ) );

if( uxPrefixLength >= 8U )
{
Expand Down
Loading
Loading