Date: Fri, 17 Jan 2025 16:02:12 GMT From: Kristof Provost <kp@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 5d28f4cab8d5 - main - pf: clean up mbuf passing for reassembly Message-ID: <202501171602.50HG2CYU065695@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by kp: URL: https://cgit.FreeBSD.org/src/commit/?id=5d28f4cab8d5919aba1365e885a91a96c0655b59 commit 5d28f4cab8d5919aba1365e885a91a96c0655b59 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2025-01-06 06:56:24 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2025-01-17 16:00:08 +0000 pf: clean up mbuf passing for reassembly When we call pf_normalize_ip() or pf_normalize_ip6() we passed the mbuf twice. Once as m0, and once inside the struct pf_pdesc. Remove the former to avoid confusion when we free *m0, but don't update pd->m. This could lead to use-after-free errors e.g. if reassembly failed. PR: 283705 Reported by: Yichen Chai <yichen.chai@gmail.com>, Zhuo Ying Jiang Li <zyj20@cl.cam.ac.uk> MFC after: 3 days Sponsored by: Rubicon Communications, LLC ("Netgate") --- sys/net/pfvar.h | 4 ++-- sys/netpfil/pf/pf.c | 10 ++++++---- sys/netpfil/pf/pf_norm.c | 17 +++++------------ 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h index 6c9d75bb1365..6e9418f59aef 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -2390,11 +2390,11 @@ int pf_test(sa_family_t, int, int, struct ifnet *, struct mbuf **, struct inpcb struct pf_rule_actions *); #endif #ifdef INET -int pf_normalize_ip(struct mbuf **, u_short *, struct pf_pdesc *); +int pf_normalize_ip(u_short *, struct pf_pdesc *); #endif /* INET */ #ifdef INET6 -int pf_normalize_ip6(struct mbuf **, int, u_short *, struct pf_pdesc *); +int pf_normalize_ip6(int, u_short *, struct pf_pdesc *); void pf_poolmask(struct pf_addr *, struct pf_addr*, struct pf_addr *, struct pf_addr *, sa_family_t); void pf_addr_inc(struct pf_addr *, sa_family_t); diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index 07b7aa543dbd..3b9c71296a2b 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -9918,12 +9918,13 @@ pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf **m0, return (-1); } - if (pf_normalize_ip(m0, reason, pd) != PF_PASS) { + if (pf_normalize_ip(reason, pd) != PF_PASS) { /* We do IP header normalization and packet reassembly here */ + *m0 = pd->m; *action = PF_DROP; return (-1); } - pd->m = *m0; + *m0 = pd->m; h = mtod(pd->m, struct ip *); pd->off = h->ip_hl << 2; @@ -9994,12 +9995,13 @@ pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf **m0, } /* We do IP header normalization and packet reassembly here */ - if (pf_normalize_ip6(m0, pd->fragoff, reason, pd) != + if (pf_normalize_ip6(pd->fragoff, reason, pd) != PF_PASS) { + *m0 = pd->m; *action = PF_DROP; return (-1); } - pd->m = *m0; + *m0 = pd->m; if (pd->m == NULL) { /* packet sits in reassembly queue, no error */ *action = PF_PASS; diff --git a/sys/netpfil/pf/pf_norm.c b/sys/netpfil/pf/pf_norm.c index c9a7f7d2df04..39afde04a6d1 100644 --- a/sys/netpfil/pf/pf_norm.c +++ b/sys/netpfil/pf/pf_norm.c @@ -1093,11 +1093,10 @@ pf_refragment6(struct ifnet *ifp, struct mbuf **m0, struct m_tag *mtag, #ifdef INET int -pf_normalize_ip(struct mbuf **m0, u_short *reason, - struct pf_pdesc *pd) +pf_normalize_ip(u_short *reason, struct pf_pdesc *pd) { struct pf_krule *r; - struct ip *h = mtod(*m0, struct ip *); + struct ip *h = mtod(pd->m, struct ip *); int mff = (ntohs(h->ip_off) & IP_MF); int hlen = h->ip_hl << 2; u_int16_t fragoff = (ntohs(h->ip_off) & IP_OFFMASK) << 3; @@ -1109,8 +1108,6 @@ pf_normalize_ip(struct mbuf **m0, u_short *reason, PF_RULES_RASSERT(); - MPASS(pd->m == *m0); - r = TAILQ_FIRST(pf_main_ruleset.rules[PF_RULESET_SCRUB].active.ptr); /* * Check if there are any scrub rules, matching or not. @@ -1219,13 +1216,12 @@ pf_normalize_ip(struct mbuf **m0, u_short *reason, * Might return a completely reassembled mbuf, or NULL */ PF_FRAG_LOCK(); DPFPRINTF(("reass frag %d @ %d-%d\n", h->ip_id, fragoff, max)); - verdict = pf_reassemble(m0, pd->dir, reason); + verdict = pf_reassemble(&pd->m, pd->dir, reason); PF_FRAG_UNLOCK(); if (verdict != PF_PASS) return (PF_DROP); - pd->m = *m0; if (pd->m == NULL) return (PF_DROP); @@ -1257,7 +1253,7 @@ pf_normalize_ip(struct mbuf **m0, u_short *reason, #ifdef INET6 int -pf_normalize_ip6(struct mbuf **m0, int off, u_short *reason, +pf_normalize_ip6(int off, u_short *reason, struct pf_pdesc *pd) { struct pf_krule *r; @@ -1267,8 +1263,6 @@ pf_normalize_ip6(struct mbuf **m0, int off, u_short *reason, PF_RULES_RASSERT(); - pd->m = *m0; - r = TAILQ_FIRST(pf_main_ruleset.rules[PF_RULESET_SCRUB].active.ptr); /* * Check if there are any scrub rules, matching or not. @@ -1323,9 +1317,8 @@ pf_normalize_ip6(struct mbuf **m0, int off, u_short *reason, if (pd->virtual_proto == PF_VPROTO_FRAGMENT) { /* Returns PF_DROP or *m0 is NULL or completely reassembled * mbuf. */ - if (pf_reassemble6(m0, &frag, off, pd->extoff, reason) != PF_PASS) + if (pf_reassemble6(&pd->m, &frag, off, pd->extoff, reason) != PF_PASS) return (PF_DROP); - pd->m = *m0; if (pd->m == NULL) return (PF_DROP); h = mtod(pd->m, struct ip6_hdr *);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202501171602.50HG2CYU065695>