git: 6c92016aa685 - main - pf: fix a race against kif destruction in pf_test{,6}
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Tue, 31 May 2022 20:50:11 UTC
The branch main has been updated by mjg: URL: https://cgit.FreeBSD.org/src/commit/?id=6c92016aa685486f1445f632ac3f1af1385186af commit 6c92016aa685486f1445f632ac3f1af1385186af Author: Mateusz Guzik <mjg@FreeBSD.org> AuthorDate: 2022-05-31 20:00:37 +0000 Commit: Mateusz Guzik <mjg@FreeBSD.org> CommitDate: 2022-05-31 20:11:39 +0000 pf: fix a race against kif destruction in pf_test{,6} ifp kif was dereferenced prior to taking the lock and could have been nullified later. Reviewed by: kp Sponsored by: Rubicon Communications, LLC ("Netgate") Differential Revision: --- sys/netpfil/pf/pf.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index c613194ce9b5..56dab43a2810 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -6978,18 +6978,25 @@ pf_test(int dir, int pflags, struct ifnet *ifp, struct mbuf **m0, struct inpcb * if (!V_pf_status.running) return (PF_PASS); + PF_RULES_RLOCK(); + kif = (struct pfi_kkif *)ifp->if_pf_kif; - if (kif == NULL) { + if (__predict_false(kif == NULL)) { DPFPRINTF(PF_DEBUG_URGENT, ("pf_test: kif == NULL, if_xname %s\n", ifp->if_xname)); + PF_RULES_RUNLOCK(); return (PF_DROP); } - if (kif->pfik_flags & PFI_IFLAG_SKIP) + if (kif->pfik_flags & PFI_IFLAG_SKIP) { + PF_RULES_RUNLOCK(); return (PF_PASS); + } - if (m->m_flags & M_SKIP_FIREWALL) + if (m->m_flags & M_SKIP_FIREWALL) { + PF_RULES_RUNLOCK(); return (PF_PASS); + } memset(&pd, 0, sizeof(pd)); pd.pf_mtag = pf_find_mtag(m); @@ -7000,10 +7007,12 @@ pf_test(int dir, int pflags, struct ifnet *ifp, struct mbuf **m0, struct inpcb * ifp = ifnet_byindexgen(pd.pf_mtag->if_index, pd.pf_mtag->if_idxgen); if (ifp == NULL || ifp->if_flags & IFF_DYING) { + PF_RULES_RUNLOCK(); m_freem(*m0); *m0 = NULL; return (PF_PASS); } + PF_RULES_RUNLOCK(); (ifp->if_output)(ifp, m, sintosa(&pd.pf_mtag->dst), NULL); *m0 = NULL; return (PF_PASS); @@ -7023,12 +7032,11 @@ pf_test(int dir, int pflags, struct ifnet *ifp, struct mbuf **m0, struct inpcb * /* But only once. We may see the packet multiple times (e.g. * PFIL_IN/PFIL_OUT). */ pd.pf_mtag->flags &= ~PF_TAG_DUMMYNET; + PF_RULES_RUNLOCK(); return (PF_PASS); } - PF_RULES_RLOCK(); - if (__predict_false(ip_divert_ptr != NULL) && ((ipfwtag = m_tag_locate(m, MTAG_IPFW_RULE, 0, NULL)) != NULL)) { struct ipfw_rule_ref *rr = (struct ipfw_rule_ref *)(ipfwtag+1); @@ -7468,17 +7476,24 @@ pf_test6(int dir, int pflags, struct ifnet *ifp, struct mbuf **m0, struct inpcb if (!V_pf_status.running) return (PF_PASS); + PF_RULES_RLOCK(); + kif = (struct pfi_kkif *)ifp->if_pf_kif; - if (kif == NULL) { + if (__predict_false(kif == NULL)) { DPFPRINTF(PF_DEBUG_URGENT, ("pf_test6: kif == NULL, if_xname %s\n", ifp->if_xname)); + PF_RULES_RUNLOCK(); return (PF_DROP); } - if (kif->pfik_flags & PFI_IFLAG_SKIP) + if (kif->pfik_flags & PFI_IFLAG_SKIP) { + PF_RULES_RUNLOCK(); return (PF_PASS); + } - if (m->m_flags & M_SKIP_FIREWALL) + if (m->m_flags & M_SKIP_FIREWALL) { + PF_RULES_RUNLOCK(); return (PF_PASS); + } memset(&pd, 0, sizeof(pd)); pd.pf_mtag = pf_find_mtag(m); @@ -7489,10 +7504,12 @@ pf_test6(int dir, int pflags, struct ifnet *ifp, struct mbuf **m0, struct inpcb ifp = ifnet_byindexgen(pd.pf_mtag->if_index, pd.pf_mtag->if_idxgen); if (ifp == NULL || ifp->if_flags & IFF_DYING) { + PF_RULES_RUNLOCK(); m_freem(*m0); *m0 = NULL; return (PF_PASS); } + PF_RULES_RUNLOCK(); nd6_output_ifp(ifp, ifp, m, (struct sockaddr_in6 *)&pd.pf_mtag->dst, NULL); *m0 = NULL; @@ -7510,11 +7527,10 @@ pf_test6(int dir, int pflags, struct ifnet *ifp, struct mbuf **m0, struct inpcb /* Dummynet re-injects packets after they've * completed their delay. We've already * processed them, so pass unconditionally. */ + PF_RULES_RUNLOCK(); return (PF_PASS); } - PF_RULES_RLOCK(); - /* We do IP header normalization and packet reassembly here */ if (pf_normalize_ip6(m0, dir, kif, &reason, &pd) != PF_PASS) { action = PF_DROP;