-
Notifications
You must be signed in to change notification settings - Fork 165
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.
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
Changes from 2 commits
1803d90
91badc9
a57b099
e5a8f60
e9f9126
87c517d
2422335
9104b2e
f7d2e35
83f34fd
5669fd8
855d6d5
085741c
35a61c1
c706b49
364bc05
5b24042
a0b59ff
3c97367
7cdbef2
c29f055
5240680
31da177
c2cd991
3b9a259
d695740
d7ee3c2
c05141f
a73a8c1
20f16c2
1150510
16352be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 } }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good point. 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. */ | ||
|
@@ -242,14 +242,6 @@ const struct xIPv6_Address FreeRTOS_in6addr_loopback = { { 0, 0, 0, 0, 0, 0, 0, | |
*/ | ||
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. | ||
* | ||
|
@@ -270,34 +262,60 @@ static void xGetIPv6MulticastGroupID( const IPv6_Address_t * pxIPv6Address, | |
|
||
/*-----------------------------------------------------------*/ | ||
|
||
/** | ||
* @brief Check if the IP-address is an IPv6 loopback address. | ||
* | ||
* @param[in] ulIPAddress 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 ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 */ | ||
|
||
|
||
|
@@ -476,10 +494,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 ) ) && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I answered this and rearranged the code like this:
|
||
( ( 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 ) ) ) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -155,14 +155,6 @@ struct xIPv6_Couple | |
|
||
if( pxInterface != NULL ) | ||
{ | ||
/* This interface will be added to the end of the list of interfaces, so | ||
* there is no pxNext yet. */ | ||
pxInterface->pxNext = NULL; | ||
|
||
/* The end point for this interface has not yet been set. */ | ||
/*_RB_ As per other comments, why not set the end point at the same time? */ | ||
pxInterface->pxEndPoint = NULL; | ||
|
||
if( pxNetworkInterfaces == NULL ) | ||
{ | ||
/* No other interfaces are set yet, so this is the first in the list. */ | ||
|
@@ -189,6 +181,7 @@ struct xIPv6_Couple | |
if( pxIterator->pxNext == NULL ) | ||
{ | ||
pxIterator->pxNext = pxInterface; | ||
pxInterface->pxNext = NULL; | ||
break; | ||
} | ||
|
||
|
@@ -248,10 +241,6 @@ struct xIPv6_Couple | |
{ | ||
NetworkEndPoint_t * pxIterator = NULL; | ||
|
||
/* This end point will go to the end of the list, so there is no pxNext | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah i think this needs to be removed in the case the endpoint is already present on the list. |
||
* yet. */ | ||
pxEndPoint->pxNext = NULL; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But i think we should also add pxEndPoint->pxNext = NULL; in line 262 in case there is no endpoint in the list There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also can we move commit as a separate PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is 10 points for you, Monika, thanks! Although we assume that endpoints have been cleared, we can not count on it. |
||
/* Double link between the NetworkInterface_t that is using the addressing | ||
* defined by this NetworkEndPoint_t structure. */ | ||
pxEndPoint->pxNetworkInterface = pxInterface; | ||
|
@@ -285,6 +274,7 @@ struct xIPv6_Couple | |
|
||
if( pxIterator->pxNext == NULL ) | ||
{ | ||
pxEndPoint->pxNext = NULL; | ||
pxIterator->pxNext = pxEndPoint; | ||
break; | ||
} | ||
|
@@ -1452,6 +1442,7 @@ IPv6_Type_t xIPv6_GetIPType( const IPv6_Address_t * pxAddress ) | |
{ eIPv6_LinkLocal, 0xFFC0U, 0xFE80U }, /* 1111 1110 10 */ | ||
{ eIPv6_SiteLocal, 0xFFC0U, 0xFEC0U }, /* 1111 1110 11 */ | ||
{ eIPv6_Multicast, 0xFF00U, 0xFF00U }, /* 1111 1111 */ | ||
{ eIPv6_Loopback, 0xFFFFU, 0x0000U }, /* 0000 0000 ::1 */ | ||
}; | ||
|
||
if( pxAddress != NULL ) | ||
|
@@ -1462,6 +1453,15 @@ IPv6_Type_t xIPv6_GetIPType( const IPv6_Address_t * pxAddress ) | |
( uint16_t ) ( ( ( ( uint16_t ) pxAddress->ucBytes[ 0 ] ) << 8 ) | | ||
( ( uint16_t ) pxAddress->ucBytes[ 1 ] ) ); | ||
|
||
if( xIPCouples[ xIndex ].eType == eIPv6_Loopback ) | ||
{ | ||
if( xIsIPv6Loopback( &pxAddress ) != pdFALSE ) | ||
{ | ||
eResult = eIPv6_Loopback; | ||
break; | ||
} | ||
} | ||
|
||
if( ( usAddress & xIPCouples[ xIndex ].usMask ) == xIPCouples[ xIndex ].usExpected ) | ||
{ | ||
eResult = xIPCouples[ xIndex ].eType; | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubnil wrote:
The check is done in
xBadIPv4Loopback()
, which is called fromprvProcessUDPPacket()
, 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.Before we had a loopback interface, loopback addresses had to be dropped:
which would be the same as :
I think that we should have tested for both
ulDestinationIPAddress
andulSourceIPAddress
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:
when either the destination or the source address is a loopback address, the packet must be dropped.
All other packets may be processed.
There was a problem hiding this comment.
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.