diff options
| author | Kristof Provost <kp@FreeBSD.org> | 2024-08-30 11:36:39 +0000 |
|---|---|---|
| committer | Mark Johnston <markj@FreeBSD.org> | 2024-09-19 12:58:55 +0000 |
| commit | 413ae023b0563b7d51ab7aa1d69900886db66497 (patch) | |
| tree | 410382017d647c31e604c120b1aa719fc0083cbe | |
| parent | e854c92f30aa96a8a3a7f8edd8be9e5ba8a20deb (diff) | |
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)
| -rw-r--r-- | sys/net/pfvar.h | 4 | ||||
| -rw-r--r-- | 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); } |