git: 413ae023b056 - releng/14.0 - pf: rework pf_icmp_state_lookup() failure mode

The branch releng/14.0 has been updated by markj:

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

commit 413ae023b0563b7d51ab7aa1d69900886db66497
Author:     Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2024-08-30 11:36:39 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2024-09-19 12:58:55 +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 38f74de7184ac3ad7acc48055551aaa9ec9cded9)
---
 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 4edbf0c51ff8..f24103c25960 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -359,8 +359,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 5a7a6563d355..8ff0fbb3c4ae 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -6189,6 +6189,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);
@@ -6241,15 +6243,16 @@ pf_test_state_icmp(struct pf_kstate **state, 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);
 		}
@@ -6656,8 +6659,10 @@ pf_test_state_icmp(struct pf_kstate **state, 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] !=
@@ -6712,16 +6717,17 @@ pf_test_state_icmp(struct pf_kstate **state, 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);
 			}