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:55:55 +0000 |
| commit | 1e965d5399e18c42441c90778333a1a5f24a105c (patch) | |
| tree | f57df538c553a44b89921e0cd42aa2bccc9d695e | |
| parent | 94c6c986f3c4f2f9bef47b02659cfb2591d837cf (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 6a01f3eb6988..676cf05cab8d 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 ed278a5526be..7c31f1fc0fd0 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -6680,6 +6680,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); @@ -6732,15 +6734,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); } @@ -7147,8 +7150,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] != @@ -7203,16 +7208,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); } |