pflog(4) tried to log the translated packet with rdr-to, nat-to,

and af-to addresses and ports applied.  Therefore it created a mbuf
chain on the stack with a partial copy.  This is too complicated
for IP options, extension header, NAT46 af-to, and fragmented mbuf
chains.  It even caused a crash in syzkaller.  Usually the length
checks in pf_setup_pdesc() rejected the faked mbuf and the goto
copy logged the packet unmodified.  Remove the pflog_mtap() function
and call bpf_mtap_hdr() directly.  As the old buggy code was bypassed
in most cases, tcpdump(8) output of pflog does not change.
Uncondionally log the unmodified packet.
Reported-by: syzbot+947e89e06ac3fec187d0@syzkaller.appspotmail.com
OK sashan@

diff --git a/sys/net/if_pflog.c b/sys/net/if_pflog.c
index 27ea9bd..b574900 100644 (file)
--- a/sys/net/if_pflog.c
+++ b/sys/net/if_pflog.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_pflog.c,v 1.94 2021/01/13 09:13:30 mvs Exp $       */
+/*     $OpenBSD: if_pflog.c,v 1.95 2021/01/19 22:22:23 bluhm Exp $     */
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr) and
@@ -81,7 +81,6 @@ int   pflogoutput(struct ifnet *, struct mbuf *, struct sockaddr *,
 int    pflogioctl(struct ifnet *, u_long, caddr_t);
 int    pflog_clone_create(struct if_clone *, int);
 int    pflog_clone_destroy(struct ifnet *);
-void   pflog_mtap(caddr_t, struct pfloghdr *, struct mbuf *);
 struct pflog_softc     *pflog_getif(int);
 
 struct if_clone                        pflog_cloner =
@@ -242,144 +241,8 @@ pflog_packet(struct pf_pdesc *pd, u_int8_t reason, struct pf_rule *rm,
        ifn->if_opackets++;
        ifn->if_obytes += pd->m->m_pkthdr.len;
 
-       pflog_mtap(if_bpf, &hdr, pd->m);
+       bpf_mtap_hdr(if_bpf, &hdr, sizeof(hdr), pd->m, BPF_DIRECTION_OUT);
 #endif
 
        return (0);
 }
-
-void
-pflog_mtap(caddr_t if_bpf, struct pfloghdr *pfloghdr, struct mbuf *m)
-{
-       struct mbuf              mhdr;
-       struct m_hdr             mptr;
-       struct mbuf             *mp;
-       u_char                  *mdst;
-       int                      afto, hlen, off;
-
-       struct pf_pdesc          pd;
-       struct pf_addr           osaddr, odaddr;
-       u_int16_t                osport = 0, odport = 0;
-       u_int8_t                 proto = 0;
-
-       afto = pfloghdr->af != pfloghdr->naf;
-
-       /*
-        * temporary mbuf will hold an ip/ip6 header and 8 bytes
-        * of the protocol header
-        */
-       m_inithdr(&mhdr);
-       mhdr.m_len = 0; /* XXX not done in m_inithdr() */
-
-#ifdef INET6
-       /* offset for a new header */
-       if (afto && pfloghdr->af == AF_INET)
-               mhdr.m_data += sizeof(struct ip6_hdr) -
-                   sizeof(struct ip);
-#endif /* INET6 */
-
-       mdst = mtod(&mhdr, char *);
-       switch (pfloghdr->af) {
-       case AF_INET: {
-               struct ip       *h;
-
-               if (m->m_pkthdr.len < sizeof(*h))
-                       goto copy;
-               m_copydata(m, 0, sizeof(*h), mdst);
-               h = (struct ip *)mdst;
-               hlen = h->ip_hl << 2;
-               if (hlen > sizeof(*h) && (m->m_pkthdr.len >= hlen))
-                       m_copydata(m, sizeof(*h), hlen - sizeof(*h),
-                           mdst + sizeof(*h));
-               break;
-           }
-#ifdef INET6
-       case AF_INET6: {
-               struct ip6_hdr  *h;
-
-               if (m->m_pkthdr.len < sizeof(*h))
-                       goto copy;
-               hlen = sizeof(struct ip6_hdr);
-               m_copydata(m, 0, hlen, mdst);
-               h = (struct ip6_hdr *)mdst;
-               proto = h->ip6_nxt;
-               break;
-           }
-#endif /* INET6 */
-       default:
-               /* shouldn't happen ever :-) */
-               goto copy;
-       }
-
-       if (m->m_pkthdr.len < hlen + 8 && proto != IPPROTO_NONE)
-               goto copy;
-       else if (proto != IPPROTO_NONE) {
-               /* copy 8 bytes of the protocol header */
-               m_copydata(m, hlen, 8, mdst + hlen);
-               hlen += 8;
-       }
-
-       mhdr.m_len = hlen;
-       mhdr.m_pkthdr.len = hlen;
-
-       /* create a chain mhdr -> mptr, mptr->m_data = (m->m_data+hlen) */
-       mp = m_getptr(m, hlen, &off);
-       if (mp != NULL) {
-               mptr.mh_flags = 0;
-               mptr.mh_data = mp->m_data + off;
-               mptr.mh_len = mp->m_len - off;
-               mptr.mh_next = mp->m_next;
-
-               mhdr.m_next = (struct mbuf *)&mptr;
-       }
-
-       /*
-        * Rewrite addresses if needed. Reason pointer must be NULL to avoid
-        * counting the packet here again.
-        */
-       if (pf_setup_pdesc(&pd, pfloghdr->af, pfloghdr->dir, NULL,
-           &mhdr, NULL) != PF_PASS)
-               goto copy;
-       pd.naf = pfloghdr->naf;
-
-       pf_addrcpy(&osaddr, pd.src, pd.af);
-       pf_addrcpy(&odaddr, pd.dst, pd.af);
-       if (pd.sport)
-               osport = *pd.sport;
-       if (pd.dport)
-               odport = *pd.dport;
-
-       if (pd.virtual_proto != PF_VPROTO_FRAGMENT &&
-           (pfloghdr->rewritten = pf_translate(&pd, &pfloghdr->saddr,
-           pfloghdr->sport, &pfloghdr->daddr, pfloghdr->dport, 0,
-           pfloghdr->dir))) {
-               m_copyback(pd.m, pd.off, min(pd.m->m_len - pd.off, pd.hdrlen),
-                   &pd.hdr, M_NOWAIT);
-#ifdef INET6
-               if (afto) {
-                       pf_addrcpy(&pd.nsaddr, &pfloghdr->saddr, pd.naf);
-                       pf_addrcpy(&pd.ndaddr, &pfloghdr->daddr, pd.naf);
-               }
-#endif /* INET6 */
-               pf_addrcpy(&pfloghdr->saddr, &osaddr, pd.af);
-               pf_addrcpy(&pfloghdr->daddr, &odaddr, pd.af);
-               pfloghdr->sport = osport;
-               pfloghdr->dport = odport;
-       }
-
-       pd.tot_len -= pd.m->m_data - pd.m->m_pktdat;
-
-#ifdef INET6
-       if (afto && pfloghdr->rewritten)
-               pf_translate_af(&pd);
-#endif /* INET6 */
-
-       m = pd.m;
-       KASSERT(m == &mhdr);
- copy:
-#if NBPFILTER > 0
-       bpf_mtap_hdr(if_bpf, pfloghdr, sizeof(*pfloghdr), m,
-           BPF_DIRECTION_OUT);
-#endif
-       return;
-}

diff --git a/sys/net/pf.c b/sys/net/pf.c
index e5e0345..5d6b493 100644 (file)
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: pf.c,v 1.1100 2021/01/16 13:09:46 bluhm Exp $ */
+/*     $OpenBSD: pf.c,v 1.1101 2021/01/19 22:22:23 bluhm Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -4184,11 +4184,6 @@ pf_translate(struct pf_pdesc *pd, struct pf_addr *saddr, u_int16_t sport,
     struct pf_addr *daddr, u_int16_t dport, u_int16_t virtual_type,
     int icmp_dir)
 {
-       /*
-        * when called from bpf_mtap_pflog, there are extra constraints:
-        * -mbuf is faked, m_data is the bpf buffer
-        * -pd is not fully set up
-        */
        int     rewrite = 0;
        int     afto = pd->af != pd->naf;
 
@@ -4203,18 +4198,17 @@ pf_translate(struct pf_pdesc *pd, struct pf_addr *saddr, u_int16_t sport,
                break;
 
        case IPPROTO_ICMP:
-               /* pf_translate() is also used when logging invalid packets */
                if (pd->af != AF_INET)
                        return (0);
 
-               if (afto) {
 #ifdef INET6
+               if (afto) {
                        if (pf_translate_icmp_af(pd, AF_INET6, &pd->hdr.icmp))
                                return (0);
                        pd->proto = IPPROTO_ICMPV6;
                        rewrite = 1;
-#endif /* INET6 */
                }
+#endif /* INET6 */
                if (virtual_type == htons(ICMP_ECHO)) {
                        u_int16_t icmpid = (icmp_dir == PF_IN) ? sport : dport;
                        rewrite += pf_patch_16(pd,
@@ -4224,7 +4218,6 @@ pf_translate(struct pf_pdesc *pd, struct pf_addr *saddr, u_int16_t sport,
 
 #ifdef INET6
        case IPPROTO_ICMPV6:
-               /* pf_translate() is also used when logging invalid packets */
                if (pd->af != AF_INET6)
                        return (0);