FreeRTOS+TCP receives ICMP packets with bad IP checksum

Libraries
1

Please consider this post as more of a discussion than a bug-report, but I will start with what I believe is a bug.

Let’s consider an implementation where there is absolutely no CRC offloading.
In FreeRTOS_IP.c in prvAllowIPPacket() we have:

  #if( ipconfigDRIVER_INCLUDED_RX_IP_CHECKSUM == 0 )
  {
      /* Some drivers of NIC's with checksum-offloading will enable the above
      define, so that the checksum won't be checked again here */
      if (eReturn == eProcessBuffer )
      {
          /* Is the IP header checksum correct? */
          if( ( pxIPHeader->ucProtocol != ( uint8_t ) ipPROTOCOL_ICMP ) &&
              ( usGenerateChecksum( 0U, ( uint8_t * ) &( pxIPHeader->ucVersionHeaderLength ), ( size_t ) uxHeaderLength ) != ipCORRECT_CRC ) )
          {
              /* Check sum in IP-header not correct. */
              eReturn = eReleaseBuffer;
          }
          /* Is the upper-layer checksum (TCP/UDP/ICMP) correct? */
          else if( usGenerateProtocolChecksum( ( uint8_t * )( pxNetworkBuffer->pucEthernetBuffer ), pxNetworkBuffer->xDataLength, pdFALSE ) != ipCORRECT_CRC )
          {
              /* Protocol checksum not accepted. */
              eReturn = eReleaseBuffer;
          }
          else
          {
              /* The checksum of the received packet is OK. */
          }
      }
  }
  #else

An ICMP packet with a bad IP checksum will and does get received instead of dropped. I don’t understand why there was an extra check added to the if() that deals with checking the IP checksum. Removing the ICMP part fixes the issue:

  #if( ipconfigDRIVER_INCLUDED_RX_IP_CHECKSUM == 0 )
  {
      /* Some drivers of NIC's with checksum-offloading will enable the above
      define, so that the checksum won't be checked again here */
      if (eReturn == eProcessBuffer )
      {
          /* Is the IP header checksum correct? */
          if( ( usGenerateChecksum( 0U, ( uint8_t * ) &( pxIPHeader->ucVersionHeaderLength ), ( size_t ) uxHeaderLength ) != ipCORRECT_CRC ) )
          {
              /* Check sum in IP-header not correct. */
              eReturn = eReleaseBuffer;
          }
          /* Is the upper-layer checksum (TCP/UDP/ICMP) correct? */
          else if( usGenerateProtocolChecksum( ( uint8_t * )( pxNetworkBuffer->pucEthernetBuffer ), pxNetworkBuffer->xDataLength, pdFALSE ) != ipCORRECT_CRC )
          {
              /* Protocol checksum not accepted. */
              eReturn = eReleaseBuffer;
          }
          else
          {
              /* The checksum of the received packet is OK. */
          }
      }
  }
  #else

That concludes the bug-report. Let me know what you think… I might be missing something.

While I was messing around with checksum offloading disabled I realized that when I enable it in my driver I still see weirdness and this is where I need other people’s opinion. It turns out that the offloading in the MCU that I’m using ( atsame70q21 ) only does IP/TCP/UDP. This leaves the door open for other ICMP, IGMP and probably other protocols to be assumed as filtered by the driver and not checked by the stack.
Please chime in with what other MACs calculate.
In my case, the atsame70 can filter frames with bad IP/TCP/UDP checksums but will not attempt to filter packets with a bad ICMP checksum. Since I had ipconfigDRIVER_INCLUDED_RX_IP_CHECKSUM enabled, the stack was just assuming everything is fine.
I haven’t checked, but I’m afraid most MACs will be limited to IP/TCP/UDP which would mean that there were some wrong assumptions made when designing FreeRTOS+TCP. If I’m correct, the code would have to be modified to perform CRC calculation and verification for protocols like ICMP, IGMP, etc. even when the network driver provides rx crc offloading. The same logic also may apply to tx offloading when outgoing pings are enabled, but I haven’t checked that part of the code.
What do you guys and gals think, should we have a little more fine grained control of what the stack takes for granted?

2

Emil, thank you very much for your input. You are right that the test for ipPROTOCOL_ICMP should not be there: ICMP packets also have an IP-header!
If you want, you can prepare a PR for this, if not we will do so.

About ICMP and checksums: the behaviour of checksum-offloading differs per peripheral.
Some require that the driver clears the protocol checksum field of ICMP packet before sending. Yet other peripherals seem to exclude checksum-offloading for ICMP.
For the IP-checksum, and for the protocols TCP and UDP things are easier: no clearing is needed.

If the SAME70 does not verify the ICMP checksum of incoming packets, it is necessary to do this manually. This should happen in the function prvEMACRxPoll().
That’s another PR! When an ICMP packet is received, check the protocol checksum. That is for the SAM driver only.

I do not think it is necessary to create more configuration macros like ipconfigDRIVER_INCLUDED_RX_ICMP_CHECKSUM. We can change the behaviour per network interface.

3

Thanks for your comments Hein
I think I understand the stack’s approach a little better now. So we tell the stack that RX checksums are already checked, but if certain hardware doesn’t check all protocols, we’d handle this as an exception in the driver layer. That does make sense.

This seems like a very easy bug to fix, but I’m currently swamped at work and still don’t feel very comfortable with PRs. Would it be appropriate to submit an issue and let you guys fix it?

4

Hello Emil,
I have created a PR for the same: Include ICMP while calculating IP header checksum by AniruddhaKanhere · Pull Request #198 · FreeRTOS/FreeRTOS-Plus-TCP (github.com).

I will create another PR for adding the ICMP checksum in the prvEMACRxPoll function later on as pointed by Hein.

Thanks for bringing this issue to our attention.

5

:+1:
You guys are awesome