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@

diff --git a/sys/net/pf.c b/sys/net/pf.c
index ad2d73a..dfbac2b 100644 (file)
--- 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 ef2c884..fde4a21 100644 (file)
--- 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 <provos@citi.umich.edu>
@@ -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 247d876..c2a406c 100644 (file)
--- 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 <frantzen@w4g.org>
@@ -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 27cce82..b8f1286 100644 (file)
--- 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)