commit 5008836c7a75422ba9653f807ede965a3ce7e1b6
author John Grossman <johngro@google.com> Wed Jul 08 00:08:06 2020 +0000
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Wed Jul 08 00:08:06 2020 +0000
tree 6096e57f9b4b1349dac22f6e2d55e0318893cebe
parent 5f2654ff682a11260e4fe23bc5e0ce1ee0a40361

[bt][hci] Fix a potential overflow

Fix a potential overflow found during a security audit.  See the
referenced bug for details

Currently, there are no good ways to write automated tests for this.
My understanding is that this entire driver is going to be replaced
with a new version sometime soon, so investing a lot of extra effort
in automated testing of this may not be worth it.  I have manually
done a spot check of this patch by deploying to a Sherlock and making
sure that I can still discover, pair with, and communicate to a set of
BT headphones.

Fixed: 55287
Change-Id: I17c5333434335bb4277e21e89983e50da4a15c9c
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/404695
Commit-Queue: John Grossman <johngro@google.com>
Reviewed-by: Jeff Belgum <belgum@google.com>
Testability-Review: Jeff Belgum <belgum@google.com>

src/connectivity/bluetooth/hci/transport/uart/bt-transport-uart.c

diff --git a/src/connectivity/bluetooth/hci/transport/uart/bt-transport-uart.c b/src/connectivity/bluetooth/hci/transport/uart/bt-transport-uart.c
index 3550740..a53f18dc 100644
--- a/src/connectivity/bluetooth/hci/transport/uart/bt-transport-uart.c
+++ b/src/connectivity/bluetooth/hci/transport/uart/bt-transport-uart.c
@@ -253,8 +253,10 @@
 
       size_t remaining = end - src;
       size_t copy = packet_length - hci->event_buffer_offset;
-      if (copy > remaining)
+      if (copy > remaining) {
         copy = remaining;
+      }
+      ZX_ASSERT((hci->event_buffer_offset + copy) <= sizeof(hci->event_buffer));
       memcpy(hci->event_buffer + hci->event_buffer_offset, src, copy);
       src += copy;
       hci->event_buffer_offset += copy;
@@ -290,21 +292,44 @@
         hci->event_buffer_offset = 1;
       }
     } else {  // HCI_ACL_DATA
-      size_t packet_length = EVENT_PACKET_LENGTH(hci);
+      size_t packet_length = ACL_PACKET_LENGTH(hci);
 
       while (!packet_length && src < end) {
         // read until we have enough to compute packet length
         hci->acl_buffer[hci->acl_buffer_offset++] = *src++;
         packet_length = ACL_PACKET_LENGTH(hci);
       }
+
+      // Out of bytes, but we still don't know the packet length.  Just wait for
+      // the next packet.
       if (!packet_length) {
         break;
       }
 
+      // Sanity check out packet length.  The value computed by
+      // ACL_PACKET_LENGTH includes not only the packet payload size (as read
+      // from the packet itself), but also the 5 bytes of packet overhead.  We
+      // should be able to simply check packet_length against the size of the
+      // reassembly buffer.
+      if (packet_length > sizeof(hci->acl_buffer)) {
+        zxlogf(ERROR,
+               "bt-transport-uart: packet_length is too large (%zu > %zu) during ACL packet "
+               "reassembly.  Dropping and attempting to re-sync.\n",
+               packet_length, sizeof(hci->acl_buffer_offset));
+
+        // reset the reassembly state machine.
+        packet_type = HCI_NONE;
+        hci->acl_buffer_offset = 1;
+        break;
+      }
+
       size_t remaining = end - src;
       size_t copy = packet_length - hci->acl_buffer_offset;
-      if (copy > remaining)
+      if (copy > remaining) {
         copy = remaining;
+      }
+
+      ZX_ASSERT((hci->acl_buffer_offset + copy) <= sizeof(hci->acl_buffer));
       memcpy(hci->acl_buffer + hci->acl_buffer_offset, src, copy);
       src += copy;
       hci->acl_buffer_offset += copy;
@@ -613,8 +638,7 @@
 
   hci->serial = serial;
   if (status != ZX_OK) {
-    zxlogf(ERROR, "bt-transport-uart: serial_open_socket failed: %s",
-           zx_status_get_string(status));
+    zxlogf(ERROR, "bt-transport-uart: serial_open_socket failed: %s", zx_status_get_string(status));
     goto fail;
   }