do not let through unicast packets on inactive endpoints

[AlfaSegato]

Description

I am experiencing an issue when obtaining IP through DHCP, here is what happens:

• the MCU is on the network doing its thing, then it gets reset;
• the MCU issues a DHCP discover;
• the DHCP server already has a lease for the MCU’s MAC address, it tries to ping the lease before offering it;
• an ICMP ping request is issued with destination MAC of the MCU (ARP request is skipped as the host already has a fresh ARP entry for that MAC address);
• the MCU replies to that ICMP ping request!
• the DHCP server deems that lease occupied, so it abandons it and offers the next available IP.

This happens because the function "prvAllowIPPacketIPv4” will allow any unicast packet through when one endpoint is down.

The changes I propose affect the logic to now NOT let through unicast packets on inactive endpoints.
The function “prvAllowIPPacketIPv4” used to allow through any packet if at least one endpoint is down, so I removed that condition.
The function “FreeRTOS_FindEndPointOnIP_IPv4” used to return endpoints that are down too, so I removed that behaviour (ultimately to affect “prvAllowIPPacketIPv4” conditions); from what I understand, this change should not affect the other places where this function is called.

So now the packet is KEPT if ANY of the following:

• the packet dst IP is multicast
• the packet dst IP is broadcast
• there is an endpoint that either:
  • has an IP that matches packet’s dst IP
  • if packet dst IP == 0 (in which case any interface is fine)

Test Steps

Configure FreeRTOS+TCP in DHCP mode, host isc-dhcp-server, configure a very short lease time to increase chances of this issue appearing.
Alternatively, send any unicast IPv4 packet with MCU's MAC as destination MAC address while the endpoint is still down.

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

This is the forum post where this subject was discussed:
https://forums.freertos.org/t/all-unicast-packets-allowed-on-inactive-endpoint/21371

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

[AlfaSegato]

From what I can understand from the logs, both unit-test failures are coming from intentional changes:

• "FreeRTOS_FindEndPointOnIP_IPv4" no longer returns endpoints that are down;
• "prvAllowIPPacketIPv4" no longer calls "FreeRTOS_IsNetworkUp".

I don't know how to fix the unit-test. I only modified one one of them as it seemed easy, but I was not able to run the test locally on my PC.

I'm open to discussion.

[htibosch]

AlfaSegato, my compliments! You managed to analyse the problem and to find a solution for it. Also your PR text is good, it makes things very clear.

This happens because the function "prvAllowIPPacketIPv4” will allow any unicast packet through when one endpoint is down

Although most packets will be stopped later in the process, it is good to stop unwanted packets as early as possible, which is in prvAllowIPPacketIPv[46].

I tested your PR in a Xilinx/Zybo. I run the "Dual DHCP DNS Server" on my laptop so I can follow all DHCP traffic. My Router also responds to DHCP requests but it is much slower.

I tested both the Discover/Offer/Request/ACK sequence, as well as the Request/ACK to ask for a new lease period.

I'm curious to read other reviews, but I am positive.