Date: Wed, 10 Sep 2025 19:52:22 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: 94804658ab04 - main - pf: Remove dead code in pf_pull_hdr(). Message-ID: <202509101952.58AJqM0u095229@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=94804658ab045fd386c2f031186c86f686c6870a commit 94804658ab045fd386c2f031186c86f686c6870a Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2025-08-19 11:16:51 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2025-09-10 19:51:39 +0000 pf: Remove dead code in pf_pull_hdr(). pf_pull_hdr() allows to pass an action pointer parameter as output value. This is never used, all callers pass a NULL argument. Remove ACTION_SET() entirely. The logic (fragoff >= len) in pf_pull_hdr() does not work since revision 1.4. Before it was used to drop short TCP or UDP fragments that contained only part of the header. Current code in pf_pull_hdr() drops the packets anyway, so always set reason PFRES_FRAG. OK kn@ sashan@ Obtained from: OpenBSD, bluhm <bluhm@openbsd.org>, 46650f23db Sponsored by: Rubicon Communications, LLC ("Netgate") --- sys/net/pfvar.h | 2 +- sys/netpfil/pf/pf.c | 74 ++++++++++++++++++++++-------------------------- sys/netpfil/pf/pf_norm.c | 12 ++++---- sys/netpfil/pf/pf_osfp.c | 2 +- 4 files changed, 42 insertions(+), 48 deletions(-) diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h index f73420494000..e6fb1c2c3e1b 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -2423,7 +2423,7 @@ int pf_multihome_scan_init(int, int, struct pf_pdesc *); int pf_multihome_scan_asconf(int, int, struct pf_pdesc *); u_int32_t pf_new_isn(struct pf_kstate *); -void *pf_pull_hdr(const struct mbuf *, int, void *, int, u_short *, u_short *, +void *pf_pull_hdr(const struct mbuf *, int, void *, int, u_short *, sa_family_t); void pf_change_a(void *, u_int16_t *, u_int32_t, u_int8_t); void pf_change_proto_a(struct mbuf *, void *, u_int16_t *, u_int32_t, diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index 3a047ea44c47..5889bb9d68e6 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -1676,7 +1676,7 @@ pf_state_key_addr_setup(struct pf_pdesc *pd, if (multi) return (-1); if (!pf_pull_hdr(pd->m, pd->off, &nd, sizeof(nd), NULL, - NULL, pd->af)) + pd->af)) return (-1); target = (struct pf_addr *)&nd.nd_ns_target; daddr = target; @@ -1685,7 +1685,7 @@ pf_state_key_addr_setup(struct pf_pdesc *pd, if (multi) return (-1); if (!pf_pull_hdr(pd->m, pd->off, &nd, sizeof(nd), NULL, - NULL, pd->af)) + pd->af)) return (-1); target = (struct pf_addr *)&nd.nd_ns_target; saddr = target; @@ -4044,7 +4044,7 @@ pf_modulate_sack(struct pf_pdesc *pd, struct tcphdr *th, optsoff = pd->off + sizeof(struct tcphdr); #define TCPOLEN_MINSACK (TCPOLEN_SACK + 2) if (olen < TCPOLEN_MINSACK || - !pf_pull_hdr(pd->m, optsoff, opts, olen, NULL, NULL, pd->af)) + !pf_pull_hdr(pd->m, optsoff, opts, olen, NULL, pd->af)) return (0); eoh = opts + olen; @@ -5107,7 +5107,7 @@ pf_get_wscale(struct pf_pdesc *pd) olen = (pd->hdr.tcp.th_off << 2) - sizeof(struct tcphdr); if (olen < TCPOLEN_WINDOW || !pf_pull_hdr(pd->m, - pd->off + sizeof(struct tcphdr), opts, olen, NULL, NULL, pd->af)) + pd->off + sizeof(struct tcphdr), opts, olen, NULL, pd->af)) return (0); opt = opts; @@ -5132,7 +5132,7 @@ pf_get_mss(struct pf_pdesc *pd) olen = (pd->hdr.tcp.th_off << 2) - sizeof(struct tcphdr); if (olen < TCPOLEN_MAXSEG || !pf_pull_hdr(pd->m, - pd->off + sizeof(struct tcphdr), opts, olen, NULL, NULL, pd->af)) + pd->off + sizeof(struct tcphdr), opts, olen, NULL, pd->af)) return (0); opt = opts; @@ -7660,7 +7660,7 @@ pf_multihome_scan(int start, int len, struct pf_pdesc *pd, int op) while (off < len) { struct sctp_paramhdr h; - if (!pf_pull_hdr(pd->m, start + off, &h, sizeof(h), NULL, NULL, + if (!pf_pull_hdr(pd->m, start + off, &h, sizeof(h), NULL, pd->af)) return (PF_DROP); @@ -7680,7 +7680,7 @@ pf_multihome_scan(int start, int len, struct pf_pdesc *pd, int op) return (PF_DROP); if (!pf_pull_hdr(pd->m, start + off + sizeof(h), &t, sizeof(t), - NULL, NULL, pd->af)) + NULL, pd->af)) return (PF_DROP); if (in_nullhost(t)) @@ -7724,7 +7724,7 @@ pf_multihome_scan(int start, int len, struct pf_pdesc *pd, int op) return (PF_DROP); if (!pf_pull_hdr(pd->m, start + off + sizeof(h), &t, sizeof(t), - NULL, NULL, pd->af)) + NULL, pd->af)) return (PF_DROP); if (memcmp(&t, &pd->src->v6, sizeof(t)) == 0) break; @@ -7754,7 +7754,7 @@ pf_multihome_scan(int start, int len, struct pf_pdesc *pd, int op) struct sctp_asconf_paramhdr ah; if (!pf_pull_hdr(pd->m, start + off, &ah, sizeof(ah), - NULL, NULL, pd->af)) + NULL, pd->af)) return (PF_DROP); ret = pf_multihome_scan(start + off + sizeof(ah), @@ -7769,7 +7769,7 @@ pf_multihome_scan(int start, int len, struct pf_pdesc *pd, int op) struct sctp_asconf_paramhdr ah; if (!pf_pull_hdr(pd->m, start + off, &ah, sizeof(ah), - NULL, NULL, pd->af)) + NULL, pd->af)) return (PF_DROP); ret = pf_multihome_scan(start + off + sizeof(ah), ntohs(ah.ph.param_length) - sizeof(ah), pd, @@ -8051,7 +8051,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pf_pdesc *pd, ipoff2 = pd->off + ICMP_MINLEN; if (!pf_pull_hdr(pd->m, ipoff2, &h2, sizeof(h2), - NULL, reason, pd2.af)) { + reason, pd2.af)) { DPFPRINTF(PF_DEBUG_MISC, "pf: ICMP error message too short " "(ip)"); @@ -8083,7 +8083,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pf_pdesc *pd, ipoff2 = pd->off + sizeof(struct icmp6_hdr); if (!pf_pull_hdr(pd->m, ipoff2, &h2_6, sizeof(h2_6), - NULL, reason, pd2.af)) { + reason, pd2.af)) { DPFPRINTF(PF_DEBUG_MISC, "pf: ICMP error message too short " "(ip6)"); @@ -8136,7 +8136,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pf_pdesc *pd, * expected. Don't access any TCP header fields after * th_seq, an ackskew test is not possible. */ - if (!pf_pull_hdr(pd->m, pd2.off, th, 8, NULL, reason, + if (!pf_pull_hdr(pd->m, pd2.off, th, 8, reason, pd2.af)) { DPFPRINTF(PF_DEBUG_MISC, "pf: ICMP error message too short " @@ -8332,7 +8332,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pf_pdesc *pd, int action; if (!pf_pull_hdr(pd->m, pd2.off, uh, sizeof(*uh), - NULL, reason, pd2.af)) { + reason, pd2.af)) { DPFPRINTF(PF_DEBUG_MISC, "pf: ICMP error message too short " "(udp)"); @@ -8463,7 +8463,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pf_pdesc *pd, int copyback = 0; int action; - if (! pf_pull_hdr(pd->m, pd2.off, sh, sizeof(*sh), NULL, reason, + if (! pf_pull_hdr(pd->m, pd2.off, sh, sizeof(*sh), reason, pd2.af)) { DPFPRINTF(PF_DEBUG_MISC, "pf: ICMP error message too short " @@ -8619,7 +8619,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pf_pdesc *pd, } if (!pf_pull_hdr(pd->m, pd2.off, iih, ICMP_MINLEN, - NULL, reason, pd2.af)) { + reason, pd2.af)) { DPFPRINTF(PF_DEBUG_MISC, "pf: ICMP error message too short i" "(icmp)"); @@ -8739,7 +8739,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pf_pdesc *pd, } if (!pf_pull_hdr(pd->m, pd2.off, iih, - sizeof(struct icmp6_hdr), NULL, reason, pd2.af)) { + sizeof(struct icmp6_hdr), reason, pd2.af)) { DPFPRINTF(PF_DEBUG_MISC, "pf: ICMP error message too short " "(icmp6)"); @@ -8921,7 +8921,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pf_pdesc *pd, */ void * pf_pull_hdr(const struct mbuf *m, int off, void *p, int len, - u_short *actionp, u_short *reasonp, sa_family_t af) + u_short *reasonp, sa_family_t af) { int iplen = 0; switch (af) { @@ -8931,12 +8931,7 @@ pf_pull_hdr(const struct mbuf *m, int off, void *p, int len, u_int16_t fragoff = (ntohs(h->ip_off) & IP_OFFMASK) << 3; if (fragoff) { - if (fragoff >= len) - ACTION_SET(actionp, PF_PASS); - else { - ACTION_SET(actionp, PF_DROP); - REASON_SET(reasonp, PFRES_FRAG); - } + REASON_SET(reasonp, PFRES_FRAG); return (NULL); } iplen = ntohs(h->ip_len); @@ -8953,7 +8948,6 @@ pf_pull_hdr(const struct mbuf *m, int off, void *p, int len, #endif /* INET6 */ } if (m->m_pkthdr.len < off + len || iplen < off + len) { - ACTION_SET(actionp, PF_DROP); REASON_SET(reasonp, PFRES_SHORT); return (NULL); } @@ -10052,7 +10046,7 @@ pf_walk_header(struct pf_pdesc *pd, struct ip *h, u_short *reason) end < pd->off + sizeof(ext)) return (PF_PASS); if (!pf_pull_hdr(pd->m, pd->off, &ext, sizeof(ext), - NULL, reason, AF_INET)) { + reason, AF_INET)) { DPFPRINTF(PF_DEBUG_MISC, "IP short exthdr"); return (PF_DROP); } @@ -10078,7 +10072,7 @@ pf_walk_option6(struct pf_pdesc *pd, struct ip6_hdr *h, int off, int end, while (off < end) { if (!pf_pull_hdr(pd->m, off, &opt.ip6o_type, - sizeof(opt.ip6o_type), NULL, reason, AF_INET6)) { + sizeof(opt.ip6o_type), reason, AF_INET6)) { DPFPRINTF(PF_DEBUG_MISC, "IPv6 short opt type"); return (PF_DROP); } @@ -10086,7 +10080,7 @@ pf_walk_option6(struct pf_pdesc *pd, struct ip6_hdr *h, int off, int end, off++; continue; } - if (!pf_pull_hdr(pd->m, off, &opt, sizeof(opt), NULL, + if (!pf_pull_hdr(pd->m, off, &opt, sizeof(opt), reason, AF_INET6)) { DPFPRINTF(PF_DEBUG_MISC, "IPv6 short opt"); return (PF_DROP); @@ -10111,7 +10105,7 @@ pf_walk_option6(struct pf_pdesc *pd, struct ip6_hdr *h, int off, int end, REASON_SET(reason, PFRES_IPOPTIONS); return (PF_DROP); } - if (!pf_pull_hdr(pd->m, off, &jumbo, sizeof(jumbo), NULL, + if (!pf_pull_hdr(pd->m, off, &jumbo, sizeof(jumbo), reason, AF_INET6)) { DPFPRINTF(PF_DEBUG_MISC, "IPv6 short jumbo"); return (PF_DROP); @@ -10160,7 +10154,7 @@ pf_walk_header6(struct pf_pdesc *pd, struct ip6_hdr *h, u_short *reason) break; case IPPROTO_HOPOPTS: if (!pf_pull_hdr(pd->m, pd->off, &ext, sizeof(ext), - NULL, reason, AF_INET6)) { + reason, AF_INET6)) { DPFPRINTF(PF_DEBUG_MISC, "IPv6 short exthdr"); return (PF_DROP); } @@ -10187,7 +10181,7 @@ pf_walk_header6(struct pf_pdesc *pd, struct ip6_hdr *h, u_short *reason) return (PF_DROP); } if (!pf_pull_hdr(pd->m, pd->off, &frag, sizeof(frag), - NULL, reason, AF_INET6)) { + reason, AF_INET6)) { DPFPRINTF(PF_DEBUG_MISC, "IPv6 short fragment"); return (PF_DROP); } @@ -10215,7 +10209,7 @@ pf_walk_header6(struct pf_pdesc *pd, struct ip6_hdr *h, u_short *reason) return (PF_PASS); } if (!pf_pull_hdr(pd->m, pd->off, &rthdr, sizeof(rthdr), - NULL, reason, AF_INET6)) { + reason, AF_INET6)) { DPFPRINTF(PF_DEBUG_MISC, "IPv6 short rthdr"); return (PF_DROP); } @@ -10236,7 +10230,7 @@ pf_walk_header6(struct pf_pdesc *pd, struct ip6_hdr *h, u_short *reason) case IPPROTO_AH: case IPPROTO_DSTOPTS: if (!pf_pull_hdr(pd->m, pd->off, &ext, sizeof(ext), - NULL, reason, AF_INET6)) { + reason, AF_INET6)) { DPFPRINTF(PF_DEBUG_MISC, "IPv6 short exthdr"); return (PF_DROP); } @@ -10269,7 +10263,7 @@ pf_walk_header6(struct pf_pdesc *pd, struct ip6_hdr *h, u_short *reason) return (PF_PASS); } if (!pf_pull_hdr(pd->m, pd->off, &icmp6, sizeof(icmp6), - NULL, reason, AF_INET6)) { + reason, AF_INET6)) { DPFPRINTF(PF_DEBUG_MISC, "IPv6 short icmp6hdr"); return (PF_DROP); @@ -10502,7 +10496,7 @@ pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf **m0, case IPPROTO_TCP: { struct tcphdr *th = &pd->hdr.tcp; - if (!pf_pull_hdr(pd->m, pd->off, th, sizeof(*th), action, + if (!pf_pull_hdr(pd->m, pd->off, th, sizeof(*th), reason, af)) { *action = PF_DROP; REASON_SET(reason, PFRES_SHORT); @@ -10518,7 +10512,7 @@ pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf **m0, case IPPROTO_UDP: { struct udphdr *uh = &pd->hdr.udp; - if (!pf_pull_hdr(pd->m, pd->off, uh, sizeof(*uh), action, + if (!pf_pull_hdr(pd->m, pd->off, uh, sizeof(*uh), reason, af)) { *action = PF_DROP; REASON_SET(reason, PFRES_SHORT); @@ -10539,7 +10533,7 @@ pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf **m0, } case IPPROTO_SCTP: { if (!pf_pull_hdr(pd->m, pd->off, &pd->hdr.sctp, sizeof(pd->hdr.sctp), - action, reason, af)) { + reason, af)) { *action = PF_DROP; REASON_SET(reason, PFRES_SHORT); return (-1); @@ -10569,7 +10563,7 @@ pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf **m0, } case IPPROTO_ICMP: { if (!pf_pull_hdr(pd->m, pd->off, &pd->hdr.icmp, ICMP_MINLEN, - action, reason, af)) { + reason, af)) { *action = PF_DROP; REASON_SET(reason, PFRES_SHORT); return (-1); @@ -10583,7 +10577,7 @@ pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf **m0, size_t icmp_hlen = sizeof(struct icmp6_hdr); if (!pf_pull_hdr(pd->m, pd->off, &pd->hdr.icmp6, icmp_hlen, - action, reason, af)) { + reason, af)) { *action = PF_DROP; REASON_SET(reason, PFRES_SHORT); return (-1); @@ -10609,7 +10603,7 @@ pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf **m0, } if (icmp_hlen > sizeof(struct icmp6_hdr) && !pf_pull_hdr(pd->m, pd->off, &pd->hdr.icmp6, icmp_hlen, - action, reason, af)) { + reason, af)) { *action = PF_DROP; REASON_SET(reason, PFRES_SHORT); return (-1); diff --git a/sys/netpfil/pf/pf_norm.c b/sys/netpfil/pf/pf_norm.c index a684d778ab42..56074bedbc40 100644 --- a/sys/netpfil/pf/pf_norm.c +++ b/sys/netpfil/pf/pf_norm.c @@ -1354,7 +1354,7 @@ pf_normalize_ip6(int off, u_short *reason, pf_rule_to_actions(r, &pd->act); } - if (!pf_pull_hdr(pd->m, off, &frag, sizeof(frag), NULL, reason, AF_INET6)) + if (!pf_pull_hdr(pd->m, off, &frag, sizeof(frag), reason, AF_INET6)) return (PF_DROP); /* Offset now points to data portion. */ @@ -1542,7 +1542,7 @@ pf_normalize_tcp_init(struct pf_pdesc *pd, struct tcphdr *th, olen = (th->th_off << 2) - sizeof(*th); if (olen < TCPOLEN_TIMESTAMP || !pf_pull_hdr(pd->m, - pd->off + sizeof(*th), opts, olen, NULL, NULL, pd->af)) + pd->off + sizeof(*th), opts, olen, NULL, pd->af)) return (0); opt = opts; @@ -1645,7 +1645,7 @@ pf_normalize_tcp_stateful(struct pf_pdesc *pd, if (olen >= TCPOLEN_TIMESTAMP && ((src->scrub && (src->scrub->pfss_flags & PFSS_TIMESTAMP)) || (dst->scrub && (dst->scrub->pfss_flags & PFSS_TIMESTAMP))) && - pf_pull_hdr(pd->m, pd->off + sizeof(*th), opts, olen, NULL, NULL, pd->af)) { + pf_pull_hdr(pd->m, pd->off + sizeof(*th), opts, olen, NULL, pd->af)) { /* Modulate the timestamps. Can be used for NAT detection, OS * uptime determination or reboot detection. */ @@ -1975,7 +1975,7 @@ pf_normalize_mss(struct pf_pdesc *pd) olen = (pd->hdr.tcp.th_off << 2) - sizeof(struct tcphdr); optsoff = pd->off + sizeof(struct tcphdr); if (olen < TCPOLEN_MAXSEG || - !pf_pull_hdr(pd->m, optsoff, opts, olen, NULL, NULL, pd->af)) + !pf_pull_hdr(pd->m, optsoff, opts, olen, NULL, pd->af)) return (0); opt = opts; @@ -2009,7 +2009,7 @@ pf_scan_sctp(struct pf_pdesc *pd) int ret; while (pd->off + chunk_off < pd->tot_len) { - if (!pf_pull_hdr(pd->m, pd->off + chunk_off, &ch, sizeof(ch), NULL, + if (!pf_pull_hdr(pd->m, pd->off + chunk_off, &ch, sizeof(ch), NULL, pd->af)) return (PF_DROP); @@ -2026,7 +2026,7 @@ pf_scan_sctp(struct pf_pdesc *pd) struct sctp_init_chunk init; if (!pf_pull_hdr(pd->m, pd->off + chunk_start, &init, - sizeof(init), NULL, NULL, pd->af)) + sizeof(init), NULL, pd->af)) return (PF_DROP); /* diff --git a/sys/netpfil/pf/pf_osfp.c b/sys/netpfil/pf/pf_osfp.c index 150626c5f3fb..8c041d45eae8 100644 --- a/sys/netpfil/pf/pf_osfp.c +++ b/sys/netpfil/pf/pf_osfp.c @@ -82,7 +82,7 @@ pf_osfp_fingerprint(struct pf_pdesc *pd, const struct tcphdr *tcp) ip6 = mtod(pd->m, struct ip6_hdr *); break; } - if (!pf_pull_hdr(pd->m, pd->off, hdr, tcp->th_off << 2, NULL, NULL, + if (!pf_pull_hdr(pd->m, pd->off, hdr, tcp->th_off << 2, NULL, pd->af)) return (NULL); return (pf_osfp_fingerprint_hdr(ip, ip6, (struct tcphdr *)hdr));
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202509101952.58AJqM0u095229>