Skip to contentSkip to conversation

bt: use size_t type for length parameters instead of int

PATCH In review P J P 5 participants 7 messages Nov 19, 2018 – Jan 29, 2019

lore
Share & tools
lore

P J P

author
#0 lore raw

PATCH v2 1/1 bt: use size_t type for length parameters instead of int

From: Prasad J Pandit <[email protected]>

The length parameter values are not negative, thus use an unsigned type 'size_t' for them. Many routines pass 'len' values to memcpy(3) calls. If it was negative, it could lead to memory corruption issues. Add check to avoid it.

Reported-by Arash TC <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 bt-host.c              |  8 +++---
 bt-vhci.c              |  7 +++---
 hw/bt/core.c           |  2 +-
 hw/bt/hci-csr.c        | 32 ++++++++++++------------
 hw/bt/hci.c            | 38 ++++++++++++++--------------
 hw/bt/hid.c            | 10 ++++----
 hw/bt/l2cap.c          | 56 ++++++++++++++++++++++--------------------
 hw/bt/sdp.c            |  6 ++---
 hw/usb/dev-bluetooth.c | 12 ++++-----
 include/hw/bt.h        |  8 +++---
 include/sysemu/bt.h    | 10 ++++----
 11 files changed, 96 insertions(+), 93 deletions(-)

Update v2: modify assert calls
-> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg01036.html

bt-host.c +4 -4
index 2f8f631c25..b73a44d07d 100644
@@ -43,7 +43,7 @@ struct bt_host_hci_s {
 };
 
 static void bt_host_send(struct HCIInfo *hci,
-                int type, const uint8_t *data, int len)
+                int type, const uint8_t *data, size_t len)
 {
     struct bt_host_hci_s *s = (struct bt_host_hci_s *) hci;
     uint8_t pkt = type;

@@ -63,17 +63,17 @@ static void bt_host_send(struct HCIInfo *hci,
     }
 }
 
-static void bt_host_cmd(struct HCIInfo *hci, const uint8_t *data, int len)
+static void bt_host_cmd(struct HCIInfo *hci, const uint8_t *data, size_t len)
 {
     bt_host_send(hci, HCI_COMMAND_PKT, data, len);
 }
 
-static void bt_host_acl(struct HCIInfo *hci, const uint8_t *data, int len)
+static void bt_host_acl(struct HCIInfo *hci, const uint8_t *data, size_t len)
 {
     bt_host_send(hci, HCI_ACLDATA_PKT, data, len);
 }
 
-static void bt_host_sco(struct HCIInfo *hci, const uint8_t *data, int len)
+static void bt_host_sco(struct HCIInfo *hci, const uint8_t *data, size_t len)
 {
     bt_host_send(hci, HCI_SCODATA_PKT, data, len);
 }
bt-vhci.c +4 -3
index 9d277c32bf..765773188d 100644
@@ -90,7 +90,7 @@ static void vhci_read(void *opaque)
 }
 
 static void vhci_host_send(void *opaque,
-                int type, const uint8_t *data, int len)
+                int type, const uint8_t *data, size_t len)
 {
     struct bt_vhci_s *s = (struct bt_vhci_s *) opaque;
 #if 0

@@ -113,6 +113,7 @@ static void vhci_host_send(void *opaque,
     static uint8_t buf[4096];
 
     buf[0] = type;
+    assert(len < sizeof(buf));
     memcpy(buf + 1, data, len);
 
     while (write(s->fd, buf, len + 1) < 0)

@@ -125,13 +126,13 @@ static void vhci_host_send(void *opaque,
 }
 
 static void vhci_out_hci_packet_event(void *opaque,
-                const uint8_t *data, int len)
+                const uint8_t *data, size_t len)
 {
     vhci_host_send(opaque, HCI_EVENT_PKT, data, len);
 }
 
 static void vhci_out_hci_packet_acl(void *opaque,
-                const uint8_t *data, int len)
+                const uint8_t *data, size_t len)
 {
     vhci_host_send(opaque, HCI_ACLDATA_PKT, data, len);
 }
hw/bt/core.c +1 -1
index 78370e64f5..62720d1663 100644
@@ -45,7 +45,7 @@ static void bt_dummy_lmp_disconnect_master(struct bt_link_s *link)
 }
 
 static void bt_dummy_lmp_acl_resp(struct bt_link_s *link,
-                const uint8_t *data, int start, int len)
+                const uint8_t *data, int start, size_t len)
 {
     error_report("%s: stray ACL response PDU, fixme", __func__);
     exit(-1);
hw/bt/hci-csr.c +16 -16
index 0341ded50c..26bd516d31 100644
@@ -103,7 +103,7 @@ static inline void csrhci_fifo_wake(struct csrhci_s *s)
 }
 
 #define csrhci_out_packetz(s, len) memset(csrhci_out_packet(s, len), 0, len)
-static uint8_t *csrhci_out_packet(struct csrhci_s *s, int len)
+static uint8_t *csrhci_out_packet(struct csrhci_s *s, size_t len)
 {
     int off = s->out_start + s->out_len;
 

@@ -112,14 +112,14 @@ static uint8_t *csrhci_out_packet(struct csrhci_s *s, int len)
 
     if (off < FIFO_LEN) {
         if (off + len > FIFO_LEN && (s->out_size = off + len) > FIFO_LEN * 2) {
-            error_report("%s: can't alloc %i bytes", __func__, len);
+            error_report("%s: can't alloc %zu bytes", __func__, len);
             exit(-1);
         }
         return s->outfifo + off;
     }
 
     if (s->out_len > s->out_size) {
-        error_report("%s: can't alloc %i bytes", __func__, len);
+        error_report("%s: can't alloc %zu bytes", __func__, len);
         exit(-1);
     }
 

@@ -127,7 +127,7 @@ static uint8_t *csrhci_out_packet(struct csrhci_s *s, int len)
 }
 
 static inline uint8_t *csrhci_out_packet_csr(struct csrhci_s *s,
-                int type, int len)
+                int type, size_t len)
 {
     uint8_t *ret = csrhci_out_packetz(s, len + 2);
 

@@ -138,7 +138,7 @@ static inline uint8_t *csrhci_out_packet_csr(struct csrhci_s *s,
 }
 
 static inline uint8_t *csrhci_out_packet_event(struct csrhci_s *s,
-                int evt, int len)
+                int evt, size_t len)
 {
     uint8_t *ret = csrhci_out_packetz(s,
                     len + 1 + sizeof(struct hci_event_hdr));

@@ -151,7 +151,7 @@ static inline uint8_t *csrhci_out_packet_event(struct csrhci_s *s,
 }
 
 static void csrhci_in_packet_vendor(struct csrhci_s *s, int ocf,
-                uint8_t *data, int len)
+                uint8_t *data, size_t len)
 {
     int offset;
     uint8_t *rpkt;

@@ -320,18 +320,18 @@ static int csrhci_write(struct Chardev *chr,
     struct csrhci_s *s = (struct csrhci_s *)chr;
     int total = 0;
 
-    if (!s->enable)
+    if (!s->enable || len <= 0)
         return 0;
 
     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;
-        }
+        assert(cnt > 0);
+
+        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;

@@ -363,7 +363,7 @@ static int csrhci_write(struct Chardev *chr,
 }
 
 static void csrhci_out_hci_packet_event(void *opaque,
-                const uint8_t *data, int len)
+                const uint8_t *data, size_t len)
 {
     struct csrhci_s *s = (struct csrhci_s *) opaque;
     uint8_t *pkt = csrhci_out_packet(s, (len + 2) & ~1);	/* Align */

@@ -375,7 +375,7 @@ static void csrhci_out_hci_packet_event(void *opaque,
 }
 
 static void csrhci_out_hci_packet_acl(void *opaque,
-                const uint8_t *data, int len)
+                const uint8_t *data, size_t len)
 {
     struct csrhci_s *s = (struct csrhci_s *) opaque;
     uint8_t *pkt = csrhci_out_packet(s, (len + 2) & ~1);	/* Align */
hw/bt/hci.c +19 -19
index c6b2cc1d48..c59ccc55b9 100644
@@ -32,7 +32,7 @@
 
 struct bt_hci_s {
     uint8_t *(*evt_packet)(void *opaque);
-    void (*evt_submit)(void *opaque, int len);
+    void (*evt_submit)(void *opaque, size_t len);
     void *opaque;
     uint8_t evt_buf[256];
 

@@ -62,7 +62,7 @@ struct bt_hci_s {
         struct bt_hci_master_link_s {
             struct bt_link_s *link;
             void (*lmp_acl_data)(struct bt_link_s *link,
-                            const uint8_t *data, int start, int len);
+                            const uint8_t *data, int start, size_t len);
             QEMUTimer *acl_mode_timer;
         } handle[HCI_HANDLES_MAX];
         uint32_t role_bmp;

@@ -434,7 +434,7 @@ static const uint8_t bt_event_reserved_mask[8] = {
 };
 
 
-static void null_hci_send(struct HCIInfo *hci, const uint8_t *data, int len)
+static void null_hci_send(struct HCIInfo *hci, const uint8_t *data, size_t len)
 {
 }
 

@@ -452,13 +452,13 @@ struct HCIInfo null_hci = {
 
 
 static inline uint8_t *bt_hci_event_start(struct bt_hci_s *hci,
-                int evt, int len)
+                int evt, size_t len)
 {
     uint8_t *packet, mask;
     int mask_byte;
 
     if (len > 255) {
-        error_report("%s: HCI event params too long (%ib)", __func__, len);
+        error_report("%s: HCI event params too long (%zub)", __func__, len);
         exit(-1);
     }
 

@@ -475,7 +475,7 @@ static inline uint8_t *bt_hci_event_start(struct bt_hci_s *hci,
 }
 
 static inline void bt_hci_event(struct bt_hci_s *hci, int evt,
-                void *params, int len)
+                void *params, size_t len)
 {
     uint8_t *packet = bt_hci_event_start(hci, evt, len);
 

@@ -500,7 +500,7 @@ static inline void bt_hci_event_status(struct bt_hci_s *hci, int status)
 }
 
 static inline void bt_hci_event_complete(struct bt_hci_s *hci,
-                void *ret, int len)
+                void *ret, size_t len)
 {
     uint8_t *packet = bt_hci_event_start(hci, EVT_CMD_COMPLETE,
                     len + EVT_CMD_COMPLETE_SIZE);

@@ -1477,7 +1477,7 @@ static inline void bt_hci_event_num_comp_pkts(struct bt_hci_s *hci,
 }
 
 static void bt_submit_hci(struct HCIInfo *info,
-                const uint8_t *data, int length)
+                const uint8_t *data, size_t length)
 {
     struct bt_hci_s *hci = hci_from_info(info);
     uint16_t cmd;

@@ -1971,7 +1971,7 @@ static void bt_submit_hci(struct HCIInfo *info,
         break;
 
     short_hci:
-        error_report("%s: HCI packet too short (%iB)", __func__, length);
+        error_report("%s: HCI packet too short (%zuB)", __func__, length);
         bt_hci_event_status(hci, HCI_INVALID_PARAMETERS);
         break;
     }

@@ -1982,7 +1982,7 @@ static void bt_submit_hci(struct HCIInfo *info,
  * know that a packet contained the last fragment of the SDU when the next
  * SDU starts.  */
 static inline void bt_hci_lmp_acl_data(struct bt_hci_s *hci, uint16_t handle,
-                const uint8_t *data, int start, int len)
+                const uint8_t *data, int start, size_t len)
 {
     struct hci_acl_hdr *pkt = (void *) hci->acl_buf;
 

@@ -1990,7 +1990,7 @@ static inline void bt_hci_lmp_acl_data(struct bt_hci_s *hci, uint16_t handle,
     /* TODO: avoid memcpy'ing */
 
     if (len + HCI_ACL_HDR_SIZE > sizeof(hci->acl_buf)) {
-        error_report("%s: can't take ACL packets %i bytes long",
+        error_report("%s: can't take ACL packets %zu bytes long",
                      __func__, len);
         return;
     }

@@ -2004,7 +2004,7 @@ static inline void bt_hci_lmp_acl_data(struct bt_hci_s *hci, uint16_t handle,
 }
 
 static void bt_hci_lmp_acl_data_slave(struct bt_link_s *btlink,
-                const uint8_t *data, int start, int len)
+                const uint8_t *data, int start, size_t len)
 {
     struct bt_hci_link_s *link = (struct bt_hci_link_s *) btlink;
 

@@ -2013,14 +2013,14 @@ static void bt_hci_lmp_acl_data_slave(struct bt_link_s *btlink,
 }
 
 static void bt_hci_lmp_acl_data_host(struct bt_link_s *link,
-                const uint8_t *data, int start, int len)
+                const uint8_t *data, int start, size_t len)
 {
     bt_hci_lmp_acl_data(hci_from_device(link->host),
                     link->handle, data, start, len);
 }
 
 static void bt_submit_acl(struct HCIInfo *info,
-                const uint8_t *data, int length)
+                const uint8_t *data, size_t length)
 {
     struct bt_hci_s *hci = hci_from_info(info);
     uint16_t handle;

@@ -2028,7 +2028,7 @@ static void bt_submit_acl(struct HCIInfo *info,
     struct bt_link_s *link;
 
     if (length < HCI_ACL_HDR_SIZE) {
-        error_report("%s: ACL packet too short (%iB)", __func__, length);
+        error_report("%s: ACL packet too short (%zuB)", __func__, length);
         return;
     }
 

@@ -2046,7 +2046,7 @@ static void bt_submit_acl(struct HCIInfo *info,
     handle &= ~HCI_HANDLE_OFFSET;
 
     if (datalen > length) {
-        error_report("%s: ACL packet too short (%iB < %iB)",
+        error_report("%s: ACL packet too short (%zuB < %iB)",
                      __func__, length, datalen);
         return;
     }

@@ -2088,7 +2088,7 @@ static void bt_submit_acl(struct HCIInfo *info,
 }
 
 static void bt_submit_sco(struct HCIInfo *info,
-                const uint8_t *data, int length)
+                const uint8_t *data, size_t length)
 {
     struct bt_hci_s *hci = hci_from_info(info);
     uint16_t handle;

@@ -2107,7 +2107,7 @@ static void bt_submit_sco(struct HCIInfo *info,
     }
 
     if (datalen > length) {
-        error_report("%s: SCO packet too short (%iB < %iB)",
+        error_report("%s: SCO packet too short (%zuB < %iB)",
                      __func__, length, datalen);
         return;
     }

@@ -2128,7 +2128,7 @@ static uint8_t *bt_hci_evt_packet(void *opaque)
     return s->evt_buf;
 }
 
-static void bt_hci_evt_submit(void *opaque, int len)
+static void bt_hci_evt_submit(void *opaque, size_t len)
 {
     /* TODO: notify upper layer */
     struct bt_hci_s *s = opaque;
hw/bt/hid.c +5 -5
index 056291f9b5..c5ecc8bdcd 100644
@@ -96,7 +96,7 @@ struct bt_hid_device_s {
     int data_type;
     int intr_state;
     struct {
-        int len;
+        size_t len;
         uint8_t buffer[1024];
     } dataother, datain, dataout, feature, intrdataout;
     enum {

@@ -169,7 +169,7 @@ static void bt_hid_disconnect(struct bt_hid_device_s *s)
 }
 
 static void bt_hid_send_data(struct bt_l2cap_conn_params_s *ch, int type,
-                const uint8_t *data, int len)
+                const uint8_t *data, size_t len)
 {
     uint8_t *pkt, hdr = (BT_DATA << 4) | type;
     int plen;

@@ -190,7 +190,7 @@ static void bt_hid_send_data(struct bt_l2cap_conn_params_s *ch, int type,
 }
 
 static void bt_hid_control_transaction(struct bt_hid_device_s *s,
-                const uint8_t *data, int len)
+                const uint8_t *data, size_t len)
 {
     uint8_t type, parameter;
     int rlen, ret = -1;

@@ -362,7 +362,7 @@ static void bt_hid_control_transaction(struct bt_hid_device_s *s,
         bt_hid_send_handshake(s, ret);
 }
 
-static void bt_hid_control_sdu(void *opaque, const uint8_t *data, int len)
+static void bt_hid_control_sdu(void *opaque, const uint8_t *data, size_t len)
 {
     struct bt_hid_device_s *hid = opaque;
 

@@ -388,7 +388,7 @@ static void bt_hid_datain(HIDState *hs)
                         hid->datain.buffer, hid->datain.len);
 }
 
-static void bt_hid_interrupt_sdu(void *opaque, const uint8_t *data, int len)
+static void bt_hid_interrupt_sdu(void *opaque, const uint8_t *data, size_t len)
 {
     struct bt_hid_device_s *hid = opaque;
hw/bt/l2cap.c +29 -27

202 lines not on this page · raw message

hw/bt/sdp.c +3 -3
index f4aba9d74f..163d315874 100644
@@ -497,7 +497,7 @@ static ssize_t sdp_svc_search_attr_get(struct bt_l2cap_sdp_state_s *sdp,
     return end + 2;
 }
 
-static void bt_l2cap_sdp_sdu_in(void *opaque, const uint8_t *data, int len)
+static void bt_l2cap_sdp_sdu_in(void *opaque, const uint8_t *data, size_t len)
 {
     struct bt_l2cap_sdp_state_s *sdp = opaque;
     enum bt_sdp_cmd pdu_id;

@@ -507,7 +507,7 @@ static void bt_l2cap_sdp_sdu_in(void *opaque, const uint8_t *data, int len)
     int rsp_len = 0;
 
     if (len < 5) {
-        error_report("%s: short SDP PDU (%iB).", __func__, len);
+        error_report("%s: short SDP PDU (%zuB).", __func__, len);
         return;
     }
 

@@ -518,7 +518,7 @@ static void bt_l2cap_sdp_sdu_in(void *opaque, const uint8_t *data, int len)
     len -= 5;
 
     if (len != plen) {
-        error_report("%s: wrong SDP PDU length (%iB != %iB).",
+        error_report("%s: wrong SDP PDU length (%iB != %zuB).",
                         __func__, plen, len);
         err = SDP_INVALID_PDU_SIZE;
         goto respond;
hw/usb/dev-bluetooth.c +6 -6
index eac7365b0a..cf46ba06c6 100644
@@ -265,7 +265,7 @@ static void usb_bt_fifo_reset(struct usb_hci_in_fifo_s *fifo)
 }
 
 static void usb_bt_fifo_enqueue(struct usb_hci_in_fifo_s *fifo,
-                const uint8_t *data, int len)
+                const uint8_t *data, size_t len)
 {
     int off = fifo->dstart + fifo->dlen;
     uint8_t *buf;

@@ -274,13 +274,13 @@ static void usb_bt_fifo_enqueue(struct usb_hci_in_fifo_s *fifo,
     if (off <= DFIFO_LEN_MASK) {
         if (off + len > DFIFO_LEN_MASK + 1 &&
                         (fifo->dsize = off + len) > (DFIFO_LEN_MASK + 1) * 2) {
-            fprintf(stderr, "%s: can't alloc %i bytes\n", __func__, len);
+            fprintf(stderr, "%s: can't alloc %zu bytes\n", __func__, len);
             exit(-1);
         }
         buf = fifo->data + off;
     } else {
         if (fifo->dlen > fifo->dsize) {
-            fprintf(stderr, "%s: can't alloc %i bytes\n", __func__, len);
+            fprintf(stderr, "%s: can't alloc %zu bytes\n", __func__, len);
             exit(-1);
         }
         buf = fifo->data + off - fifo->dsize;

@@ -319,7 +319,7 @@ static inline void usb_bt_fifo_dequeue(struct usb_hci_in_fifo_s *fifo,
 
 static inline void usb_bt_fifo_out_enqueue(struct USBBtState *s,
                 struct usb_hci_out_fifo_s *fifo,
-                void (*send)(struct HCIInfo *, const uint8_t *, int),
+                void (*send)(struct HCIInfo *, const uint8_t *, size_t),
                 int (*complete)(const uint8_t *, int),
                 USBPacket *p)
 {

@@ -478,7 +478,7 @@ static void usb_bt_handle_data(USBDevice *dev, USBPacket *p)
 }
 
 static void usb_bt_out_hci_packet_event(void *opaque,
-                const uint8_t *data, int len)
+                const uint8_t *data, size_t len)
 {
     struct USBBtState *s = (struct USBBtState *) opaque;
 

@@ -489,7 +489,7 @@ static void usb_bt_out_hci_packet_event(void *opaque,
 }
 
 static void usb_bt_out_hci_packet_acl(void *opaque,
-                const uint8_t *data, int len)
+                const uint8_t *data, size_t len)
 {
     struct USBBtState *s = (struct USBBtState *) opaque;
include/hw/bt.h +4 -4
index b5e11d4d43..bc362aa662 100644
@@ -94,9 +94,9 @@ struct bt_device_s {
     void (*lmp_disconnect_master)(struct bt_link_s *link);
     void (*lmp_disconnect_slave)(struct bt_link_s *link);
     void (*lmp_acl_data)(struct bt_link_s *link, const uint8_t *data,
-                    int start, int len);
+                    int start, size_t len);
     void (*lmp_acl_resp)(struct bt_link_s *link, const uint8_t *data,
-                    int start, int len);
+                    int start, size_t len);
     void (*lmp_mode_change)(struct bt_link_s *link);
 
     void (*handle_destroy)(struct bt_device_s *device);

@@ -148,12 +148,12 @@ struct bt_l2cap_device_s {
 
 struct bt_l2cap_conn_params_s {
     /* Input */
-    uint8_t *(*sdu_out)(struct bt_l2cap_conn_params_s *chan, int len);
+    uint8_t *(*sdu_out)(struct bt_l2cap_conn_params_s *chan, size_t len);
     void (*sdu_submit)(struct bt_l2cap_conn_params_s *chan);
     int remote_mtu;
     /* Output */
     void *opaque;
-    void (*sdu_in)(void *opaque, const uint8_t *data, int len);
+    void (*sdu_in)(void *opaque, const uint8_t *data, size_t len);
     void (*close)(void *opaque);
 };
include/sysemu/bt.h +5 -5
index ddb05cd109..db935c695d 100644
@@ -5,12 +5,12 @@
 
 struct HCIInfo {
     int (*bdaddr_set)(struct HCIInfo *hci, const uint8_t *bd_addr);
-    void (*cmd_send)(struct HCIInfo *hci, const uint8_t *data, int len);
-    void (*sco_send)(struct HCIInfo *hci, const uint8_t *data, int len);
-    void (*acl_send)(struct HCIInfo *hci, const uint8_t *data, int len);
+    void (*cmd_send)(struct HCIInfo *hci, const uint8_t *data, size_t len);
+    void (*sco_send)(struct HCIInfo *hci, const uint8_t *data, size_t len);
+    void (*acl_send)(struct HCIInfo *hci, const uint8_t *data, size_t len);
     void *opaque;
-    void (*evt_recv)(void *opaque, const uint8_t *data, int len);
-    void (*acl_recv)(void *opaque, const uint8_t *data, int len);
+    void (*evt_recv)(void *opaque, const uint8_t *data, size_t len);
+    void (*acl_recv)(void *opaque, const uint8_t *data, size_t len);
 };
 
 /* bt-host.c */
Signature · 1 line
2.17.2
author
#1 lore raw
+-- On Mon, 19 Nov 2018, P J P wrote --+
| From: Prasad J Pandit <[email protected]>
| 
| The length parameter values are not negative, thus use an unsigned
| type 'size_t' for them. Many routines pass 'len' values to memcpy(3)
| calls. If it was negative, it could lead to memory corruption issues.
| Add check to avoid it.
| 
| Reported-by: Arash TC <[email protected]>
| Signed-off-by: Prasad J Pandit <[email protected]>
| ---
|  bt-host.c              |  8 +++---
|  bt-vhci.c              |  7 +++---
|  hw/bt/core.c           |  2 +-
|  hw/bt/hci-csr.c        | 32 ++++++++++++------------
|  hw/bt/hci.c            | 38 ++++++++++++++--------------
|  hw/bt/hid.c            | 10 ++++----
|  hw/bt/l2cap.c          | 56 ++++++++++++++++++++++--------------------
|  hw/bt/sdp.c            |  6 ++---
|  hw/usb/dev-bluetooth.c | 12 ++++-----
|  include/hw/bt.h        |  8 +++---
|  include/sysemu/bt.h    | 10 ++++----
|  11 files changed, 96 insertions(+), 93 deletions(-)
| 
| Update v2: modify assert calls
|   -> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg01036.html
|

Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Reviewed-by on v2 1/1
#2 lore raw
In reply to P J P
Quoted 23 lines · P J P

From: Prasad J Pandit <[email protected]>

The length parameter values are not negative, thus use an unsigned type 'size_t' for them. Many routines pass 'len' values to memcpy(3) calls. If it was negative, it could lead to memory corruption issues. Add check to avoid it.

Reported-by Arash TC <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 bt-host.c              |  8 +++---
 bt-vhci.c              |  7 +++---
 hw/bt/core.c           |  2 +-
 hw/bt/hci-csr.c        | 32 ++++++++++++------------
 hw/bt/hci.c            | 38 ++++++++++++++--------------
 hw/bt/hid.c            | 10 ++++----
 hw/bt/l2cap.c          | 56 ++++++++++++++++++++++--------------------
 hw/bt/sdp.c            |  6 ++---
 hw/usb/dev-bluetooth.c | 12 ++++-----
 include/hw/bt.h        |  8 +++---
 include/sysemu/bt.h    | 10 ++++----
 11 files changed, 96 insertions(+), 93 deletions(-)
Quoted 2 lines

Update v2: modify assert calls
-> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg01036.html

Reviewed-by Thomas Huth <[email protected]>

Even though it's a rather big patch, I think the changes are trivial enough so that it could go via qemu-trivial (now on CC:). If not, maybe Paolo could take it through his "misc" tree?

Thomas

#3 lore raw
Quoted 24 lines · Thomas Huth
Quoted 23 lines · P J P

From: Prasad J Pandit <[email protected]>

The length parameter values are not negative, thus use an unsigned type 'size_t' for them. Many routines pass 'len' values to memcpy(3) calls. If it was negative, it could lead to memory corruption issues. Add check to avoid it.

Reported-by Arash TC <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 bt-host.c              |  8 +++---
 bt-vhci.c              |  7 +++---
 hw/bt/core.c           |  2 +-
 hw/bt/hci-csr.c        | 32 ++++++++++++------------
 hw/bt/hci.c            | 38 ++++++++++++++--------------
 hw/bt/hid.c            | 10 ++++----
 hw/bt/l2cap.c          | 56 ++++++++++++++++++++++--------------------
 hw/bt/sdp.c            |  6 ++---
 hw/usb/dev-bluetooth.c | 12 ++++-----
 include/hw/bt.h        |  8 +++---
 include/sysemu/bt.h    | 10 ++++----
 11 files changed, 96 insertions(+), 93 deletions(-)
Quoted 8 lines
Quoted 2 lines

Update v2: modify assert calls
-> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg01036.html

Reviewed-by Thomas Huth <[email protected]>

Even though it's a rather big patch, I think the changes are trivial enough so that it could go via qemu-trivial (now on CC:). If not, maybe Paolo could take it through his "misc" tree?

I'll take this patch via qemu-trivial only if I get clearance from Paolo. BTW, I don't plan any qemu-trivial pull request before the final rc.

Thanks,
Laurent

#4 lore raw
In reply to P J P

Hi,

Quoted 4 lines

The length parameter values are not negative, thus use an unsigned type 'size_t' for them. Many routines pass 'len' values to memcpy(3) calls. If it was negative, it could lead to memory corruption issues. Add check to avoid it.

I'm working on a Debian LTS security update for qemu and am currently thinking about addressing this issue as well.

I see this patch has not been applied yet and the bluetooth subsystem is pending deprecation. Are you still considering to apply it?

bt-vhci.c static void vhci_host_send(void *opaque, · line 119

Quoted 8 lines
bt-vhci.c +1 -0
@@ -113,6 +113,7 @@ static void vhci_host_send(void *opaque,
     static uint8_t buf[4096];
 
     buf[0] = type;
+    assert(len < sizeof(buf));
     memcpy(buf + 1, data, len);
 
     while (write(s->fd, buf, len + 1) < 0)

Any reason why assert() calls are used here ?

These checks should always be executed, but they won't if user compiles without asserts. Also, AFAIK any assert failure will stop the qemu host process which is not what we want in this case.

regards,
Hugo

Signature · 3 lines
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C
#5 lore raw

Hi Hugo,

Quoted 7 lines · Hugo Lefeuvre

Hi,

Quoted 4 lines

The length parameter values are not negative, thus use an unsigned type 'size_t' for them. Many routines pass 'len' values to memcpy(3) calls. If it was negative, it could lead to memory corruption issues. Add check to avoid it.

Quoted 5 lines

I'm working on a Debian LTS security update for qemu and am currently thinking about addressing this issue as well.

I see this patch has not been applied yet and the bluetooth subsystem is pending deprecation. Are you still considering to apply it?

I have been assigned to fix this issue, but rather fixing locally this BT device, fix the pattern on all devices.
I'll post the series during the week and Cc you (and eventually the Debian LTS list when it gets merged). The series obsoletes this patch, so the plan is to not apply it.

Quoted 8 lines
Quoted 8 lines
bt-vhci.c +1 -0
@@ -113,6 +113,7 @@ static void vhci_host_send(void *opaque,
     static uint8_t buf[4096];
 
     buf[0] = type;
+    assert(len < sizeof(buf));
     memcpy(buf + 1, data, len);
Quoted 7 lines

bt-vhci.c static void vhci_host_send(void *opaque, · line 119

Quoted 1 line
bt-vhci.c +0 -0
     while (write(s->fd, buf, len + 1) < 0)

Any reason why assert() calls are used here ?

These checks should always be executed, but they won't if user compiles without asserts. Also, AFAIK any assert failure will stop the qemu host process which is not what we want in this case.

There was a discussion about this, and the outcome is QEMU does not support building without assertions. See this commit:

https://git.qemu.org/?p=qemu.git;a=blobdiff;f=include/qemu/osdep.h;h=9966638;hp=6855b94;hb=262a69f42;hpb=825bfa005

Regards,

Phil.

#6 lore raw

Hi Philippe,

Quoted 5 lines

I have been assigned to fix this issue, but rather fixing locally this BT device, fix the pattern on all devices.
I'll post the series during the week and Cc you (and eventually the Debian LTS list when it gets merged). The series obsoletes this patch, so the plan is to not apply it.

Thanks ! I will wait for your patch then.

Quoted 10 lines
Quoted 5 lines

Any reason why assert() calls are used here ?

These checks should always be executed, but they won't if user compiles without asserts. Also, AFAIK any assert failure will stop the qemu host process which is not what we want in this case.

There was a discussion about this, and the outcome is QEMU does not support building without assertions. See this commit:

https://git.qemu.org/?p=qemu.git;a=blobdiff;f=include/qemu/osdep.h;h=9966638;hp=6855b94;hb=262a69f42;hpb=825bfa005

Makes sense. But I'm still sceptical about assert() being used here.

For example:

    @@ -113,6 +113,7 @@ static void vhci_host_send(void *opaque,
         static uint8_t buf[4096];
     
         buf[0] = type;
    +    assert(len < sizeof(buf));
         memcpy(buf + 1, data, len);
     
         while (write(s->fd, buf, len + 1) < 0)

From my understanding assert() calls are supposed to be a way to verify code assumptions at runtime. assert failures are always bugs, so they terminate the process.

If len is passed by guest systems, an excessive value should not be considered a bug but invalid user passed input, that is normal behaviour, right? In this case I expect qemu to simply reject the input instead of triggering an assert failure and terminating.

regards,
Hugo

Signature · 3 lines
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C