git: 5ab1e5f7e558 - main - pf: improve the ICMPv6 direction check

From: Kristof Provost <kp_at_FreeBSD.org>
Date: Sun, 01 Sep 2024 15:06:08 UTC
The branch main has been updated by kp:

URL: https://cgit.FreeBSD.org/src/commit/?id=5ab1e5f7e5585558a73b723f07528977a82cee82

commit 5ab1e5f7e5585558a73b723f07528977a82cee82
Author:     Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2024-08-26 12:59:38 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2024-09-01 15:05:28 +0000

    pf: improve the ICMPv6 direction check
    
    Following bluhm's advice this changes the way we setup state keys and
    perform state lookups for ICMPv6 Neighbor Discovery packets:
      - replace the NS-dst with ND target address;
      - replace the NA-src with ND target address;
      - replace the NA-dst with unspecified address if it is a multicast.
    
    This allows pf to match Address Resolution, Neighbor Unreachability
    Detection and Duplicate Address Detection packets to the corresponding
    states without the need to create new ones or match unrelated ones.
    As a side effect we're doing now one state table lookup for ND packets
    instead of two.
    
    Fixes a bug uncovered by one of the previous commits that virtually
    breaks IPv6 connectivity after few minutes of use.
    
    ok stsp henning, with and ok bluhm
    
    PR:             280701
    MFC after:      1 week
    Obtained from:  OpenBSD, mikeb <mikeb@openbsd.org>, 2633ae8c4c8a
    Sponsored by:   Rubicon Communications, LLC ("Netgate")
---
 sys/net/pfvar.h        |   4 +-
 sys/netpfil/pf/pf.c    | 116 ++++++++++++++++++++++++++++++++++---------------
 sys/netpfil/pf/pf_lb.c |   2 +-
 3 files changed, 85 insertions(+), 37 deletions(-)

diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 10bf527c025d..43d4b908a407 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -2576,8 +2576,8 @@ u_short			 pf_get_translation(struct pf_pdesc *, struct mbuf *,
 			    uint16_t, uint16_t, struct pf_kanchor_stackframe *,
 			    struct pf_krule **);
 
-struct pf_state_key	*pf_state_key_setup(struct pf_pdesc *, struct pf_addr *,
-			    struct pf_addr *, u_int16_t, u_int16_t);
+struct pf_state_key	*pf_state_key_setup(struct pf_pdesc *, struct mbuf *, int,
+			    struct pf_addr *, struct pf_addr *, u_int16_t, u_int16_t);
 struct pf_state_key	*pf_state_key_clone(const struct pf_state_key *);
 void			 pf_rule_to_actions(struct pf_krule *,
 			    struct pf_rule_actions *);
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index acbaf304995f..b4f8bdeeff24 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -331,6 +331,9 @@ static int		 pf_create_state(struct pf_krule *, struct pf_krule *,
 			    u_int16_t, u_int16_t, int *, struct pfi_kkif *,
 			    struct pf_kstate **, int, u_int16_t, u_int16_t,
 			    int, struct pf_krule_slist *);
+static int		 pf_state_key_addr_setup(struct pf_pdesc *, struct mbuf *,
+			    int, struct pf_state_key_cmp *, int, struct pf_addr *,
+			    int, struct pf_addr *, int);
 static int		 pf_test_fragment(struct pf_krule **, struct pfi_kkif *,
 			    struct mbuf *, void *, struct pf_pdesc *,
 			    struct pf_krule **, struct pf_kruleset **);
@@ -347,7 +350,7 @@ static int		 pf_test_state_udp(struct pf_kstate **,
 			    void *, struct pf_pdesc *);
 int			 pf_icmp_state_lookup(struct pf_state_key_cmp *,
 			    struct pf_pdesc *, struct pf_kstate **, struct mbuf *,
-			    int, struct pfi_kkif *, u_int16_t, u_int16_t,
+			    int, int, struct pfi_kkif *, u_int16_t, u_int16_t,
 			    int, int *, int, int);
 static int		 pf_test_state_icmp(struct pf_kstate **,
 			    struct pfi_kkif *, struct mbuf *, int,
@@ -401,7 +404,7 @@ extern struct proc *pf_purge_proc;
 
 VNET_DEFINE(struct pf_limit, pf_limits[PF_LIMIT_MAX]);
 
-enum { PF_ICMP_MULTI_NONE, PF_ICMP_MULTI_SOLICITED, PF_ICMP_MULTI_LINK };
+enum { PF_ICMP_MULTI_NONE, PF_ICMP_MULTI_LINK };
 
 #define	PACKET_UNDO_NAT(_m, _pd, _off, _s)		\
 	do {								\
@@ -1495,9 +1498,66 @@ pf_state_key_ctor(void *mem, int size, void *arg, int flags)
 	return (0);
 }
 
+static int
+pf_state_key_addr_setup(struct pf_pdesc *pd, struct mbuf *m, int off,
+    struct pf_state_key_cmp *key, int sidx, struct pf_addr *saddr,
+    int didx, struct pf_addr *daddr, int multi)
+{
+#ifdef INET6
+	struct nd_neighbor_solicit nd;
+	struct pf_addr *target;
+	u_short action, reason;
+
+	if (pd->af == AF_INET || pd->proto != IPPROTO_ICMPV6)
+		goto copy;
+
+	switch (pd->hdr.icmp6.icmp6_type) {
+	case ND_NEIGHBOR_SOLICIT:
+		if (multi)
+			return (-1);
+		if (!pf_pull_hdr(m, off, &nd, sizeof(nd), &action, &reason, pd->af))
+			return (-1);
+		target = (struct pf_addr *)&nd.nd_ns_target;
+		daddr = target;
+		break;
+	case ND_NEIGHBOR_ADVERT:
+		if (multi)
+			return (-1);
+		if (!pf_pull_hdr(m, off, &nd, sizeof(nd), &action, &reason, pd->af))
+			return (-1);
+		target = (struct pf_addr *)&nd.nd_ns_target;
+		saddr = target;
+		if (IN6_IS_ADDR_MULTICAST(&pd->dst->v6)) {
+			key->addr[didx].addr32[0] = 0;
+			key->addr[didx].addr32[1] = 0;
+			key->addr[didx].addr32[2] = 0;
+			key->addr[didx].addr32[3] = 0;
+			daddr = NULL; /* overwritten */
+		}
+		break;
+	default:
+		if (multi == PF_ICMP_MULTI_LINK) {
+			key->addr[sidx].addr32[0] = IPV6_ADDR_INT32_MLL;
+			key->addr[sidx].addr32[1] = 0;
+			key->addr[sidx].addr32[2] = 0;
+			key->addr[sidx].addr32[3] = IPV6_ADDR_INT32_ONE;
+			saddr = NULL; /* overwritten */
+		}
+	}
+copy:
+#endif
+	if (saddr)
+		PF_ACPY(&key->addr[sidx], saddr, pd->af);
+	if (daddr)
+		PF_ACPY(&key->addr[didx], daddr, pd->af);
+
+	return (0);
+}
+
 struct pf_state_key *
-pf_state_key_setup(struct pf_pdesc *pd, struct pf_addr *saddr,
-	struct pf_addr *daddr, u_int16_t sport, u_int16_t dport)
+pf_state_key_setup(struct pf_pdesc *pd, struct mbuf *m, int off,
+    struct pf_addr *saddr, struct pf_addr *daddr, u_int16_t sport,
+    u_int16_t dport)
 {
 	struct pf_state_key *sk;
 
@@ -1505,8 +1565,12 @@ pf_state_key_setup(struct pf_pdesc *pd, struct pf_addr *saddr,
 	if (sk == NULL)
 		return (NULL);
 
-	PF_ACPY(&sk->addr[pd->sidx], saddr, pd->af);
-	PF_ACPY(&sk->addr[pd->didx], daddr, pd->af);
+	if (pf_state_key_addr_setup(pd, m, off, (struct pf_state_key_cmp *)sk,
+	    pd->sidx, pd->src, pd->didx, pd->dst, 0)) {
+		uma_zfree(V_pf_state_key_z, sk);
+		return (NULL);
+	}
+
 	sk->port[pd->sidx] = sport;
 	sk->port[pd->didx] = dport;
 	sk->proto = pd->proto;
@@ -5210,7 +5274,7 @@ pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a,
 	if (nr == NULL) {
 		KASSERT((sk == NULL && nk == NULL), ("%s: nr %p sk %p, nk %p",
 		    __func__, nr, sk, nk));
-		sk = pf_state_key_setup(pd, pd->src, pd->dst, sport, dport);
+		sk = pf_state_key_setup(pd, m, off, pd->src, pd->dst, sport, dport);
 		if (sk == NULL)
 			goto csfailed;
 		nk = sk;
@@ -6655,9 +6719,9 @@ pf_multihome_scan_asconf(struct mbuf *m, int start, int len,
 
 int
 pf_icmp_state_lookup(struct pf_state_key_cmp *key, struct pf_pdesc *pd,
-    struct pf_kstate **state, struct mbuf *m, int direction, struct pfi_kkif *kif,
-    u_int16_t icmpid, u_int16_t type, int icmp_dir, int *iidx, int multi,
-    int inner)
+    struct pf_kstate **state, struct mbuf *m, int off, int direction,
+    struct pfi_kkif *kif, u_int16_t icmpid, u_int16_t type, int icmp_dir,
+    int *iidx, int multi, int inner)
 {
 	key->af = pd->af;
 	key->proto = pd->proto;
@@ -6670,25 +6734,9 @@ pf_icmp_state_lookup(struct pf_state_key_cmp *key, struct pf_pdesc *pd,
 		key->port[pd->sidx] = type;
 		key->port[pd->didx] = icmpid;
 	}
-	if (pd->af == AF_INET6 && multi != PF_ICMP_MULTI_NONE) {
-		switch (multi) {
-		case PF_ICMP_MULTI_SOLICITED:
-			key->addr[pd->sidx].addr32[0] = IPV6_ADDR_INT32_MLL;
-			key->addr[pd->sidx].addr32[1] = 0;
-			key->addr[pd->sidx].addr32[2] = IPV6_ADDR_INT32_ONE;
-			key->addr[pd->sidx].addr32[3] = pd->src->addr32[3];
-			key->addr[pd->sidx].addr8[12] = 0xff;
-			break;
-		case PF_ICMP_MULTI_LINK:
-			key->addr[pd->sidx].addr32[0] = IPV6_ADDR_INT32_MLL;
-			key->addr[pd->sidx].addr32[1] = 0;
-			key->addr[pd->sidx].addr32[2] = 0;
-			key->addr[pd->sidx].addr32[3] = IPV6_ADDR_INT32_ONE;
-			break;
-		}
-	} else
-		PF_ACPY(&key->addr[pd->sidx], pd->src, key->af);
-	PF_ACPY(&key->addr[pd->didx], pd->dst, key->af);
+	if (pf_state_key_addr_setup(pd, m, off, key, pd->sidx, pd->src,
+	    pd->didx, pd->dst, multi))
+		return (PF_DROP);
 
 	STATE_LOOKUP(kif, key, *state, pd);
 
@@ -6751,7 +6799,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif,
 		 * ICMP query/reply message not related to a TCP/UDP packet.
 		 * Search for an ICMP state.
 		 */
-		ret = pf_icmp_state_lookup(&key, pd, state, m, pd->dir,
+		ret = pf_icmp_state_lookup(&key, pd, state, m, off, pd->dir,
 		    kif, virtual_id, virtual_type, icmp_dir, &iidx,
 		    PF_ICMP_MULTI_NONE, 0);
 		if (ret >= 0) {
@@ -6759,7 +6807,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif,
 			    icmp_dir == PF_OUT) {
 				if (*state != NULL)
 					PF_STATE_UNLOCK((*state));
-				ret = pf_icmp_state_lookup(&key, pd, state, m,
+				ret = pf_icmp_state_lookup(&key, pd, state, m, off,
 				    pd->dir, kif, virtual_id, virtual_type,
 				    icmp_dir, &iidx, multi, 0);
 				if (ret >= 0)
@@ -7167,7 +7215,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif,
 			pf_icmp_mapping(&pd2, iih->icmp_type,
 			    &icmp_dir, &multi, &virtual_id, &virtual_type);
 
-			ret = pf_icmp_state_lookup(&key, &pd2, state, m,
+			ret = pf_icmp_state_lookup(&key, &pd2, state, m, off,
 			    pd2.dir, kif, virtual_id, virtual_type,
 			    icmp_dir, &iidx, PF_ICMP_MULTI_NONE, 1);
 			if (ret >= 0)
@@ -7222,7 +7270,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif,
 			pf_icmp_mapping(&pd2, iih->icmp6_type,
 			    &icmp_dir, &multi, &virtual_id, &virtual_type);
 
-			ret = pf_icmp_state_lookup(&key, &pd2, state, m,
+			ret = pf_icmp_state_lookup(&key, &pd2, state, m, off,
 			    pd->dir, kif, virtual_id, virtual_type,
 			    icmp_dir, &iidx, PF_ICMP_MULTI_NONE, 1);
 			if (ret >= 0) {
@@ -7231,7 +7279,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif,
 					if (*state != NULL)
 						PF_STATE_UNLOCK((*state));
 					ret = pf_icmp_state_lookup(&key, pd,
-					    state, m, pd->dir, kif,
+					    state, m, off, pd->dir, kif,
 					    virtual_id, virtual_type,
 					    icmp_dir, &iidx, multi, 1);
 					if (ret >= 0)
diff --git a/sys/netpfil/pf/pf_lb.c b/sys/netpfil/pf/pf_lb.c
index 68fc76233dab..6b0b95e9ce01 100644
--- a/sys/netpfil/pf/pf_lb.c
+++ b/sys/netpfil/pf/pf_lb.c
@@ -637,7 +637,7 @@ pf_get_translation(struct pf_pdesc *pd, struct mbuf *m, int off,
 		return (PFRES_MAX);
 	}
 
-	*skp = pf_state_key_setup(pd, saddr, daddr, sport, dport);
+	*skp = pf_state_key_setup(pd, m, off, saddr, daddr, sport, dport);
 	if (*skp == NULL)
 		return (PFRES_MEMORY);
 	*nkp = pf_state_key_clone(*skp);