[src/trunk]: src/sys/netbt Result of audit to check that mbuf length is check...

[plunky <plunky%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/cd2d3c1e9d86
branches:  trunk
changeset: 992260:cd2d3c1e9d86
user:      plunky <plunky%NetBSD.org@localhost>
date:      Tue Aug 21 14:59:13 2018 +0000

description:
Result of audit to check that mbuf length is checked before m_copydata()
and that any data supposedly copied out is valid before use.

prompted by maxv@, I have checked every usage of m_copydata() and made
the following corrections

hci_event.c:
        hci_event_command_compl()
                check that the packet does contain enough data for there to
                be a status code before noting possible failures.

        hci_event_num_compl_pkts()
                check that the packet does contain data to cover the
                stated number of handle/num pairs

l2cap_signal.c:
        l2cap_recv_signal()
                just ignore packets with not enough data rather than
                trying to reject them (may not have cmd.ident)

        l2cap_recv_command_rej()
                check we have a valid reason and/or data before use

diffstat:

 sys/netbt/hci_event.c    |  21 +++++++++++++--------
 sys/netbt/l2cap_signal.c |  28 ++++++++++++++++++----------
 2 files changed, 31 insertions(+), 18 deletions(-)

diffs (137 lines):

diff -r 4dfa44f630e5 -r cd2d3c1e9d86 sys/netbt/hci_event.c
--- a/sys/netbt/hci_event.c     Tue Aug 21 14:09:41 2018 +0000
+++ b/sys/netbt/hci_event.c     Tue Aug 21 14:59:13 2018 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: hci_event.c,v 1.24 2015/11/28 09:04:34 plunky Exp $    */
+/*     $NetBSD: hci_event.c,v 1.25 2018/08/21 14:59:13 plunky Exp $    */
 
 /*-
  * Copyright (c) 2005 Iain Hibbert.
@@ -31,7 +31,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: hci_event.c,v 1.24 2015/11/28 09:04:34 plunky Exp $");
+__KERNEL_RCSID(0, "$NetBSD: hci_event.c,v 1.25 2018/08/21 14:59:13 plunky Exp $");
 
 #include <sys/param.h>
 #include <sys/kernel.h>
@@ -316,12 +316,14 @@
         * that a command_complete packet will contain the status though most
         * do seem to.
         */
-       m_copydata(m, 0, sizeof(rp), &rp);
-       if (rp.status > 0)
-               aprint_error_dev(unit->hci_dev,
-                   "CommandComplete opcode (%03x|%04x) failed (status=0x%02x)\n",
-                   HCI_OGF(le16toh(ep.opcode)), HCI_OCF(le16toh(ep.opcode)),
-                   rp.status);
+       if (m->m_pkthdr.len >= sizeof(rp)) {
+               m_copydata(m, 0, sizeof(rp), &rp);
+               if (rp.status > 0)
+                       aprint_error_dev(unit->hci_dev,
+                           "CommandComplete opcode (%03x|%04x) failed (status=0x%02x)\n",
+                           HCI_OGF(le16toh(ep.opcode)), HCI_OCF(le16toh(ep.opcode)),
+                           rp.status);
+       }
 
        /*
         * post processing of completed commands
@@ -383,6 +385,9 @@
        m_copydata(m, 0, sizeof(ep), &ep);
        m_adj(m, sizeof(ep));
 
+       if (m->m_pkthdr.len < ep.num_con_handles * (sizeof(handle) + sizeof(num)))
+               return;
+
        while (ep.num_con_handles--) {
                m_copydata(m, 0, sizeof(handle), &handle);
                m_adj(m, sizeof(handle));
diff -r 4dfa44f630e5 -r cd2d3c1e9d86 sys/netbt/l2cap_signal.c
--- a/sys/netbt/l2cap_signal.c  Tue Aug 21 14:09:41 2018 +0000
+++ b/sys/netbt/l2cap_signal.c  Tue Aug 21 14:59:13 2018 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: l2cap_signal.c,v 1.18 2016/10/04 14:13:46 joerg Exp $  */
+/*     $NetBSD: l2cap_signal.c,v 1.19 2018/08/21 14:59:13 plunky Exp $ */
 
 /*-
  * Copyright (c) 2005 Iain Hibbert.
@@ -31,7 +31,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: l2cap_signal.c,v 1.18 2016/10/04 14:13:46 joerg Exp $");
+__KERNEL_RCSID(0, "$NetBSD: l2cap_signal.c,v 1.19 2018/08/21 14:59:13 plunky Exp $");
 
 #include <sys/param.h>
 #include <sys/kernel.h>
@@ -64,7 +64,8 @@
 
 /*
  * process incoming signal packets (CID 0x0001). Can contain multiple
- * requests/responses.
+ * requests/responses. The signal hander should clear the command from
+ * the mbuf before returning.
  */
 void
 l2cap_recv_signal(struct mbuf *m, struct hci_link *link)
@@ -72,12 +73,9 @@
        l2cap_cmd_hdr_t cmd;
 
        for(;;) {
-               if (m->m_pkthdr.len == 0)
+               if (m->m_pkthdr.len < sizeof(cmd))
                        goto finish;
 
-               if (m->m_pkthdr.len < sizeof(cmd))
-                       goto reject;
-
                m_copydata(m, 0, sizeof(cmd), &cmd);
                cmd.length = le16toh(cmd.length);
 
@@ -183,32 +181,42 @@
 
        cmd.length = le16toh(cmd.length);
 
+       /* The length here must contain the reason (2 octets) plus
+        * any data (0 or more octets) but we already know it is not
+        * bigger than l2cap_cmd_rej_cp
+        */
        m_copydata(m, 0, cmd.length, &cp);
        m_adj(m, cmd.length);
 
+       if (cmd.length < 2)
+               return;
+
        req = l2cap_request_lookup(link, cmd.ident);
        if (req == NULL)
                return;
 
        switch (le16toh(cp.reason)) {
-       case L2CAP_REJ_NOT_UNDERSTOOD:
+       case L2CAP_REJ_NOT_UNDERSTOOD:  /* data length = 0 octets */
                /*
                 * I dont know what to do, just move up the timeout
                 */
                callout_schedule(&req->lr_rtx, 0);
                break;
 
-       case L2CAP_REJ_MTU_EXCEEDED:
+       case L2CAP_REJ_MTU_EXCEEDED:    /* data length = 2 octets */
                /*
                 * I didnt send any commands over L2CAP_MTU_MINIMUM size, but..
                 *
                 * XXX maybe we should resend this, instead?
                 */
+               if (cmd.length != 4)
+                       return;
+
                link->hl_mtu = le16toh(cp.data[0]);
                callout_schedule(&req->lr_rtx, 0);
                break;
 
-       case L2CAP_REJ_INVALID_CID:
+       case L2CAP_REJ_INVALID_CID:     /* data length = 4 octets */
                /*
                 * Well, if they dont have such a channel then our channel is
                 * most likely closed. Make it so.