From eea750a5623ddac7a61982eec8f1c93481857578 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:50:56 +0300 Subject: virtio-net: out-of-bounds buffer write on invalid state load CVE-2013-4150 QEMU 1.5.0 out-of-bounds buffer write in virtio_net_load()@hw/net/virtio-net.c This code is in hw/net/virtio-net.c: if (n->max_queues > 1) { if (n->max_queues != qemu_get_be16(f)) { error_report("virtio-net: different max_queues "); return -1; } n->curr_queues = qemu_get_be16(f); for (i = 1; i < n->curr_queues; i++) { n->vqs[i].tx_waiting = qemu_get_be32(f); } } Number of vqs is max_queues, so if we get invalid input here, for example if max_queues = 2, curr_queues = 3, we get write beyond end of the buffer, with data that comes from wire. This might be used to corrupt qemu memory in hard to predict ways. Since we have lots of function pointers around, RCE might be possible. Signed-off-by: Michael S. Tsirkin Acked-by: Jason Wang Reviewed-by: Michael Roth Signed-off-by: Juan Quintela --- hw/net/virtio-net.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'hw') diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 33bd233..0a8cb40 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1407,6 +1407,11 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) } n->curr_queues = qemu_get_be16(f); + if (n->curr_queues > n->max_queues) { + error_report("virtio-net: curr_queues %x > max_queues %x", + n->curr_queues, n->max_queues); + return -1; + } for (i = 1; i < n->curr_queues; i++) { n->vqs[i].tx_waiting = qemu_get_be32(f); } -- cgit v1.1 From cc45995294b92d95319b4782750a3580cabdbc0c Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:51:14 +0300 Subject: virtio: out-of-bounds buffer write on invalid state load CVE-2013-4151 QEMU 1.0 out-of-bounds buffer write in virtio_load@hw/virtio/virtio.c So we have this code since way back when: num = qemu_get_be32(f); for (i = 0; i < num; i++) { vdev->vq[i].vring.num = qemu_get_be32(f); array of vqs has size VIRTIO_PCI_QUEUE_MAX, so on invalid input this will write beyond end of buffer. Signed-off-by: Michael S. Tsirkin Reviewed-by: Michael Roth Signed-off-by: Juan Quintela --- hw/virtio/virtio.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index aeabf3a..05f05e7 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -891,7 +891,8 @@ int virtio_set_features(VirtIODevice *vdev, uint32_t val) int virtio_load(VirtIODevice *vdev, QEMUFile *f) { - int num, i, ret; + int i, ret; + uint32_t num; uint32_t features; uint32_t supported_features; BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); @@ -919,6 +920,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) num = qemu_get_be32(f); + if (num > VIRTIO_PCI_QUEUE_MAX) { + error_report("Invalid number of PCI queues: 0x%x", num); + return -1; + } + for (i = 0; i < num; i++) { vdev->vq[i].vring.num = qemu_get_be32(f); if (k->has_variable_vring_alignment) { -- cgit v1.1 From ae2158ad6ce0845b2fae2a22aa7f19c0d7a71ce5 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:51:18 +0300 Subject: ahci: fix buffer overrun on invalid state load CVE-2013-4526 Within hw/ide/ahci.c, VARRAY refers to ports which is also loaded. So we use the old version of ports to read the array but then allow any value for ports. This can cause the code to overflow. There's no reason to migrate ports - it never changes. So just make sure it matches. Reported-by: Anthony Liguori Signed-off-by: Michael S. Tsirkin Reviewed-by: Peter Maydell Signed-off-by: Juan Quintela --- hw/ide/ahci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 50327ff..e57c583 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1293,7 +1293,7 @@ const VMStateDescription vmstate_ahci = { VMSTATE_UINT32(control_regs.impl, AHCIState), VMSTATE_UINT32(control_regs.version, AHCIState), VMSTATE_UINT32(idp_index, AHCIState), - VMSTATE_INT32(ports, AHCIState), + VMSTATE_INT32_EQUAL(ports, AHCIState), VMSTATE_END_OF_LIST() }, }; -- cgit v1.1 From 3f1c49e2136fa08ab1ef3183fd55def308829584 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:51:23 +0300 Subject: hpet: fix buffer overrun on invalid state load CVE-2013-4527 hw/timer/hpet.c buffer overrun hpet is a VARRAY with a uint8 size but static array of 32 To fix, make sure num_timers is valid using VMSTATE_VALID hook. Reported-by: Anthony Liguori Signed-off-by: Michael S. Tsirkin Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Juan Quintela --- hw/timer/hpet.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'hw') diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index e15d6bc..2792f89 100644 --- a/hw/timer/hpet.c +++ b/hw/timer/hpet.c @@ -239,6 +239,18 @@ static int hpet_pre_load(void *opaque) return 0; } +static bool hpet_validate_num_timers(void *opaque, int version_id) +{ + HPETState *s = opaque; + + if (s->num_timers < HPET_MIN_TIMERS) { + return false; + } else if (s->num_timers > HPET_MAX_TIMERS) { + return false; + } + return true; +} + static int hpet_post_load(void *opaque, int version_id) { HPETState *s = opaque; @@ -307,6 +319,7 @@ static const VMStateDescription vmstate_hpet = { VMSTATE_UINT64(isr, HPETState), VMSTATE_UINT64(hpet_counter, HPETState), VMSTATE_UINT8_V(num_timers, HPETState, 2), + VMSTATE_VALIDATE("num_timers in range", hpet_validate_num_timers), VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0, vmstate_hpet_timer, HPETTimer), VMSTATE_END_OF_LIST() -- cgit v1.1 From 5f691ff91d323b6f97c6600405a7f9dc115a0ad1 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:51:31 +0300 Subject: hw/pci/pcie_aer.c: fix buffer overruns on invalid state load 4) CVE-2013-4529 hw/pci/pcie_aer.c pcie aer log can overrun the buffer if log_num is too large There are two issues in this file: 1. log_max from remote can be larger than on local then buffer will overrun with data coming from state file. 2. log_num can be larger then we get data corruption again with an overflow but not adversary controlled. Fix both issues. Reported-by: Anthony Liguori Reported-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Juan Quintela --- hw/pci/pcie_aer.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c index 991502e..535be2c 100644 --- a/hw/pci/pcie_aer.c +++ b/hw/pci/pcie_aer.c @@ -795,6 +795,13 @@ static const VMStateDescription vmstate_pcie_aer_err = { } }; +static bool pcie_aer_state_log_num_valid(void *opaque, int version_id) +{ + PCIEAERLog *s = opaque; + + return s->log_num <= s->log_max; +} + const VMStateDescription vmstate_pcie_aer_log = { .name = "PCIE_AER_ERROR_LOG", .version_id = 1, @@ -802,7 +809,8 @@ const VMStateDescription vmstate_pcie_aer_log = { .minimum_version_id_old = 1, .fields = (VMStateField[]) { VMSTATE_UINT16(log_num, PCIEAERLog), - VMSTATE_UINT16(log_max, PCIEAERLog), + VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog), + VMSTATE_VALIDATE("log_num <= log_max", pcie_aer_state_log_num_valid), VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num, vmstate_pcie_aer_err, PCIEAERErr), VMSTATE_END_OF_LIST() -- cgit v1.1 From d8d0a0bc7e194300e53a346d25fe5724fd588387 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:51:35 +0300 Subject: pl022: fix buffer overun on invalid state load CVE-2013-4530 pl022.c did not bounds check tx_fifo_head and rx_fifo_head after loading them from file and before they are used to dereference array. Reported-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Signed-off-by: Juan Quintela --- hw/ssi/pl022.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'hw') diff --git a/hw/ssi/pl022.c b/hw/ssi/pl022.c index fd479ef..b19bc71 100644 --- a/hw/ssi/pl022.c +++ b/hw/ssi/pl022.c @@ -240,11 +240,25 @@ static const MemoryRegionOps pl022_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; +static int pl022_post_load(void *opaque, int version_id) +{ + PL022State *s = opaque; + + if (s->tx_fifo_head < 0 || + s->tx_fifo_head >= ARRAY_SIZE(s->tx_fifo) || + s->rx_fifo_head < 0 || + s->rx_fifo_head >= ARRAY_SIZE(s->rx_fifo)) { + return -1; + } + return 0; +} + static const VMStateDescription vmstate_pl022 = { .name = "pl022_ssp", .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, + .post_load = pl022_post_load, .fields = (VMStateField[]) { VMSTATE_UINT32(cr0, PL022State), VMSTATE_UINT32(cr1, PL022State), -- cgit v1.1 From 4b53c2c72cb5541cf394033b528a6fe2a86c0ac1 Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Thu, 3 Apr 2014 19:51:46 +0300 Subject: virtio: avoid buffer overrun on incoming migration CVE-2013-6399 vdev->queue_sel is read from the wire, and later used in the emulation code as an index into vdev->vq[]. If the value of vdev->queue_sel exceeds the length of vdev->vq[], currently allocated to be VIRTIO_PCI_QUEUE_MAX elements, subsequent PIO operations such as VIRTIO_PCI_QUEUE_PFN can be used to overrun the buffer with arbitrary data originating from the source. Fix this by failing migration if the value from the wire exceeds VIRTIO_PCI_QUEUE_MAX. Signed-off-by: Michael Roth Signed-off-by: Michael S. Tsirkin Reviewed-by: Peter Maydell Signed-off-by: Juan Quintela --- hw/virtio/virtio.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'hw') diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 05f05e7..0072542 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -907,6 +907,9 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) qemu_get_8s(f, &vdev->status); qemu_get_8s(f, &vdev->isr); qemu_get_be16s(f, &vdev->queue_sel); + if (vdev->queue_sel >= VIRTIO_PCI_QUEUE_MAX) { + return -1; + } qemu_get_be32s(f, &features); if (virtio_set_features(vdev, features) < 0) { -- cgit v1.1 From 36cf2a37132c7f01fa9adb5f95f5312b27742fd4 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:51:53 +0300 Subject: virtio: validate num_sg when mapping CVE-2013-4535 CVE-2013-4536 Both virtio-block and virtio-serial read, VirtQueueElements are read in as buffers, and passed to virtqueue_map_sg(), where num_sg is taken from the wire and can force writes to indicies beyond VIRTQUEUE_MAX_SIZE. To fix, validate num_sg. Reported-by: Michael Roth Signed-off-by: Michael S. Tsirkin Cc: Amit Shah Signed-off-by: Juan Quintela --- hw/virtio/virtio.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'hw') diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 0072542..a70169a 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -430,6 +430,12 @@ void virtqueue_map_sg(struct iovec *sg, hwaddr *addr, unsigned int i; hwaddr len; + if (num_sg >= VIRTQUEUE_MAX_SIZE) { + error_report("virtio: map attempt out of bounds: %zd > %d", + num_sg, VIRTQUEUE_MAX_SIZE); + exit(1); + } + for (i = 0; i < num_sg; i++) { len = sg[i].iov_len; sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write); -- cgit v1.1 From caa881abe0e01f9931125a0977ec33c5343e4aa7 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:51:57 +0300 Subject: pxa2xx: avoid buffer overrun on incoming migration CVE-2013-4533 s->rx_level is read from the wire and used to determine how many bytes to subsequently read into s->rx_fifo[]. If s->rx_level exceeds the length of s->rx_fifo[] the buffer can be overrun with arbitrary data from the wire. Fix this by validating rx_level against the size of s->rx_fifo. Cc: Don Koch Reported-by: Michael Roth Signed-off-by: Michael S. Tsirkin Reviewed-by: Peter Maydell Reviewed-by: Don Koch Signed-off-by: Juan Quintela --- hw/arm/pxa2xx.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'hw') diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c index 0429148..e0cd847 100644 --- a/hw/arm/pxa2xx.c +++ b/hw/arm/pxa2xx.c @@ -732,7 +732,7 @@ static void pxa2xx_ssp_save(QEMUFile *f, void *opaque) static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id) { PXA2xxSSPState *s = (PXA2xxSSPState *) opaque; - int i; + int i, v; s->enable = qemu_get_be32(f); @@ -746,7 +746,11 @@ static int pxa2xx_ssp_load(QEMUFile *f, void *opaque, int version_id) qemu_get_8s(f, &s->ssrsa); qemu_get_8s(f, &s->ssacd); - s->rx_level = qemu_get_byte(f); + v = qemu_get_byte(f); + if (v < 0 || v > ARRAY_SIZE(s->rx_fifo)) { + return -EINVAL; + } + s->rx_level = v; s->rx_start = 0; for (i = 0; i < s->rx_level; i ++) s->rx_fifo[i] = qemu_get_byte(f); -- cgit v1.1 From ead7a57df37d2187813a121308213f41591bd811 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:52:05 +0300 Subject: ssd0323: fix buffer overun on invalid state load CVE-2013-4538 s->cmd_len used as index in ssd0323_transfer() to store 32-bit field. Possible this field might then be supplied by guest to overwrite a return addr somewhere. Same for row/col fields, which are indicies into framebuffer array. To fix validate after load. Additionally, validate that the row/col_start/end are within bounds; otherwise the guest can provoke an overrun by either setting the _end field so large that the row++ increments just walk off the end of the array, or by setting the _start value to something bogus and then letting the "we hit end of row" logic reset row to row_start. For completeness, validate mode as well. Signed-off-by: Michael S. Tsirkin Reviewed-by: Peter Maydell Signed-off-by: Juan Quintela --- hw/display/ssd0323.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'hw') diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c index 971152e..9727007 100644 --- a/hw/display/ssd0323.c +++ b/hw/display/ssd0323.c @@ -312,18 +312,42 @@ static int ssd0323_load(QEMUFile *f, void *opaque, int version_id) return -EINVAL; s->cmd_len = qemu_get_be32(f); + if (s->cmd_len < 0 || s->cmd_len > ARRAY_SIZE(s->cmd_data)) { + return -EINVAL; + } s->cmd = qemu_get_be32(f); for (i = 0; i < 8; i++) s->cmd_data[i] = qemu_get_be32(f); s->row = qemu_get_be32(f); + if (s->row < 0 || s->row >= 80) { + return -EINVAL; + } s->row_start = qemu_get_be32(f); + if (s->row_start < 0 || s->row_start >= 80) { + return -EINVAL; + } s->row_end = qemu_get_be32(f); + if (s->row_end < 0 || s->row_end >= 80) { + return -EINVAL; + } s->col = qemu_get_be32(f); + if (s->col < 0 || s->col >= 64) { + return -EINVAL; + } s->col_start = qemu_get_be32(f); + if (s->col_start < 0 || s->col_start >= 64) { + return -EINVAL; + } s->col_end = qemu_get_be32(f); + if (s->col_end < 0 || s->col_end >= 64) { + return -EINVAL; + } s->redraw = qemu_get_be32(f); s->remap = qemu_get_be32(f); s->mode = qemu_get_be32(f); + if (s->mode != SSD0323_CMD && s->mode != SSD0323_DATA) { + return -EINVAL; + } qemu_get_buffer(f, s->framebuffer, sizeof(s->framebuffer)); ss->cs = qemu_get_be32(f); -- cgit v1.1 From 5193be3be35f29a35bc465036cd64ad60d43385f Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:52:09 +0300 Subject: tsc210x: fix buffer overrun on invalid state load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CVE-2013-4539 s->precision, nextprecision, function and nextfunction come from wire and are used as idx into resolution[] in TSC_CUT_RESOLUTION. Validate after load to avoid buffer overrun. Cc: Andreas Färber Signed-off-by: Michael S. Tsirkin Signed-off-by: Juan Quintela --- hw/input/tsc210x.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'hw') diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c index 485c9e5..aa5b688 100644 --- a/hw/input/tsc210x.c +++ b/hw/input/tsc210x.c @@ -1070,9 +1070,21 @@ static int tsc210x_load(QEMUFile *f, void *opaque, int version_id) s->enabled = qemu_get_byte(f); s->host_mode = qemu_get_byte(f); s->function = qemu_get_byte(f); + if (s->function < 0 || s->function >= ARRAY_SIZE(mode_regs)) { + return -EINVAL; + } s->nextfunction = qemu_get_byte(f); + if (s->nextfunction < 0 || s->nextfunction >= ARRAY_SIZE(mode_regs)) { + return -EINVAL; + } s->precision = qemu_get_byte(f); + if (s->precision < 0 || s->precision >= ARRAY_SIZE(resolution)) { + return -EINVAL; + } s->nextprecision = qemu_get_byte(f); + if (s->nextprecision < 0 || s->nextprecision >= ARRAY_SIZE(resolution)) { + return -EINVAL; + } s->filter = qemu_get_byte(f); s->pin_func = qemu_get_byte(f); s->ref = qemu_get_byte(f); -- cgit v1.1 From 52f91c3723932f8340fe36c8ec8b18a757c37b2b Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:52:13 +0300 Subject: zaurus: fix buffer overrun on invalid state load CVE-2013-4540 Within scoop_gpio_handler_update, if prev_level has a high bit set, then we get bit > 16 and that causes a buffer overrun. Since prev_level comes from wire indirectly, this can happen on invalid state load. Similarly for gpio_level and gpio_dir. To fix, limit to 16 bit. Reported-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Juan Quintela --- hw/gpio/zaurus.c | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'hw') diff --git a/hw/gpio/zaurus.c b/hw/gpio/zaurus.c index dc79a8ba..8e2ce04 100644 --- a/hw/gpio/zaurus.c +++ b/hw/gpio/zaurus.c @@ -203,6 +203,15 @@ static bool is_version_0 (void *opaque, int version_id) return version_id == 0; } +static bool vmstate_scoop_validate(void *opaque, int version_id) +{ + ScoopInfo *s = opaque; + + return !(s->prev_level & 0xffff0000) && + !(s->gpio_level & 0xffff0000) && + !(s->gpio_dir & 0xffff0000); +} + static const VMStateDescription vmstate_scoop_regs = { .name = "scoop", .version_id = 1, @@ -215,6 +224,7 @@ static const VMStateDescription vmstate_scoop_regs = { VMSTATE_UINT32(gpio_level, ScoopInfo), VMSTATE_UINT32(gpio_dir, ScoopInfo), VMSTATE_UINT32(prev_level, ScoopInfo), + VMSTATE_VALIDATE("irq levels are 16 bit", vmstate_scoop_validate), VMSTATE_UINT16(mcr, ScoopInfo), VMSTATE_UINT16(cdr, ScoopInfo), VMSTATE_UINT16(ccr, ScoopInfo), -- cgit v1.1 From 3c3ce981423e0d6c18af82ee62f1850c2cda5976 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:52:17 +0300 Subject: virtio-scsi: fix buffer overrun on invalid state load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CVE-2013-4542 hw/scsi/scsi-bus.c invokes load_request. virtio_scsi_load_request does: qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem)); this probably can make elem invalid, for example, make in_num or out_num huge, then: virtio_scsi_parse_req(s, vs->cmd_vqs[n], req); will do: if (req->elem.out_num > 1) { qemu_sgl_init_external(req, &req->elem.out_sg[1], &req->elem.out_addr[1], req->elem.out_num - 1); } else { qemu_sgl_init_external(req, &req->elem.in_sg[1], &req->elem.in_addr[1], req->elem.in_num - 1); } and this will access out of array bounds. Note: this adds security checks within assert calls since SCSIBusInfo's load_request cannot fail. For now simply disable builds with NDEBUG - there seems to be little value in supporting these. Cc: Andreas Färber Signed-off-by: Michael S. Tsirkin Signed-off-by: Juan Quintela --- hw/scsi/virtio-scsi.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'hw') diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index b0d7517..1752193 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -147,6 +147,15 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq) qemu_get_be32s(f, &n); assert(n < vs->conf.num_queues); qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem)); + /* TODO: add a way for SCSIBusInfo's load_request to fail, + * and fail migration instead of asserting here. + * When we do, we might be able to re-enable NDEBUG below. + */ +#ifdef NDEBUG +#error building with NDEBUG is not supported +#endif + assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg)); + assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg)); virtio_scsi_parse_req(s, vs->cmd_vqs[n], req); scsi_req_ref(sreq); -- cgit v1.1 From 3476436a44c29725efef0cabf5b3ea4e70054d57 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:52:21 +0300 Subject: vmstate: s/VMSTATE_INT32_LE/VMSTATE_INT32_POSITIVE_LE/ As the macro verifies the value is positive, rename it to make the function clearer. Signed-off-by: Michael S. Tsirkin Signed-off-by: Juan Quintela --- hw/pci/pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'hw') diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 2a9f08e..517ff2a 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -475,7 +475,7 @@ const VMStateDescription vmstate_pci_device = { .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = (VMStateField []) { - VMSTATE_INT32_LE(version_id, PCIDevice), + VMSTATE_INT32_POSITIVE_LE(version_id, PCIDevice), VMSTATE_BUFFER_UNSAFE_INFO(config, PCIDevice, 0, vmstate_info_pci_config, PCI_CONFIG_SPACE_SIZE), @@ -492,7 +492,7 @@ const VMStateDescription vmstate_pcie_device = { .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = (VMStateField []) { - VMSTATE_INT32_LE(version_id, PCIDevice), + VMSTATE_INT32_POSITIVE_LE(version_id, PCIDevice), VMSTATE_BUFFER_UNSAFE_INFO(config, PCIDevice, 0, vmstate_info_pci_config, PCIE_CONFIG_SPACE_SIZE), -- cgit v1.1 From 9f8e9895c504149d7048e9fc5eb5cbb34b16e49a Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 3 Apr 2014 19:52:25 +0300 Subject: usb: sanity check setup_index+setup_len in post_load CVE-2013-4541 s->setup_len and s->setup_index are fed into usb_packet_copy as size/offset into s->data_buf, it's possible for invalid state to exploit this to load arbitrary data. setup_len and setup_index should be checked to make sure they are not negative. Cc: Gerd Hoffmann Signed-off-by: Michael S. Tsirkin Reviewed-by: Gerd Hoffmann Signed-off-by: Juan Quintela --- hw/usb/bus.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/usb/bus.c b/hw/usb/bus.c index fe70429..e48b19f 100644 --- a/hw/usb/bus.c +++ b/hw/usb/bus.c @@ -49,7 +49,9 @@ static int usb_device_post_load(void *opaque, int version_id) } else { dev->attached = 1; } - if (dev->setup_index >= sizeof(dev->data_buf) || + if (dev->setup_index < 0 || + dev->setup_len < 0 || + dev->setup_index >= sizeof(dev->data_buf) || dev->setup_len >= sizeof(dev->data_buf)) { return -EINVAL; } -- cgit v1.1 From a9c380db3b8c6af19546a68145c8d1438a09c92b Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Mon, 28 Apr 2014 16:08:14 +0300 Subject: ssi-sd: fix buffer overrun on invalid state load CVE-2013-4537 s->arglen is taken from wire and used as idx in ssi_sd_transfer(). Validate it before access. Signed-off-by: Michael S. Tsirkin Signed-off-by: Juan Quintela --- hw/sd/ssi-sd.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'hw') diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index 3273c8a..b012e57 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -230,8 +230,17 @@ static int ssi_sd_load(QEMUFile *f, void *opaque, int version_id) for (i = 0; i < 5; i++) s->response[i] = qemu_get_be32(f); s->arglen = qemu_get_be32(f); + if (s->mode == SSI_SD_CMDARG && + (s->arglen < 0 || s->arglen >= ARRAY_SIZE(s->cmdarg))) { + return -EINVAL; + } s->response_pos = qemu_get_be32(f); s->stopping = qemu_get_be32(f); + if (s->mode == SSI_SD_RESPONSE && + (s->response_pos < 0 || s->response_pos >= ARRAY_SIZE(s->response) || + (!s->stopping && s->arglen > ARRAY_SIZE(s->response)))) { + return -EINVAL; + } ss->cs = qemu_get_be32(f); -- cgit v1.1 From 73d963c0a75cb99c6aaa3f6f25e427aa0b35a02e Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Mon, 28 Apr 2014 16:08:17 +0300 Subject: openpic: avoid buffer overrun on incoming migration CVE-2013-4534 opp->nb_cpus is read from the wire and used to determine how many IRQDest elements to read into opp->dst[]. If the value exceeds the length of opp->dst[], MAX_CPU, opp->dst[] can be overrun with arbitrary data from the wire. Fix this by failing migration if the value read from the wire exceeds MAX_CPU. Signed-off-by: Michael Roth Reviewed-by: Alexander Graf Signed-off-by: Michael S. Tsirkin Signed-off-by: Juan Quintela --- hw/intc/openpic.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) (limited to 'hw') diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c index be76fbd..17136c9 100644 --- a/hw/intc/openpic.c +++ b/hw/intc/openpic.c @@ -41,6 +41,7 @@ #include "hw/sysbus.h" #include "hw/pci/msi.h" #include "qemu/bitops.h" +#include "qapi/qmp/qerror.h" //#define DEBUG_OPENPIC @@ -1416,7 +1417,7 @@ static void openpic_load_IRQ_queue(QEMUFile* f, IRQQueue *q) static int openpic_load(QEMUFile* f, void *opaque, int version_id) { OpenPICState *opp = (OpenPICState *)opaque; - unsigned int i; + unsigned int i, nb_cpus; if (version_id != 1) { return -EINVAL; @@ -1428,7 +1429,11 @@ static int openpic_load(QEMUFile* f, void *opaque, int version_id) qemu_get_be32s(f, &opp->spve); qemu_get_be32s(f, &opp->tfrr); - qemu_get_be32s(f, &opp->nb_cpus); + qemu_get_be32s(f, &nb_cpus); + if (opp->nb_cpus != nb_cpus) { + return -EINVAL; + } + assert(nb_cpus > 0 && nb_cpus <= MAX_CPU); for (i = 0; i < opp->nb_cpus; i++) { qemu_get_sbe32s(f, &opp->dst[i].ctpr); @@ -1567,6 +1572,13 @@ static void openpic_realize(DeviceState *dev, Error **errp) {NULL} }; + if (opp->nb_cpus > MAX_CPU) { + error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, + TYPE_OPENPIC, "nb_cpus", (uint64_t)opp->nb_cpus, + (uint64_t)0, (uint64_t)MAX_CPU); + return; + } + switch (opp->model) { case OPENPIC_MODEL_FSL_MPIC_20: default: -- cgit v1.1 From 98f93ddd84800f207889491e0b5d851386b459cf Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Mon, 28 Apr 2014 16:08:21 +0300 Subject: virtio-net: out-of-bounds buffer write on load CVE-2013-4149 QEMU 1.3.0 out-of-bounds buffer write in virtio_net_load()@hw/net/virtio-net.c > } else if (n->mac_table.in_use) { > uint8_t *buf = g_malloc0(n->mac_table.in_use); We are allocating buffer of size n->mac_table.in_use > qemu_get_buffer(f, buf, n->mac_table.in_use * ETH_ALEN); and read to the n->mac_table.in_use size buffer n->mac_table.in_use * ETH_ALEN bytes, corrupting memory. If adversary controls state then memory written there is controlled by adversary. Reviewed-by: Michael Roth Signed-off-by: Michael S. Tsirkin Signed-off-by: Juan Quintela --- hw/net/virtio-net.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) (limited to 'hw') diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 0a8cb40..940a7cf 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1362,10 +1362,17 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) { qemu_get_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN); - } else if (n->mac_table.in_use) { - uint8_t *buf = g_malloc0(n->mac_table.in_use); - qemu_get_buffer(f, buf, n->mac_table.in_use * ETH_ALEN); - g_free(buf); + } else { + int64_t i; + + /* Overflow detected - can happen if source has a larger MAC table. + * We simply set overflow flag so there's no need to maintain the + * table of addresses, discard them all. + * Note: 64 bit math to avoid integer overflow. + */ + for (i = 0; i < (int64_t)n->mac_table.in_use * ETH_ALEN; ++i) { + qemu_get_byte(f); + } n->mac_table.multi_overflow = n->mac_table.uni_overflow = 1; n->mac_table.in_use = 0; } -- cgit v1.1 From a890a2f9137ac3cf5b607649e66a6f3a5512d8dc Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Mon, 28 Apr 2014 16:08:23 +0300 Subject: virtio: validate config_len on load Malformed input can have config_len in migration stream exceed the array size allocated on destination, the result will be heap overflow. To fix, that config_len matches on both sides. CVE-2014-0182 Reported-by: "Dr. David Alan Gilbert" Signed-off-by: Michael S. Tsirkin Signed-off-by: Juan Quintela -- v2: use %ix and %zx to print config_len values Signed-off-by: Juan Quintela --- hw/virtio/virtio.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index a70169a..7f4e7ec 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -898,6 +898,7 @@ int virtio_set_features(VirtIODevice *vdev, uint32_t val) int virtio_load(VirtIODevice *vdev, QEMUFile *f) { int i, ret; + int32_t config_len; uint32_t num; uint32_t features; uint32_t supported_features; @@ -924,7 +925,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) features, supported_features); return -1; } - vdev->config_len = qemu_get_be32(f); + config_len = qemu_get_be32(f); + if (config_len != vdev->config_len) { + error_report("Unexpected config length 0x%x. Expected 0x%zx", + config_len, vdev->config_len); + return -1; + } qemu_get_buffer(f, vdev->config, vdev->config_len); num = qemu_get_be32(f); -- cgit v1.1