From 46650f23db713388da0ecb54ba1bc00221ce6b2d Mon Sep 17 00:00:00 2001 From: bluhm Date: Tue, 10 Oct 2023 11:25:31 +0000 Subject: [PATCH] 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@ --- sys/net/pf.c | 58 +++++++++++++++++++++-------------------------- sys/net/pf_norm.c | 10 ++++---- sys/net/pf_osfp.c | 5 ++-- sys/net/pfvar.h | 11 ++------- 4 files changed, 35 insertions(+), 49 deletions(-) diff --git a/sys/net/pf.c b/sys/net/pf.c index ad2d73af28b..dfbac2bbb97 100644 --- a/sys/net/pf.c +++ b/sys/net/pf.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf.c,v 1.1186 2023/09/08 13:40:52 naddy Exp $ */ +/* $OpenBSD: pf.c,v 1.1187 2023/10/10 11:25:31 bluhm Exp $ */ /* * Copyright (c) 2001 Daniel Hartmeier @@ -3201,7 +3201,7 @@ pf_modulate_sack(struct pf_pdesc *pd, struct pf_state_peer *dst) 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; @@ -3871,7 +3871,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; @@ -3896,7 +3896,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; @@ -5691,7 +5691,7 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, ipoff2 = pd->off + ICMP_MINLEN; if (!pf_pull_hdr(pd2.m, ipoff2, &h2, sizeof(h2), - NULL, reason, pd2.af)) { + reason, pd2.af)) { DPFPRINTF(LOG_NOTICE, "ICMP error message too short (ip)"); return (PF_DROP); @@ -5719,7 +5719,7 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, ipoff2 = pd->off + sizeof(struct icmp6_hdr); if (!pf_pull_hdr(pd2.m, ipoff2, &h2_6, sizeof(h2_6), - NULL, reason, pd2.af)) { + reason, pd2.af)) { DPFPRINTF(LOG_NOTICE, "ICMP error message too short (ip6)"); return (PF_DROP); @@ -5770,7 +5770,7 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, * expected. Don't access any TCP header fields after * th_seq, an ackskew test is not possible. */ - if (!pf_pull_hdr(pd2.m, pd2.off, th, 8, NULL, reason, + if (!pf_pull_hdr(pd2.m, pd2.off, th, 8, reason, pd2.af)) { DPFPRINTF(LOG_NOTICE, "ICMP error message too short (tcp)"); @@ -5951,7 +5951,7 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, int action; if (!pf_pull_hdr(pd2.m, pd2.off, uh, sizeof(*uh), - NULL, reason, pd2.af)) { + reason, pd2.af)) { DPFPRINTF(LOG_NOTICE, "ICMP error message too short (udp)"); return (PF_DROP); @@ -6079,7 +6079,7 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, } if (!pf_pull_hdr(pd2.m, pd2.off, iih, ICMP_MINLEN, - NULL, reason, pd2.af)) { + reason, pd2.af)) { DPFPRINTF(LOG_NOTICE, "ICMP error message too short (icmp)"); return (PF_DROP); @@ -6185,7 +6185,7 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, } if (!pf_pull_hdr(pd2.m, pd2.off, iih, - sizeof(struct icmp6_hdr), NULL, reason, pd2.af)) { + sizeof(struct icmp6_hdr), reason, pd2.af)) { DPFPRINTF(LOG_NOTICE, "ICMP error message too short (icmp6)"); return (PF_DROP); @@ -6364,7 +6364,7 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp, */ void * pf_pull_hdr(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; @@ -6374,12 +6374,7 @@ pf_pull_hdr(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); @@ -6395,7 +6390,6 @@ pf_pull_hdr(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); } @@ -6949,7 +6943,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(LOG_NOTICE, "IP short exthdr"); return (PF_DROP); } @@ -6975,7 +6969,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(LOG_NOTICE, "IPv6 short opt type"); return (PF_DROP); } @@ -6984,7 +6978,7 @@ pf_walk_option6(struct pf_pdesc *pd, struct ip6_hdr *h, int off, int end, continue; } if (!pf_pull_hdr(pd->m, off, &opt, sizeof(opt), - NULL, reason, AF_INET6)) { + reason, AF_INET6)) { DPFPRINTF(LOG_NOTICE, "IPv6 short opt"); return (PF_DROP); } @@ -7009,7 +7003,7 @@ pf_walk_option6(struct pf_pdesc *pd, struct ip6_hdr *h, int off, int end, return (PF_DROP); } if (!pf_pull_hdr(pd->m, off, &jumbo, sizeof(jumbo), - NULL, reason, AF_INET6)) { + reason, AF_INET6)) { DPFPRINTF(LOG_NOTICE, "IPv6 short jumbo"); return (PF_DROP); } @@ -7058,7 +7052,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(LOG_NOTICE, "IPv6 short exthdr"); return (PF_DROP); } @@ -7085,7 +7079,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(LOG_NOTICE, "IPv6 short fragment"); return (PF_DROP); } @@ -7113,7 +7107,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(LOG_NOTICE, "IPv6 short rthdr"); return (PF_DROP); } @@ -7140,7 +7134,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, &ext, sizeof(ext), - NULL, reason, AF_INET6)) { + reason, AF_INET6)) { DPFPRINTF(LOG_NOTICE, "IPv6 short exthdr"); return (PF_DROP); } @@ -7167,7 +7161,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(LOG_NOTICE, "IPv6 short icmp6hdr"); return (PF_DROP); } @@ -7338,7 +7332,7 @@ pf_setup_pdesc(struct pf_pdesc *pd, sa_family_t af, int dir, struct tcphdr *th = &pd->hdr.tcp; if (!pf_pull_hdr(pd->m, pd->off, th, sizeof(*th), - NULL, reason, pd->af)) + reason, pd->af)) return (PF_DROP); pd->hdrlen = sizeof(*th); if (th->th_dport == 0 || @@ -7357,7 +7351,7 @@ pf_setup_pdesc(struct pf_pdesc *pd, sa_family_t af, int dir, struct udphdr *uh = &pd->hdr.udp; if (!pf_pull_hdr(pd->m, pd->off, uh, sizeof(*uh), - NULL, reason, pd->af)) + reason, pd->af)) return (PF_DROP); pd->hdrlen = sizeof(*uh); if (uh->uh_dport == 0 || @@ -7373,7 +7367,7 @@ pf_setup_pdesc(struct pf_pdesc *pd, sa_family_t af, int dir, } case IPPROTO_ICMP: { if (!pf_pull_hdr(pd->m, pd->off, &pd->hdr.icmp, ICMP_MINLEN, - NULL, reason, pd->af)) + reason, pd->af)) return (PF_DROP); pd->hdrlen = ICMP_MINLEN; if (pd->off + pd->hdrlen > pd->tot_len) { @@ -7388,7 +7382,7 @@ pf_setup_pdesc(struct pf_pdesc *pd, sa_family_t af, int dir, size_t icmp_hlen = sizeof(struct icmp6_hdr); if (!pf_pull_hdr(pd->m, pd->off, &pd->hdr.icmp6, icmp_hlen, - NULL, reason, pd->af)) + reason, pd->af)) return (PF_DROP); /* ICMP headers we look further into to match state */ switch (pd->hdr.icmp6.icmp6_type) { @@ -7411,7 +7405,7 @@ pf_setup_pdesc(struct pf_pdesc *pd, sa_family_t af, int dir, } if (icmp_hlen > sizeof(struct icmp6_hdr) && !pf_pull_hdr(pd->m, pd->off, &pd->hdr.icmp6, icmp_hlen, - NULL, reason, pd->af)) + reason, pd->af)) return (PF_DROP); pd->hdrlen = icmp_hlen; if (pd->off + pd->hdrlen > pd->tot_len) { diff --git a/sys/net/pf_norm.c b/sys/net/pf_norm.c index ef2c884918f..fde4a21f06b 100644 --- a/sys/net/pf_norm.c +++ b/sys/net/pf_norm.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf_norm.c,v 1.228 2023/07/06 04:55:05 dlg Exp $ */ +/* $OpenBSD: pf_norm.c,v 1.229 2023/10/10 11:25:31 bluhm Exp $ */ /* * Copyright 2001 Niels Provos @@ -1075,7 +1075,7 @@ pf_normalize_ip6(struct pf_pdesc *pd, u_short *reason) if (pd->fragoff == 0) goto no_fragment; - if (!pf_pull_hdr(pd->m, pd->fragoff, &frag, sizeof(frag), NULL, reason, + if (!pf_pull_hdr(pd->m, pd->fragoff, &frag, sizeof(frag), reason, AF_INET6)) return (PF_DROP); @@ -1216,7 +1216,7 @@ pf_normalize_tcp_init(struct pf_pdesc *pd, struct pf_state_peer *src) 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; @@ -1299,7 +1299,7 @@ pf_normalize_tcp_stateful(struct pf_pdesc *pd, u_short *reason, 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, + pf_pull_hdr(pd->m, pd->off + sizeof(*th), opts, olen, NULL, pd->af)) { /* Modulate the timestamps. Can be used for NAT detection, OS @@ -1633,7 +1633,7 @@ pf_normalize_mss(struct pf_pdesc *pd, u_int16_t maxmss) 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; diff --git a/sys/net/pf_osfp.c b/sys/net/pf_osfp.c index 247d8760001..c2a406c32b2 100644 --- a/sys/net/pf_osfp.c +++ b/sys/net/pf_osfp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf_osfp.c,v 1.46 2023/05/07 12:45:21 kn Exp $ */ +/* $OpenBSD: pf_osfp.c,v 1.47 2023/10/10 11:25:31 bluhm Exp $ */ /* * Copyright (c) 2003 Mike Frantzen @@ -111,8 +111,7 @@ pf_osfp_fingerprint(struct pf_pdesc *pd) ip6 = mtod(pd->m, struct ip6_hdr *); break; } - if (!pf_pull_hdr(pd->m, pd->off, hdr, th->th_off << 2, NULL, NULL, - pd->af)) + if (!pf_pull_hdr(pd->m, pd->off, hdr, th->th_off << 2, NULL, pd->af)) return (NULL); return (pf_osfp_fingerprint_hdr(ip, ip6, (struct tcphdr *)hdr)); diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h index 27cce82bdad..b8f1286e661 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -1,4 +1,4 @@ -/* $OpenBSD: pfvar.h,v 1.533 2023/07/06 04:55:05 dlg Exp $ */ +/* $OpenBSD: pfvar.h,v 1.534 2023/10/10 11:25:31 bluhm Exp $ */ /* * Copyright (c) 2001 Daniel Hartmeier @@ -1192,12 +1192,6 @@ enum pfi_kif_refs { #define SCNT_SRC_NODE_REMOVALS 2 #define SCNT_MAX 3 -#define ACTION_SET(a, x) \ - do { \ - if ((a) != NULL) \ - *(a) = (x); \ - } while (0) - #define REASON_SET(a, x) \ do { \ if ((void *)(a) != NULL) { \ @@ -1649,8 +1643,7 @@ 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); -void *pf_pull_hdr(struct mbuf *, int, void *, int, u_short *, u_short *, - sa_family_t); +void *pf_pull_hdr(struct mbuf *, int, void *, int, u_short *, sa_family_t); #define PF_HI (true) #define PF_LO (!PF_HI) #define PF_ALGNMNT(off) (((off) % 2) == 0 ? PF_HI : PF_LO) -- 2.20.1