kern: wg: remove overly-restrictive address family check

[kevans]

Summary

IPv6 packets can be routed via an IPv4 nexthop and vice-versa, so the
handling of the parsed address family is more strict than it needs to
be. If we have a valid header that matches a known peer, then we have
no reason to decline the packet.

Add a test case that approximates a setup like in the PR and
demonstrates the issue.

PR: 284857

[kevans]

Ninja cleanup address assignments in the test

[zlei]

IPv6 packets can be routed via an IPv4 nexthop

IIRC that has not been implemented / supported yet, as IPv6 has much richer addresses, e.g., IPv6 via IPv6 LL nexthop, we do not need IPv6 via IPv4 nexthop.

and vice-versa

True. That is what https://reviews.freebsd.org/D30398 did. See also

# grep rfc5549 sys/net/route/route_ctl.c
FEATURE(ipv4_rfc5549_support, "Route IPv4 packets via IPv6 nexthops")

sys/dev/wg/if_wg.c

2338

The dst parameter is actually the gateway. D30398 makes that more clear. The original address family of a packet can be retrieved via the macro RO_GET_FAMILY().

2363

Then no need to remove this check.

[zlei]

sys/dev/wg/if_wg.c

2363

For wg_output(), m_unshare() may dup the mbuf, so the src / dst of a mbuf will not be changed, then re-determining the address family is redundant, So I think MPASS(parsed_af == af) is sufficient. Then we can mark parsed_af with __diagused and directly use af.

---	sa_family_t parsed_af;
+++	sa_family_t parsed_af __diagused;

...
---	if (parsed_af != af) {
---		xmit_err(ifp, m, NULL, AF_UNSPEC);
---		return (EAFNOSUPPORT);
---	}
+++	MPASS(parsed_af == af);

...

---	return (wg_xmit(ifp, m, parsed_af, mtu));
+++	return (wg_xmit(ifp, m, af, mtu));

[kevans]

sys/dev/wg/if_wg.c

2338

Wouldn't that also mean the conditional above is wrong? Seems like we should probably assign af = RO_GET_FAMILY(ro, dst); first, then copy dst->sa_data if af == AF_UNSPEC || af == pseudo_AF_HDRCMPLT

2363

I guess I don't have any reason to objection to leaving an assertion behind, though it seems likely to be redundant; presumably it's validated elsewhere in the stack at least once more.

[kevans]

In D49172#1122101, @zlei wrote:

IPv6 packets can be routed via an IPv4 nexthop

IIRC that has not been implemented / supported yet, as IPv6 has much richer addresses, e.g., IPv6 via IPv6 LL nexthop, we do not need IPv6 via IPv4 nexthop.

I noticed that when I went to write the test by copying the wg_basic test and setting up IPv6 on the other side. :-) If that's a known issue, I won't file a bug for it as I had considered -- it was mostly unexpected, as I'd pictured such a thing being useful to provide a legacy bridge to a modern network

[kevans]

sys/dev/wg/if_wg.c

2338

I see that gif does the same thing, so perhaps not -- reflecting a bit, I suppose those families would imply that it's not a gateway.

[zlei]

In D49172#1122546, @kevans wrote:

In D49172#1122101, @zlei wrote:

IPv6 packets can be routed via an IPv4 nexthop

IIRC that has not been implemented / supported yet, as IPv6 has much richer addresses, e.g., IPv6 via IPv6 LL nexthop, we do not need IPv6 via IPv4 nexthop.

I noticed that when I went to write the test by copying the wg_basic test and setting up IPv6 on the other side. :-)

That is a perfect valid usage.

If that's a known issue, I won't file a bug for it as I had considered -- it was mostly unexpected, as I'd pictured such a thing being useful to provide a legacy bridge to a modern network

D30398 had been landed prior to re-importing the WireGuard driver via D36909, probably the original author did not notice that the feature IPv4 via IPv6 nexthop is available at that time. I think I also missed that part while reviewing D36909.

sys/dev/wg/if_wg.c

2338

The condition if (dst->sa_family == AF_UNSPEC || dst->sa_family == pseudo_AF_HDRCMPLT) is right.

BPF may set dst->sa_family to pseudo_AF_HDRCMPLT and calls ifp->if_ouput() directly, and ro does not has valid nexthop or the original dest information. See https://cgit.freebsd.org/src/tree/sys/net/bpf.c#n1177 , also the change https://cgit.freebsd.org/src/commit/sys/net/bpf.c?id=4fb3a8208cf96df2a43eca52a11cbe339419e3a9 .

2338

The dst parameter is actually the gateway. D30398 makes that more clear. The original address family of a packet can be retrieved via the macro RO_GET_FAMILY().

Emm, that is not accurate.

1. BPF write has special HACK
2. For directly connected network, the dst is the destination of the packet
3. Otherwise, dst is the gateway, and ro contains information about the nexthop and original destination.

CC @melifaro .

2363

Or we can do that in a separated reviewing / commit, and leave it as is right now.

[kevans]

sys/dev/wg/if_wg.c

2363

I'm going to go ahead and incorporate this and your suggestion to use RO_GET_FAMILY as-is -- though I'm leaving the wg_xmit() call alone and usiing the parsed_af there so that where assertions are disabled, we're using the information from the packet if we did somehow end up with a mismatch and thus, wg_xmit will use the correct type for mtod().

[kevans]

Address feedback:

• Use RO_GET_FAMILY() to be more technically correct
• Assert that we get the same answer from the encapsulated ip hdr
• Fix the text description since we can't route ipv4 over an ipv6 nexthop

[zlei]

Looks good to me.
Just a reminder, the commit message should be revised as well.

[kevans]

Yes, sorry, I forgot that arc doesn't update commit messages. This is what I have locally:

kern: wg: remove overly-restrictive address family check

IPv4 packets can be routed via an IPv6 nexthop, so the handling of the
parsed address family is more strict than it needs to be.  If we have a
valid header that matches a known peer, then we have no reason to
decline the packet.

Convert it to an assertion that it matches the destination as viewed by
the stack below it, instead.  `dst` may be the gateway instead of the
destination in the case of a nexthop, so the `af` assignment must be
switched to use the destination.

Add a test case that approximates a setup like in the PR and
demonstrates the issue.

PR:             284857
Differential Revision:  https://reviews.freebsd.org/D49172