[Qemu-devel] [PATCH] bt: rewrite csrhci_write to avoid out-of-bounds writes

[Qemu-devel] [PATCH] bt: rewrite csrhci_write to avoid out-of-bounds writes

[Paolo Bonzini]

The usage of INT_MAX in this function confuses Coverity.  I think
the defect is bogus, however there is no protection against
getting more than sizeof(s->inpkt) bytes from the character device
backend.

Rewrite the function to only fill in as much data as needed from
buf into s->inpkt.  The plen variable is replaced by a simple
state machine and there is no need anymore to shift contents to
the beginning of s->inpkt.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/bt/hci-csr.c | 67 +++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 46 insertions(+), 21 deletions(-)

diff --git a/hw/bt/hci-csr.c b/hw/bt/hci-csr.c
index e6b8998..d688372 100644
--- a/hw/bt/hci-csr.c
+++ b/hw/bt/hci-csr.c
@@ -39,9 +39,14 @@ struct csrhci_s {
     int out_size;
     uint8_t outfifo[FIFO_LEN * 2];
     uint8_t inpkt[FIFO_LEN];
+    enum {
+        CSR_HDR_LEN,
+        CSR_DATA_LEN,
+        CSR_DATA
+    } in_state;
     int in_len;
     int in_hdr;
-    int in_data;
+    int in_needed;
     QEMUTimer *out_tm;
     int64_t baud_delay;
 
@@ -296,38 +301,60 @@ static int csrhci_data_len(const uint8_t *pkt)
     exit(-1);
 }
 
+static void csrhci_ready_for_next_inpkt(struct csrhci_s *s)
+{
+    s->in_state = CSR_HDR_LEN;
+    s->in_len = 0;
+    s->in_needed = 2;
+    s->in_hdr = INT_MAX;
+}
+
 static int csrhci_write(struct CharDriverState *chr,
                 const uint8_t *buf, int len)
 {
     struct csrhci_s *s = (struct csrhci_s *) chr->opaque;
-    int plen = s->in_len;
+    int total = 0;
 
     if (!s->enable)
         return 0;
 
-    s->in_len += len;
-    memcpy(s->inpkt + plen, buf, len);
+    for (;;) {
+        int cnt = MIN(len, s->in_needed - s->in_len);
+        if (cnt) {
+            memcpy(s->inpkt + s->in_len, buf, cnt);
+            s->in_len += cnt;
+            buf += cnt;
+            len -= cnt;
+            total += cnt;
+        }
+
+        if (s->in_len < s->in_needed) {
+            break;
+        }
 
-    while (1) {
-        if (s->in_len >= 2 && plen < 2)
+        if (s->in_state == CSR_HDR_LEN) {
             s->in_hdr = csrhci_header_len(s->inpkt) + 1;
+            assert(s->in_hdr >= s->in_needed);
+            s->in_needed = s->in_hdr;
+            s->in_state = CSR_DATA_LEN;
+            continue;
+        }
 
-        if (s->in_len >= s->in_hdr && plen < s->in_hdr)
-            s->in_data = csrhci_data_len(s->inpkt) + s->in_hdr;
+        if (s->in_state == CSR_DATA_LEN) {
+            s->in_needed += csrhci_data_len(s->inpkt);
+            /* hci_acl_hdr could specify more than 4096 bytes, so assert.  */
+            assert(s->in_needed <= sizeof(s->inpkt));
+            s->in_state = CSR_DATA;
+            continue;
+        }
 
-        if (s->in_len >= s->in_data) {
+        if (s->in_state == CSR_DATA) {
             csrhci_in_packet(s, s->inpkt);
-
-            memmove(s->inpkt, s->inpkt + s->in_len, s->in_len - s->in_data);
-            s->in_len -= s->in_data;
-            s->in_hdr = INT_MAX;
-            s->in_data = INT_MAX;
-            plen = 0;
-        } else
-            break;
+            csrhci_ready_for_next_inpkt(s);
+        }
     }
 
-    return len;
+    return total;
 }
 
 static void csrhci_out_hci_packet_event(void *opaque,
@@ -389,11 +416,9 @@ static void csrhci_reset(struct csrhci_s *s)
 {
     s->out_len = 0;
     s->out_size = FIFO_LEN;
-    s->in_len = 0;
+    csrhci_ready_for_next_inpkt(s);
     s->baud_delay = NANOSECONDS_PER_SECOND;
     s->enable = 0;
-    s->in_hdr = INT_MAX;
-    s->in_data = INT_MAX;
 
     s->modem_state = 0;
     /* After a while... (but sooner than 10ms) */
-- 
2.5.5