Fix mbuf releated crashes in switch(4). They have been found by

syzkaller as pool corruption panic.  It is unclear which bug caused
what, but it should be better now.
- Check M_PKTHDR with assertion before accessing m_pkthdr.
- Do not access oh_length without m_pullup().
- After checking if there is space at the end of the mbuf, don't
  overwrite the data at the beginning.  Append the new content.
- Do not set m_len and m_pkthdr.len when it is unclear whether
  the ofp_error header fits at all.  Use m_makespace() to adjust
  the mbuf.
Reported-by: syzbot+6efc0a9d5b700b54392e@syzkaller.appspotmail.com
test akoshibe@; OK claudio@

3 files changed, 37 insertions, 22 deletions

diff --git a/sys/net/if_switch.c b/sys/net/if_switch.c
index dc06a20fa01..04df95a1843 100644
--- a/sys/net/if_switch.c
+++ b/sys/net/if_switch.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_switch.c,v 1.24 2018/12/07 16:19:40 mpi Exp $	*/
+/*	$OpenBSD: if_switch.c,v 1.25 2018/12/28 14:32:47 bluhm Exp $	*/
 
 /*
  * Copyright (c) 2016 Kazuya GODA <goda@openbsd.org>
@@ -221,6 +221,7 @@ switchintr(void)
 		return;
 
 	while ((m = ml_dequeue(&ml)) != NULL) {
+		KASSERT(m->m_flags & M_PKTHDR);
 		ifp = if_get(m->m_pkthdr.ph_ifidx);
 		if (ifp == NULL) {
 			m_freem(m);
@@ -741,6 +742,7 @@ switch_ifenqueue(struct switch_softc *sc, struct ifnet *ifp,
 	/* Loop prevention. */
 	m->m_flags |= M_PROTO1;
 
+	KASSERT(m->m_flags & M_PKTHDR);
 	len = m->m_pkthdr.len;
 
 	if (local) {
@@ -1487,22 +1489,23 @@ switch_mtap(caddr_t arg, struct mbuf *m, int dir, uint64_t datapath_id)
 int
 ofp_split_mbuf(struct mbuf *m, struct mbuf **mtail)
 {
-	struct ofp_header	*oh;
 	uint16_t		 ohlen;
 
 	*mtail = NULL;
 
  again:
 	/* We need more data. */
-	if (m->m_pkthdr.len < sizeof(*oh))
+	KASSERT(m->m_flags & M_PKTHDR);
+	if (m->m_pkthdr.len < sizeof(struct ofp_header))
 		return (-1);
 
-	oh = mtod(m, struct ofp_header *);
-	ohlen = ntohs(oh->oh_length);
+	m_copydata(m, offsetof(struct ofp_header, oh_length), sizeof(ohlen),
+	    (caddr_t)&ohlen);
+	ohlen = ntohs(ohlen);
 
 	/* We got an invalid packet header, skip it. */
-	if (ohlen < sizeof(*oh)) {
-		m_adj(m, sizeof(*oh));
+	if (ohlen < sizeof(struct ofp_header)) {
+		m_adj(m, sizeof(struct ofp_header));
 		goto again;
 	}
 
diff --git a/sys/net/switchctl.c b/sys/net/switchctl.c
index c1f67416d38..6e40ad6a2a4 100644
--- a/sys/net/switchctl.c
+++ b/sys/net/switchctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: switchctl.c,v 1.13 2018/11/09 14:14:31 claudio Exp $	*/
+/*	$OpenBSD: switchctl.c,v 1.14 2018/12/28 14:32:47 bluhm Exp $	*/
 
 /*
  * Copyright (c) 2016 Kazuya GODA <goda@openbsd.org>
@@ -236,10 +236,12 @@ switchwrite(dev_t dev, struct uio *uio, int ioflag)
 
 		sc->sc_swdev->swdev_inputm = NULL;
 	}
+	KASSERT(mhead->m_flags & M_PKTHDR);
 
 	while (len) {
 		trailing = ulmin(m_trailingspace(m), len);
-		if ((error = uiomove(mtod(m, caddr_t), trailing, uio)) != 0)
+		if ((error = uiomove(mtod(m, caddr_t) + m->m_len, trailing,
+		    uio)) != 0)
 			goto save_return;
 
 		len -= trailing;
diff --git a/sys/net/switchofp.c b/sys/net/switchofp.c
index 9d4b1169f81..8c0fe5d7272 100644
--- a/sys/net/switchofp.c
+++ b/sys/net/switchofp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: switchofp.c,v 1.71 2018/08/21 16:40:23 akoshibe Exp $	*/
+/*	$OpenBSD: switchofp.c,v 1.72 2018/12/28 14:32:47 bluhm Exp $	*/
 
 /*
  * Copyright (c) 2016 Kazuya GODA <goda@openbsd.org>
@@ -4666,7 +4666,8 @@ swofp_input(struct switch_softc *sc, struct mbuf *m)
 
 	ohlen = ntohs(oh->oh_length);
 	/* Validate that we have a sane header. */
-	if (ohlen < sizeof(*oh)) {
+	KASSERT(m->m_flags & M_PKTHDR);
+	if (ohlen < sizeof(*oh) || m->m_pkthdr.len < ohlen) {
 		swofp_send_error(sc, m, OFP_ERRTYPE_BAD_REQUEST,
 		    OFP_ERRREQ_BAD_LEN);
 		return (0);
@@ -4761,28 +4762,37 @@ void
 swofp_send_error(struct switch_softc *sc, struct mbuf *m,
     uint16_t type, uint16_t code)
 {
+	struct mbuf		*n;
+	struct ofp_header	*oh;
 	struct ofp_error	*oe;
+	int			 off;
+	uint32_t		 xid;
 	uint16_t		 len;
-	uint8_t			 data[OFP_ERRDATA_MAX];
 
 	/* Reuse mbuf from request message */
-	oe = mtod(m, struct ofp_error *);
+	oh = mtod(m, struct ofp_header *);
 
 	/* Save data for the response and copy back later. */
-	len = min(ntohs(oe->err_oh.oh_length), OFP_ERRDATA_MAX);
-	m_copydata(m, 0, len, data);
+	len = min(ntohs(oh->oh_length), OFP_ERRDATA_MAX);
+	if (len < m->m_pkthdr.len)
+		m_adj(m, len - m->m_pkthdr.len);
+	xid = oh->oh_xid;
+
+	if ((n = m_makespace(m, 0, sizeof(struct ofp_error), &off)) == NULL) {
+		m_freem(m);
+		return;
+	}
+	/* if skip is 0, off is also 0 */
+	KASSERT(off == 0);
+
+	oe = mtod(n, struct ofp_error *);
 
 	oe->err_oh.oh_version = OFP_V_1_3;
 	oe->err_oh.oh_type = OFP_T_ERROR;
+	oe->err_oh.oh_length = htons(sizeof(struct ofp_error) + len);
+	oe->err_oh.oh_xid = xid;
 	oe->err_type = htons(type);
 	oe->err_code = htons(code);
-	oe->err_oh.oh_length = htons(len + sizeof(struct ofp_error));
-	m->m_len = m->m_pkthdr.len = sizeof(struct ofp_error);
-
-	if (m_copyback(m, sizeof(struct ofp_error), len, data, M_DONTWAIT)) {
-		m_freem(m);
-		return;
-	}
 
 	(void)swofp_output(sc, m);
 }