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(-)
From: Prasad J Pandit <pjp@fedoraproject.org>
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 <tohidi.arash@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
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
diff --git a/bt-host.c b/bt-host.c
index 2f8f631c25..b73a44d07d 100644
--- a/bt-host.c
+++ b/bt-host.c
@@ -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);
}
diff --git a/bt-vhci.c b/bt-vhci.c
index 9d277c32bf..765773188d 100644
--- a/bt-vhci.c
+++ b/bt-vhci.c
@@ -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);
}
diff --git a/hw/bt/core.c b/hw/bt/core.c
index 78370e64f5..62720d1663 100644
--- a/hw/bt/core.c
+++ b/hw/bt/core.c
@@ -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);
diff --git a/hw/bt/hci-csr.c b/hw/bt/hci-csr.c
index 0341ded50c..26bd516d31 100644
--- a/hw/bt/hci-csr.c
+++ b/hw/bt/hci-csr.c
@@ -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 */
diff --git a/hw/bt/hci.c b/hw/bt/hci.c
index c6b2cc1d48..c59ccc55b9 100644
--- a/hw/bt/hci.c
+++ b/hw/bt/hci.c
@@ -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;
diff --git a/hw/bt/hid.c b/hw/bt/hid.c
index 056291f9b5..c5ecc8bdcd 100644
--- a/hw/bt/hid.c
+++ b/hw/bt/hid.c
@@ -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;
diff --git a/hw/bt/l2cap.c b/hw/bt/l2cap.c
index 9cf27f0df6..efd9a4b66a 100644
--- a/hw/bt/l2cap.c
+++ b/hw/bt/l2cap.c
@@ -32,10 +32,10 @@ struct l2cap_instance_s {
int role;
uint8_t frame_in[65535 + L2CAP_HDR_SIZE] __attribute__ ((aligned (4)));
- int frame_in_len;
+ uint32_t frame_in_len;
uint8_t frame_out[65535 + L2CAP_HDR_SIZE] __attribute__ ((aligned (4)));
- int frame_out_len;
+ uint32_t frame_out_len;
/* Signalling channel timers. They exist per-request but we can make
* sure we have no more than one outstanding request at any time. */
@@ -49,7 +49,7 @@ struct l2cap_instance_s {
struct bt_l2cap_conn_params_s params;
void (*frame_in)(struct l2cap_chan_s *chan, uint16_t cid,
- const l2cap_hdr *hdr, int len);
+ const l2cap_hdr *hdr, size_t len);
int mps;
int min_mtu;
@@ -68,7 +68,7 @@ struct l2cap_instance_s {
/* Only flow-controlled, connection-oriented channels */
uint8_t sdu[65536]; /* TODO: dynamically allocate */
- int len_cur, len_total;
+ uint32_t len_cur, len_total;
int rexmit;
int monitor_timeout;
QEMUTimer *monitor_timer;
@@ -140,7 +140,7 @@ static const uint16_t l2cap_fcs16_table[256] = {
0x8201, 0x42c0, 0x4380, 0x8341, 0x4100, 0x81c1, 0x8081, 0x4040,
};
-static uint16_t l2cap_fcs16(const uint8_t *message, int len)
+static uint16_t l2cap_fcs16(const uint8_t *message, size_t len)
{
uint16_t fcs = 0x0000;
@@ -186,7 +186,7 @@ static void l2cap_monitor_timer_update(struct l2cap_chan_s *ch)
}
static void l2cap_command_reject(struct l2cap_instance_s *l2cap, int id,
- uint16_t reason, const void *data, int plen)
+ uint16_t reason, const void *data, size_t plen)
{
uint8_t *pkt;
l2cap_cmd_hdr *hdr;
@@ -247,7 +247,7 @@ static void l2cap_connection_response(struct l2cap_instance_s *l2cap,
}
static void l2cap_configuration_request(struct l2cap_instance_s *l2cap,
- int dcid, int flag, const uint8_t *data, int len)
+ int dcid, int flag, const uint8_t *data, size_t len)
{
uint8_t *pkt;
l2cap_cmd_hdr *hdr;
@@ -275,7 +275,7 @@ static void l2cap_configuration_request(struct l2cap_instance_s *l2cap,
}
static void l2cap_configuration_response(struct l2cap_instance_s *l2cap,
- int scid, int flag, int result, const uint8_t *data, int len)
+ int scid, int flag, int result, const uint8_t *data, size_t len)
{
uint8_t *pkt;
l2cap_cmd_hdr *hdr;
@@ -322,7 +322,7 @@ static void l2cap_disconnection_response(struct l2cap_instance_s *l2cap,
}
static void l2cap_echo_response(struct l2cap_instance_s *l2cap,
- const uint8_t *data, int len)
+ const uint8_t *data, size_t len)
{
uint8_t *pkt;
l2cap_cmd_hdr *hdr;
@@ -343,7 +343,7 @@ static void l2cap_echo_response(struct l2cap_instance_s *l2cap,
}
static void l2cap_info_response(struct l2cap_instance_s *l2cap, int type,
- int result, const uint8_t *data, int len)
+ int result, const uint8_t *data, size_t len)
{
uint8_t *pkt;
l2cap_cmd_hdr *hdr;
@@ -366,16 +366,18 @@ static void l2cap_info_response(struct l2cap_instance_s *l2cap, int type,
l2cap->signalling_ch.params.sdu_submit(&l2cap->signalling_ch.params);
}
-static uint8_t *l2cap_bframe_out(struct bt_l2cap_conn_params_s *parm, int len);
+static uint8_t *l2cap_bframe_out(struct bt_l2cap_conn_params_s *parm,
+ size_t len);
static void l2cap_bframe_submit(struct bt_l2cap_conn_params_s *parms);
#if 0
-static uint8_t *l2cap_iframe_out(struct bt_l2cap_conn_params_s *parm, int len);
+static uint8_t *l2cap_iframe_out(struct bt_l2cap_conn_params_s *parm,
+ size_t len);
static void l2cap_iframe_submit(struct bt_l2cap_conn_params_s *parm);
#endif
static void l2cap_bframe_in(struct l2cap_chan_s *ch, uint16_t cid,
- const l2cap_hdr *hdr, int len);
+ const l2cap_hdr *hdr, size_t len);
static void l2cap_iframe_in(struct l2cap_chan_s *ch, uint16_t cid,
- const l2cap_hdr *hdr, int len);
+ const l2cap_hdr *hdr, size_t len);
static int l2cap_cid_new(struct l2cap_instance_s *l2cap)
{
@@ -499,7 +501,7 @@ static void l2cap_channel_config_req_event(struct l2cap_instance_s *l2cap,
static int l2cap_channel_config(struct l2cap_instance_s *l2cap,
struct l2cap_chan_s *ch, int flag,
- const uint8_t *data, int len)
+ const uint8_t *data, size_t len)
{
l2cap_conf_opt *opt;
l2cap_conf_opt_qos *qos;
@@ -684,7 +686,7 @@ static int l2cap_channel_config(struct l2cap_instance_s *l2cap,
}
static void l2cap_channel_config_req_msg(struct l2cap_instance_s *l2cap,
- int flag, int cid, const uint8_t *data, int len)
+ int flag, int cid, const uint8_t *data, size_t len)
{
struct l2cap_chan_s *ch;
@@ -716,7 +718,7 @@ static void l2cap_channel_config_req_msg(struct l2cap_instance_s *l2cap,
}
static int l2cap_channel_config_rsp_msg(struct l2cap_instance_s *l2cap,
- int result, int flag, int cid, const uint8_t *data, int len)
+ int result, int flag, int cid, const uint8_t *data, size_t len)
{
struct l2cap_chan_s *ch;
@@ -784,7 +786,7 @@ static void l2cap_info(struct l2cap_instance_s *l2cap, int type)
}
static void l2cap_command(struct l2cap_instance_s *l2cap, int code, int id,
- const uint8_t *params, int len)
+ const uint8_t *params, size_t len)
{
int err;
@@ -939,7 +941,7 @@ static void l2cap_rexmit_enable(struct l2cap_chan_s *ch, int enable)
}
/* Command frame SDU */
-static void l2cap_cframe_in(void *opaque, const uint8_t *data, int len)
+static void l2cap_cframe_in(void *opaque, const uint8_t *data, size_t len)
{
struct l2cap_instance_s *l2cap = opaque;
const l2cap_cmd_hdr *hdr;
@@ -967,7 +969,7 @@ static void l2cap_cframe_in(void *opaque, const uint8_t *data, int len)
}
/* Group frame SDU */
-static void l2cap_gframe_in(void *opaque, const uint8_t *data, int len)
+static void l2cap_gframe_in(void *opaque, const uint8_t *data, size_t len)
{
}
@@ -978,7 +980,7 @@ static void l2cap_sframe_in(struct l2cap_chan_s *ch, uint16_t ctrl)
/* Basic L2CAP mode Information frame */
static void l2cap_bframe_in(struct l2cap_chan_s *ch, uint16_t cid,
- const l2cap_hdr *hdr, int len)
+ const l2cap_hdr *hdr, size_t len)
{
/* We have a full SDU, no further processing */
ch->params.sdu_in(ch->params.opaque, hdr->data, len);
@@ -986,7 +988,7 @@ static void l2cap_bframe_in(struct l2cap_chan_s *ch, uint16_t cid,
/* Flow Control and Retransmission mode frame */
static void l2cap_iframe_in(struct l2cap_chan_s *ch, uint16_t cid,
- const l2cap_hdr *hdr, int len)
+ const l2cap_hdr *hdr, size_t len)
{
uint16_t fcs = lduw_le_p(hdr->data + len - 2);
@@ -1077,7 +1079,7 @@ static void l2cap_frame_in(struct l2cap_instance_s *l2cap,
/* "Recombination" */
static void l2cap_pdu_in(struct l2cap_instance_s *l2cap,
- const uint8_t *data, int len)
+ const uint8_t *data, size_t len)
{
const l2cap_hdr *hdr = (void *) l2cap->frame_in;
@@ -1124,7 +1126,7 @@ static inline void l2cap_pdu_submit(struct l2cap_instance_s *l2cap)
(l2cap->link, l2cap->frame_out, 1, l2cap->frame_out_len);
}
-static uint8_t *l2cap_bframe_out(struct bt_l2cap_conn_params_s *parm, int len)
+static uint8_t *l2cap_bframe_out(struct bt_l2cap_conn_params_s *parm, size_t len)
{
struct l2cap_chan_s *chan = (struct l2cap_chan_s *) parm;
@@ -1147,7 +1149,7 @@ static void l2cap_bframe_submit(struct bt_l2cap_conn_params_s *parms)
#if 0
/* Stub: Only used if an emulated device requests outgoing flow control */
-static uint8_t *l2cap_iframe_out(struct bt_l2cap_conn_params_s *parm, int len)
+static uint8_t *l2cap_iframe_out(struct bt_l2cap_conn_params_s *parm, size_t len)
{
struct l2cap_chan_s *chan = (struct l2cap_chan_s *) parm;
@@ -1292,7 +1294,7 @@ static void l2cap_lmp_disconnect_slave(struct bt_link_s *link)
}
static void l2cap_lmp_acl_data_slave(struct bt_link_s *link,
- const uint8_t *data, int start, int len)
+ const uint8_t *data, int start, size_t len)
{
struct slave_l2cap_instance_s *l2cap =
(struct slave_l2cap_instance_s *) link;
@@ -1305,7 +1307,7 @@ static void l2cap_lmp_acl_data_slave(struct bt_link_s *link,
/* Stub */
static void l2cap_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)
{
struct bt_l2cap_device_s *dev = (struct bt_l2cap_device_s *) link->host;
struct l2cap_instance_s *l2cap =
diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
index f4aba9d74f..163d315874 100644
--- a/hw/bt/sdp.c
+++ b/hw/bt/sdp.c
@@ -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;
diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c
index eac7365b0a..cf46ba06c6 100644
--- a/hw/usb/dev-bluetooth.c
+++ b/hw/usb/dev-bluetooth.c
@@ -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;
diff --git a/include/hw/bt.h b/include/hw/bt.h
index b5e11d4d43..bc362aa662 100644
--- a/include/hw/bt.h
+++ b/include/hw/bt.h
@@ -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);
};
diff --git a/include/sysemu/bt.h b/include/sysemu/bt.h
index ddb05cd109..db935c695d 100644
--- a/include/sysemu/bt.h
+++ b/include/sysemu/bt.h
@@ -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 */
--
2.17.2
+-- On Mon, 19 Nov 2018, P J P wrote --+ | From: Prasad J Pandit <pjp@fedoraproject.org> | | 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 <tohidi.arash@gmail.com> | Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> | --- | 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
On 2018-11-19 12:07, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > 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 <tohidi.arash@gmail.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > 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 Reviewed-by: Thomas Huth <thuth@redhat.com> 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
On 28/11/2018 09:53, Thomas Huth wrote: > On 2018-11-19 12:07, P J P wrote: >> From: Prasad J Pandit <pjp@fedoraproject.org> >> >> 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 <tohidi.arash@gmail.com> >> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >> --- >> 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 > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > 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
Hi,
> 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?
> @@ -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
--
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
Hi Hugo, On 1/28/19 10:31 AM, Hugo Lefeuvre wrote: > Hi, > >> 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? 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. > >> @@ -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. 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.
Hi Philippe,
> 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.
> > 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
--
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
© 2016 - 2024 Red Hat, Inc.