net/ip: fix input packet filtering criteria

[ghost]

The "is this packet for us?" filter in net_ipv4_input() has a minor
logic error which fails to discard many packets which are.. not for us.

Fixes: #14647

Signed-off-by: Charles E. Youse [email protected]

[ghost]

@dgloeck @jukkar I haven't tested these changes, but I believe they are correct. I'd appreciate it if you could verify.

[codecov-io]

Codecov Report

Merging #14656 into master will increase coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master   #14656      +/-   ##
==========================================
+ Coverage   51.97%   51.98%   +<.01%     
==========================================
  Files         309      309              
  Lines       45584    45584              
  Branches    10555    10555              
==========================================
+ Hits        23694    23697       +3     
+ Misses      17082    17079       -3     
  Partials     4808     4808

Impacted Files	Coverage Δ
subsys/net/ip/ipv4.c	54.19% <50%> (+2.29%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2320973...e82769c. Read the comment docs.

[jukkar]

We should never get into this function if the network packet destination IPv{4|6} address is not ours. Those things are already checked in ipv4.c:net_ipv4_input() line 211 and ipv6.c:net_ipv6_input() line 462 before the connection.c:net_conn_input() is called.
Could you elaborate how did you see this issue, perhaps attach pcap file that shows what the packet looks like?

Edit: as the bug was about IPv4 broadcast address, then ignore my references for IPv6 above. Anyway, we have a check for broadcast addresses in ipv4.c line 211, could you check what it does wrong in that line?

dismissed

[ghost]

@jukkar Yes, you're, right, I believe there's a flaw in the logic of that filter there that makes it too permissive. The && at the end of line 212 should be a ||. Basically, there are four criteria checked on the input packet.

1. It's for a unicast or broadcast address that we care about (my_addr)
2. It's for a multicast address we care about (addr_mcast)
3. It's an all-subnet UDP broadcast but we're not doing DHCP
4. It's a TCP packet that arrived at a broadcast address assigned to the incoming interface

Currently the logic says "drop the packet if it's (not #1 and not #2), and (either #3 or #4).
I think the logic should say "drop the packet if it's (not #1 and not #2), OR (either #3 or #4).

As it stands, any packet that gets through L2 that doesn't violate the case-specific restrictions outlined by #3 and #4 will get passed to the upper layers, if I'm reading this right.

[dgloeck]

e82769c works, no more bogus ICMP replies

[jukkar]

Thanks Charles!