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@

2 files changed, 5 insertions, 149 deletions

diff --git a/sys/net/if_pflog.c b/sys/net/if_pflog.c
index 27ea9bde3b3..b574900593e 100644
--- 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 e5e03458045..5d6b49356be 100644
--- 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);