pf: Fix handling of v6 loopback connections with pf syncookies enabled

[markj]

Summary

The SYN|ACK generated by pf needs to inherit M_LOOP from the original
SYN, otherwise it gets dropped by ip6_input().

Fix this by adding an mbuf_flags argument to pf_build_tcp() that can be
used to set both M_SKIP_FIREWALL and M_LOOP as needed. Set M_LOOP on
the output mbuf if it was generated in response to an mbuf with M_LOOP
set.

Add a regression test case. The v4 case had no problems, but the v6
case fails without this change.

[kp]

Do you have a bit more detail on exactly how this breaks?

Is the problem that we're getting a scope mismatch because pf_build_tcp() does m->m_pkthdr.rcvif = V_loif?
That leaves me thinking we ought to set the correct rcvif (i.e. the one from the ACK that causes us to re-create the original SYN packet) rather than tell the stack to just ignore the scope check.

[markj]

In D47257#1077521, @kp wrote:

Do you have a bit more detail on exactly how this breaks?

Is the problem that we're getting a scope mismatch because pf_build_tcp() does m->m_pkthdr.rcvif = V_loif?
That leaves me thinking we ought to set the correct rcvif (i.e. the one from the ACK that causes us to re-create the original SYN packet) rather than tell the stack to just ignore the scope check.

For packets with src/dstaddr == ::1, this check in ip6_input() fails:

808                 if (V_ip6_sav && !(m->m_flags & M_LOOP) &&                                                                                                                                                                                                                                                               
809                     __predict_false(in6_localip_fib(&ip6->ip6_src,                                                                                                                                                                                                                                                       
810                             rcvif->if_fib))) {                                                                                                                                                                                                                                                                           
811                         printf("%s:%d\n", __func__, __LINE__);                                                                                                                                                                                                                                                           
812                         IP6STAT_INC(ip6s_badscope); /* XXX */                                                                                                                                                                                                                                                            
813                         goto bad;                                                                                                                                                                                                                                                                                        
814                 }

In particular, I don't think setting the rcvif is sufficient for that case, but I think you're right that we should also do that.

[kp]

In D47257#1077561, @markj wrote:

In particular, I don't think setting the rcvif is sufficient for that case, but I think you're right that we should also do that.

It wouldn't be, no, because I was thinking of cases where we received an external packet, and this is about packets on the loopback interface.
Especially because we already set rcvif = V_loif in pf_build_tcp().

sys/netpfil/pf/pf.c

2210

The IPv4 stack doesn't check for it, but we may as well set M_LOOP here too, to be consistent with the IPv6 case.

2231–2235

I wonder if we should MPASS(pfse->pfse_m->m_pkthdr.rcvif == V_loif); here.

[markj]

• Set M_LOOP for v4 packets as well.
• Add a suggested assertion.
• Test non-loopback addresses as well.

[markj]

In D47257#1077599, @kp wrote:

In D47257#1077561, @markj wrote:

In particular, I don't think setting the rcvif is sufficient for that case, but I think you're right that we should also do that.

It wouldn't be, no, because I was thinking of cases where we received an external packet, and this is about packets on the loopback interface.
Especially because we already set rcvif = V_loif in pf_build_tcp().

I think for now I'll leave the rcvif alone, since it's not necessary to change it for loopback traffic. The one edge case is loopback packets between link-local addresses - pf needs some logic to set the embedded scope ID, and I'm not sure it's worth the complexity yet.

[kp]

In D47257#1078043, @markj wrote:

I think for now I'll leave the rcvif alone, since it's not necessary to change it for loopback traffic. The one edge case is loopback packets between link-local addresses - pf needs some logic to set the embedded scope ID, and I'm not sure it's worth the complexity yet.

I'm inclined to agree.

I know there are issues with embedded scope in other contexts as wel. I believe rdr to loopback is still broken for IPv6 due to scope issues.
It's also extremely common for users to 'set skip on lo', so this is an edge case that mostly doesn't affect users anyway.