Do not forward datagrams original from 169.254.0.0/16 link-local addresses

[zlei]

Summary

The current implement of ip_input() reject packets destined for 169.254.0.0/16, but not those original from 169.254.0.0/16 link-local addresses.

Fix to fully respect RFC 3927 section 2.7.

PR: 255388

Test Plan

Set up an instance net.inet.ip.forwarding=1, input packets with source address 169.254.0.0/16 , netstat -s -p ip should show increasing packets not forwardable

[• karels]

Moving the code down fixes a couple of problems, but it introduces a new one: multicasts from link-locals can be forwarded if we are a multicast router. Maybe add a separate check in that section? We should still receive such multicasts.

[zlei]

In D29968#672733, @karels wrote:

Moving the code down fixes a couple of problems, but it introduces a new one: multicasts from link-locals can be forwarded if we are a multicast router. Maybe add a separate check in that section? We should still receive such multicasts.

Good catch.

[zlei]

RFC 3927 2.7: Do not forward IPv4 packets with a Link-Local source address even if they have a multicast destination address.

[donner]

May you please add context to the diff?

[donner]

sys/netinet/ip_input.c

750–751

If the source in not link local, then check for mrouter and drop it enventually.
So if the source is link local, forward it in any case?

[zlei]

In D29968#672749, @donner wrote:

May you please add context to the diff?

This is my first differential, and I'm not familiar with Phabricator.
I uploaded the diff from git diff main. How to add context to the diff ?
Thanks.

Got it. I'll add full context.

[1] https://wiki.freebsd.org/Phabricator

[zlei]

Added full context.

[zlei]

sys/netinet/ip_input.c

769

It seems wrong that ips_forward is increased unconditionally. Should increase only when ip_mforward() succeed?

[zlei]

sys/netinet/ip_input.c

750–751

If the source is link local, it is for ours.

[donner]

sys/netinet/ip_input.c

750–751

Then the condition belongs to line 740

if (V_ip_mrouter && !IN_LINKLOCAL{(ntohl(ip->ip_src.s_addr))) {

[zlei]

Then the condition belongs to line 740
if (V_ip_mrouter && !IN_LINKLOCAL{(ntohl(ip->ip_src.s_addr))) {

Update as @donner suggested.

[donner]

So we are currently handle all multicast packets from LL sources as locally handled streams regardless of the destination.
That might be correct, even if the local machine is processing the multicast destination itself. So it is able to receive such stream over the local link.

[rgrimes]

sys/netinet/ip_input.c

743–744

Remove RFC section ref s/datagrams/packets/ add "from":

/* Do not forward packets from IN_LINKLOCAL */

790

Extra blank line sould be removed

791

Three issues use of "datagrams", the other code uses "packets" and this use of the word "for" to describe to and from and RFC's change get/revised I do not see a need for specific number and secton reference here.

I suggest a clearer:

/* Do not forward packets to or from IN_LINKLOCAL */

[zlei]

Update as @rgrimes suggested.

[rgrimes]

I would still like to see the "169.254.0.0/16" changed to IN_LINKLOCAL, purpose of macro is to locate this value one place and one place only, scattering this string in the code undoes that.

[zlei]

In D29968#673077, @rgrimes wrote:

I would still like to see the "169.254.0.0/16" changed to IN_LINKLOCAL, purpose of macro is to locate this value one place and one place only, scattering this string in the code undoes that.

Sounds reasonable. Sorry I misunderstood the IN_LINKLOCAL.

[rgrimes]

I am accepting this without the IPSTAT_INC(ips_forward) issue being fixed, as it looks to me as if that is an existing and separate problem in the code. Probably a walk through should be done to see that ips_forward and ips_cantforward are all done correctly.

sys/netinet/ip_input.c

769

Looks like this still needs chased down. I tried to view full context in phabricator, but it totally messes up the screen layout and I can not read the code at all. I do agree that from what I can see this IPSTAT_INC looks wrong here.

798

Maybe here is where the IPSTAT_INC(ips_forward) belongs?

[zlei]

sys/netinet/ip_input.c

798

No. There's already IPSTAT_INC(ips_forward) in ip_forward() at line 1078 .
The above IPSTAT_INC(ips_forward) at line 763 is solely for ip_mforward() if I understand correctly.

[melifaro]

sys/netinet/ip_input.c

784

Q: given it's pretty neat /16, which splits an 32-bit address in two halves, would it be possible to come up with a macro that operates on network address, without doing ntohl()?

[• karels]

Re: avoiding ntohl(): that would be possible, but probably not within the scope of this change.

[zlei]

Friendly ping @donner , there're changes since your first acceptance, do you have any objections?

[zlei]

Hi, since I do not have access to the src repository, could someone check in the diff please ?

[zlei]

Thanks @donner :)