The branch releng/13.3 has been updated by markj:
URL:
https://cgit.FreeBSD.org/src/commit/?id=ea9257bcd0e1ae178fa4266017bd1db7dae4e780
commit ea9257bcd0e1ae178fa4266017bd1db7dae4e780
Author: Kristof Provost <k...@freebsd.org>
AuthorDate: 2024-08-30 11:36:39 +0000
Commit: Mark Johnston <ma...@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);
}
