git: ea9257bcd0e1 - releng/13.3 - pf: rework pf_icmp_state_lookup() failure mode

The branch releng/13.3 has been updated by markj:

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

commit ea9257bcd0e1ae178fa4266017bd1db7dae4e780
Author:     Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2024-08-30 11:36:39 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2024-09-19 13:01:45 +0000

    pf: rework pf_icmp_state_lookup() failure mode
    
    If pf_icmp_state_lookup() finds a state but rejects it for not matching the
    expected direction we should unlock the state (and NULL out *state). This
    simplifies life for callers, and also ensures there's no confusion about what a
    non-NULL returned state means.
    
    Previously it could have been left in there by the caller, resulting in callers
    unlocking the same state twice.
    
    Approved by:    so
    Security:       FreeBSD-EN-24:16.pf
    MFC after:      1 week
    Sponsored by:   Rubicon Communications, LLC ("Netgate")
    
    (cherry picked from commit 0578fe492284ded4745167060be794032e6e22f0)
    (cherry picked from commit d6e5f8643d37e925aa51fc8224cfc05aba0813f7)
---
 sys/net/pfvar.h     |  4 ++--
 sys/netpfil/pf/pf.c | 20 +++++++++++++-------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index b3b8074cbe17..0f2ad39fec94 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -330,8 +330,8 @@ struct pfi_dynaddr {
 		mtx_unlock(_s->lock);					\
 	} while (0)
 #else
-#define	PF_STATE_LOCK(s)	mtx_lock(s->lock)
-#define	PF_STATE_UNLOCK(s)	mtx_unlock(s->lock)
+#define	PF_STATE_LOCK(s)	mtx_lock((s)->lock)
+#define	PF_STATE_UNLOCK(s)	mtx_unlock((s)->lock)
 #endif
 
 #ifdef INVARIANTS
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index 100302ab2ca5..27918a6fd909 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -6089,6 +6089,8 @@ pf_icmp_state_lookup(struct pf_state_key_cmp *key, struct pf_pdesc *pd,
 			pf_print_state(*state);
 			printf("\n");
 		}
+		PF_STATE_UNLOCK(*state);
+		*state = NULL;
 		return (PF_DROP);
 	}
 	return (-1);
@@ -6141,15 +6143,16 @@ pf_test_state_icmp(struct pf_kstate **state, int direction, struct pfi_kkif *kif
 		    kif, virtual_id, virtual_type, icmp_dir, &iidx,
 		    PF_ICMP_MULTI_NONE, 0);
 		if (ret >= 0) {
+			MPASS(*state == NULL);
 			if (ret == PF_DROP && pd->af == AF_INET6 &&
 			    icmp_dir == PF_OUT) {
-				if (*state != NULL)
-					PF_STATE_UNLOCK((*state));
 				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)
+				if (ret >= 0) {
+					MPASS(*state == NULL);
 					return (ret);
+				}
 			} else
 				return (ret);
 		}
@@ -6556,8 +6559,10 @@ pf_test_state_icmp(struct pf_kstate **state, int direction, struct pfi_kkif *kif
 			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)
+			if (ret >= 0) {
+				MPASS(*state == NULL);
 				return (ret);
+			}
 
 			/* translate source/destination address, if necessary */
 			if ((*state)->key[PF_SK_WIRE] !=
@@ -6612,16 +6617,17 @@ pf_test_state_icmp(struct pf_kstate **state, int direction, struct pfi_kkif *kif
 			    pd->dir, kif, virtual_id, virtual_type,
 			    icmp_dir, &iidx, PF_ICMP_MULTI_NONE, 1);
 			if (ret >= 0) {
+				MPASS(*state == NULL);
 				if (ret == PF_DROP && pd2.af == AF_INET6 &&
 				    icmp_dir == PF_OUT) {
-					if (*state != NULL)
-						PF_STATE_UNLOCK((*state));
 					ret = pf_icmp_state_lookup(&key, &pd2,
 					    state, m, off, pd->dir, kif,
 					    virtual_id, virtual_type,
 					    icmp_dir, &iidx, multi, 1);
-					if (ret >= 0)
+					if (ret >= 0) {
+						MPASS(*state == NULL);
 						return (ret);
+					}
 				} else
 					return (ret);
 			}