From bc2df3b76f932093bb3623386b8bf983cea2b26e Mon Sep 17 00:00:00 2001 From: Alexander Bluhm Date: Fri, 28 Dec 2018 14:32:48 +0000 Subject: 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@ --- sys/net/if_switch.c | 17 ++++++++++------- sys/net/switchctl.c | 6 ++++-- sys/net/switchofp.c | 36 +++++++++++++++++++++++------------- 3 files changed, 37 insertions(+), 22 deletions(-) (limited to 'sys') 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 @@ -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 @@ -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 @@ -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); } -- cgit v1.2.3