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@

diff --git a/sys/net/if_switch.c b/sys/net/if_switch.c
index dc06a20..04df95a 100644 (file)
--- 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 c1f6741..6e40ad6 100644 (file)
--- 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 9d4b116..8c0fe5d 100644 (file)
--- 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);
 }