[Qemu-devel] [PATCH 00/23] qemu state loading issues

[Qemu-devel] [PATCH 00/23] qemu state loading issues

[Michael S. Tsirkin]

(the following excellent explanation is due to Petr Matousek)

The state loading functionality was written under
the assumption that the state being loaded can be trusted. This is
mostly true, but we have identified at least two scenarios where it's
not:

* An attacker who has complete control over source qemu-kvm/node (via
  another flaw) and wants to attack destination node (source and
  destination for live migration). He can thus change the migration
  data that will be processed on the destination node, potentially
  allowing exploitation and remote code execution.

  Also, migration initiation is a privileged operation, but I think the
  attacker on the source node could probably fake some symptoms that
  would either make some automated process to start migrating off VMs
  from the node or make node admin to notice and start manual
  migration.

  MITM attack is not considered to be security relevant since the
  security between endpoints can be considered to be configuration
  issue.

* Saving/Loading state to/from file.

  For example:

  https://bugzilla.redhat.com/show_bug.cgi?id=588133#c8
  https://bugzilla.redhat.com/show_bug.cgi?id=588133#c9

After I have identified a first issue like this,
a full audit of the qemu code base was done by Anthony Liguori, Michael
Roth, myself and others, and found multiple instances where loading in
invalid image would corrupt QEMU memory, in some instances making it
possible to overwrite it with attacker-controlled data.

This patchset is the result of that audit: it addresses this set of
security issues by adding input validation and failing migration on
invalid input.

Considering the preconditions, I think that the impact on typical qemu usage is
low.  Still, I think these patches make sense for qemu-stable.

Lots of thanks to Stefan Hajnoczi, Gerd Hoffmann, Kevin Wolf, Paolo
Bonzini and Hans de Goede, for help with the code audit.  Petr
Matousek for review. I hope I didn't forget anyone involved, if I did
I apologize in advance.

I have parked them on my tree for now so they are not lost.

Please review, and consider for stable and 1.8.

Gerd Hoffmann (1):
  usb: sanity check setup_index+setup_len in post_load

Michael Roth (6):
  stellaris_enet: avoid buffer overrun on incoming migration
  stellaris_enet: avoid buffer overrun on incoming migration (part 2)
  stellaris_enet: avoid buffer orerrun on incoming migration (part 3)
  virtio: avoid buffer overrun on incoming migration
  openpic: avoid buffer overrun on incoming migration
  pxa2xx: avoid buffer overrun on incoming migration

Michael S. Tsirkin (16):
  virtio-net: fix buffer overflow on invalid state load
  virtio-net: out-of-bounds buffer write on load
  virtio-net: out-of-bounds buffer write on invalid state load
  virtio: out-of-bounds buffer write on invalid state load
  ahci: fix buffer overrun on invalid state load
  hpet: fix buffer overrun on invalid state load
  hw/pci/pcie_aer.c: fix buffer overruns on invalid state load
  pl022: fix buffer overun on invalid state load
  target-arm/machine.c: fix buffer overflow on invalid state load
  virtio: validate num_sg when mapping
  ssi-sd: fix buffer overrun on invalid state load
  ssd0323: fix buffer overun on invalid state load
  tsc210x: fix buffer overrun on invalid state load
  zaurus: fix buffer overrun on invalid state load
  virtio-scsi: fix buffer overrun on invalid state load
  savevm: fix potential segfault on invalid state

 include/hw/virtio/virtio-net.h |  4 ++--
 hw/arm/pxa2xx.c                |  6 ++++--
 hw/display/ssd0323.c           |  3 +++
 hw/gpio/zaurus.c               |  2 +-
 hw/ide/ahci.c                  |  2 +-
 hw/input/tsc210x.c             | 12 ++++++++++++
 hw/intc/openpic.c              |  3 +++
 hw/net/stellaris_enet.c        | 31 +++++++++++++++++++++----------
 hw/net/virtio-net.c            | 13 ++++++++++---
 hw/pci/pcie_aer.c              | 15 +++++++++++++--
 hw/scsi/virtio-scsi.c          |  2 ++
 hw/sd/ssi-sd.c                 |  3 +++
 hw/ssi/pl022.c                 | 12 ++++++++++++
 hw/timer/hpet.c                | 18 +++++++++++++-----
 hw/usb/bus.c                   |  4 ++++
 hw/virtio/virtio.c             | 17 ++++++++++++++++-
 savevm.c                       |  3 +++
 target-arm/machine.c           |  4 ++++
 18 files changed, 127 insertions(+), 27 deletions(-)

-- 
MST

[Qemu-devel] [PATCH 01/23] virtio-net: fix buffer overflow on invalid state load

[Michael S. Tsirkin]

CVE-2013-4148 QEMU 1.0 integer conversion in
virtio_net_load()@hw/net/virtio-net.c

Deals with loading a corrupted savevm image.

>         n->mac_table.in_use = qemu_get_be32(f);

in_use is int so it can get negative when assigned 32bit unsigned value.

>         /* MAC_TABLE_ENTRIES may be different from the saved image */
>         if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) {

passing this check ^^^

>             qemu_get_buffer(f, n->mac_table.macs,
>                             n->mac_table.in_use * ETH_ALEN);

with good in_use value, "n->mac_table.in_use * ETH_ALEN" can get
positive and bigger than mac_table.macs. For example 0x81000000
satisfies this condition when ETH_ALEN is 6.

A similar problem exists with is_multi.

Fix both by making the value unsigned.

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/virtio/virtio-net.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index df60f16..4b32440 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -176,8 +176,8 @@ typedef struct VirtIONet {
     uint8_t nobcast;
     uint8_t vhost_started;
     struct {
-        int in_use;
-        int first_multi;
+        uint32_t in_use;
+        uint32_t first_multi;
         uint8_t multi_overflow;
         uint8_t uni_overflow;
         uint8_t *macs;
-- 
MST

[Qemu-devel] [PATCH 02/23] virtio-net: out-of-bounds buffer write on load

[Michael S. Tsirkin]

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 <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/virtio-net.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index b75c753..2b92640 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1337,9 +1337,11 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
             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);
+            int i;
+
+            for (i = 0; i < 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;
         }
-- 
MST

[Qemu-devel] [PATCH 03/23] virtio-net: out-of-bounds buffer write on invalid state load

[Michael S. Tsirkin]

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 <mst@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/net/virtio-net.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 2b92640..383c1f5 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1383,6 +1383,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);
         }
-- 
MST

[Qemu-devel] [PATCH 04/23] virtio: out-of-bounds buffer write on invalid state load

[Michael S. Tsirkin]

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 <mst@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 2f1e73b..ec23cb5 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -888,7 +888,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));
@@ -916,6 +917,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) {
-- 
MST

[Qemu-devel] [PATCH 05/23] ahci: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

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 <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/ide/ahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index fbea9e8..e321274 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1290,7 +1290,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()
     },
 };
-- 
MST

[Qemu-devel] [PATCH 06/23] hpet: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

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 post_load hook.

Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/timer/hpet.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 2eb75ea..acdc874 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -211,6 +211,15 @@ static void update_irq(struct HPETTimer *timer, int set)
     }
 }
 
+static void hpet_fix_num_timers(HPETState *s)
+{
+    if (s->num_timers < HPET_MIN_TIMERS) {
+        s->num_timers = HPET_MIN_TIMERS;
+    } else if (s->num_timers > HPET_MAX_TIMERS) {
+        s->num_timers = HPET_MAX_TIMERS;
+    }
+}
+
 static void hpet_pre_save(void *opaque)
 {
     HPETState *s = opaque;
@@ -232,6 +241,8 @@ static int hpet_post_load(void *opaque, int version_id)
 {
     HPETState *s = opaque;
 
+    hpet_fix_num_timers(s);
+
     /* Recalculate the offset between the main counter and guest time */
     s->hpet_offset = ticks_to_ns(s->hpet_counter) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
@@ -719,11 +730,8 @@ static void hpet_realize(DeviceState *dev, Error **errp)
         sysbus_init_irq(sbd, &s->irqs[i]);
     }
 
-    if (s->num_timers < HPET_MIN_TIMERS) {
-        s->num_timers = HPET_MIN_TIMERS;
-    } else if (s->num_timers > HPET_MAX_TIMERS) {
-        s->num_timers = HPET_MAX_TIMERS;
-    }
+    hpet_fix_num_timers(s);
+
     for (i = 0; i < HPET_MAX_TIMERS; i++) {
         timer = &s->timer[i];
         timer->qemu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, hpet_timer, timer);
-- 
MST

[Qemu-devel] [PATCH 07/23] hw/pci/pcie_aer.c: fix buffer overruns on invalid state load

[Michael S. Tsirkin]

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 corrution
again with an overflow but not adversary controlled.

Fix both issues.

Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci/pcie_aer.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 991502e..92f3f20 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -795,15 +795,26 @@ static const VMStateDescription vmstate_pcie_aer_err = {
     }
 };
 
+static int pcie_aer_state_post_load(void *opaque, int version_id)
+{
+    PCIEAERLog *s = opaque;
+
+    if (s->log_num > s->log_max) {
+        return -1;
+    }
+    return 0;
+}
+
 const VMStateDescription vmstate_pcie_aer_log = {
     .name = "PCIE_AER_ERROR_LOG",
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
+    .post_load = pcie_aer_state_post_load,
     .fields     = (VMStateField[]) {
         VMSTATE_UINT16(log_num, PCIEAERLog),
-        VMSTATE_UINT16(log_max, PCIEAERLog),
-        VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num,
+        VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog),
+        VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_max,
                               vmstate_pcie_aer_err, PCIEAERErr),
         VMSTATE_END_OF_LIST()
     }
-- 
MST

[Qemu-devel] [PATCH 08/23] pl022: fix buffer overun on invalid state load

[Michael S. Tsirkin]

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 <mst@redhat.com
Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/ssi/pl022.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/ssi/pl022.c b/hw/ssi/pl022.c
index fd479ef..49b3f61 100644
--- a/hw/ssi/pl022.c
+++ b/hw/ssi/pl022.c
@@ -240,11 +240,23 @@ 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 > ARRAY_SIZE(s->tx_fifo) ||
+        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),
-- 
MST

[Qemu-devel] [PATCH 09/23] target-arm/machine.c: fix buffer overflow on invalid state load

[Michael S. Tsirkin]

CVE-2013-4531

cpreg_vmstate_indexes is a VARRAY_INT32. A negative value for
cpreg_vmstate_array_len will cause a buffer overflow.

Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 target-arm/machine.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-arm/machine.c b/target-arm/machine.c
index 74f010f..d46b7e8 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -178,6 +178,10 @@ static int cpu_post_load(void *opaque, int version_id)
     ARMCPU *cpu = opaque;
     int i, v;
 
+    if (cpu->cpreg_vmstate_array_len < 0) {
+        return -1;
+    }
+
     /* Update the values list from the incoming migration data.
      * Anything in the incoming data which we don't know about is
      * a migration failure; anything we know about but the incoming
-- 
MST

[Qemu-devel] [PATCH 10/23] stellaris_enet: avoid buffer overrun on incoming migration

[Michael S. Tsirkin]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

CVE-2013-4532

s->next_packet is read from wire as an index into s->rx[]. If
s->next_packet exceeds the length of s->rx[], the buffer can be
subsequently overrun with arbitrary data from the wire.

Fix this by introducing a constant that defines the length of
s->rx[], and fail migration if the s->next_packet we read from
the wire exceeds this.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/stellaris_enet.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 9dd77f7..db12a99 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -61,13 +61,14 @@ typedef struct {
     uint32_t np;
     int tx_frame_len;
     int tx_fifo_len;
-    uint8_t tx_fifo[2048];
     /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
        We implement a full 31 packet fifo.  */
+    uint8_t tx_fifo[2048];
+#define SE_RX_BUF_LEN 31
     struct {
         uint8_t data[2048];
         int len;
-    } rx[31];
+    } rx[SE_RX_BUF_LEN];
     uint8_t *rx_fifo;
     int rx_fifo_len;
     int next_packet;
@@ -92,15 +93,15 @@ static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, si
 
     if ((s->rctl & SE_RCTL_RXEN) == 0)
         return -1;
-    if (s->np >= 31) {
+    if (s->np >= SE_RX_BUF_LEN) {
         DPRINTF("Packet dropped\n");
         return -1;
     }
 
     DPRINTF("Received packet len=%d\n", size);
     n = s->next_packet + s->np;
-    if (n >= 31)
-        n -= 31;
+    if (n >= SE_RX_BUF_LEN)
+        n -= SE_RX_BUF_LEN;
     s->np++;
 
     s->rx[n].len = size + 6;
@@ -132,7 +133,7 @@ static int stellaris_enet_can_receive(NetClientState *nc)
     if ((s->rctl & SE_RCTL_RXEN) == 0)
         return 1;
 
-    return (s->np < 31);
+    return (s->np < SE_RX_BUF_LEN);
 }
 
 static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
@@ -168,7 +169,7 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
         if (s->rx_fifo_len <= 0) {
             s->rx_fifo_len = 0;
             s->next_packet++;
-            if (s->next_packet >= 31)
+            if (s->next_packet >= SE_RX_BUF_LEN)
                 s->next_packet = 0;
             s->np--;
             DPRINTF("RX done np=%d\n", s->np);
@@ -344,7 +345,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, s->tx_frame_len);
     qemu_put_be32(f, s->tx_fifo_len);
     qemu_put_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
-    for (i = 0; i < 31; i++) {
+    for (i = 0; i < SE_RX_BUF_LEN; i++) {
         qemu_put_be32(f, s->rx[i].len);
         qemu_put_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
 
@@ -375,12 +376,15 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
     s->tx_frame_len = qemu_get_be32(f);
     s->tx_fifo_len = qemu_get_be32(f);
     qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
-    for (i = 0; i < 31; i++) {
+    for (i = 0; i < SE_RX_BUF_LEN; i++) {
         s->rx[i].len = qemu_get_be32(f);
         qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
 
     }
     s->next_packet = qemu_get_be32(f);
+    if (s->next_packet >= SE_RX_BUF_LEN) {
+        return -EINVAL;
+    }
     s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
     s->rx_fifo_len = qemu_get_be32(f);
 
-- 
MST

[Qemu-devel] [PATCH 11/23] stellaris_enet: avoid buffer overrun on incoming migration (part 2)

[Michael S. Tsirkin]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

CVE-2013-4532

s->tx_fifo_len is read from the wire and later used as an index into
s->tx_fifo[] when a DATA command is issued by the guest. If
s->tx_fifo_len is greater than the length of s->tx_fifo[], or less
than 0, the buffer can be overrun/underrun by arbitrary data written out
by the guest upon resuming it's execution.

Fix this by introducing a constant that defines the length of
s->tx_fifo[] and failing migration if the value from the wire exceeds
this, or is less than 0.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/stellaris_enet.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index db12a99..65f0ba8 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -63,10 +63,11 @@ typedef struct {
     int tx_fifo_len;
     /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
        We implement a full 31 packet fifo.  */
-    uint8_t tx_fifo[2048];
+#define SE_FIFO_LEN 2048
+    uint8_t tx_fifo[SE_FIFO_LEN];
 #define SE_RX_BUF_LEN 31
     struct {
-        uint8_t data[2048];
+        uint8_t data[SE_FIFO_LEN];
         int len;
     } rx[SE_RX_BUF_LEN];
     uint8_t *rx_fifo;
@@ -375,6 +376,9 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
     s->np = qemu_get_be32(f);
     s->tx_frame_len = qemu_get_be32(f);
     s->tx_fifo_len = qemu_get_be32(f);
+    if (s->tx_fifo_len < 0 || s->tx_fifo_len > SE_FIFO_LEN) {
+        return -EINVAL;
+    }
     qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
     for (i = 0; i < SE_RX_BUF_LEN; i++) {
         s->rx[i].len = qemu_get_be32(f);
-- 
MST

[Qemu-devel] [PATCH 12/23] stellaris_enet: avoid buffer orerrun on incoming migration (part 3)

[Michael S. Tsirkin]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

CVE-2013-4532

s->tx_frame_len is read from the wire and can later used as an index
into s->tx_fifo[] for memset() when a DATA command is issued by the guest.

In this case s->tx_frame_len is checked to avoid an overrun, but if the
value is negative a subsequently executed guest can underrun the buffer
with zeros via the memset() call.

Fix this by failing migration if the incoming value is of s->tx_frame_len
is less than -1 (the emulation code allows from -1 as a special case)

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/stellaris_enet.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 65f0ba8..6eca31e 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -375,6 +375,9 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
     s->mrxd = qemu_get_be32(f);
     s->np = qemu_get_be32(f);
     s->tx_frame_len = qemu_get_be32(f);
+    if (s->tx_frame_len < -1) {
+        return -EINVAL;
+    }
     s->tx_fifo_len = qemu_get_be32(f);
     if (s->tx_fifo_len < 0 || s->tx_fifo_len > SE_FIFO_LEN) {
         return -EINVAL;
-- 
MST

[Qemu-devel] [PATCH 13/23] virtio: avoid buffer overrun on incoming migration

[Michael S. Tsirkin]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

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 <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ec23cb5..b3c38f7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -904,6 +904,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) {
-- 
MST

[Qemu-devel] [PATCH 14/23] openpic: avoid buffer overrun on incoming migration

[Michael S. Tsirkin]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

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 <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/intc/openpic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 7df72f4..ab8c43d 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -1429,6 +1429,9 @@ static int openpic_load(QEMUFile* f, void *opaque, int version_id)
     qemu_get_be32s(f, &opp->tfrr);
 
     qemu_get_be32s(f, &opp->nb_cpus);
+    if (opp->nb_cpus > MAX_CPU) {
+        return -EINVAL;
+    }
 
     for (i = 0; i < opp->nb_cpus; i++) {
         qemu_get_sbe32s(f, &opp->dst[i].ctpr);
-- 
MST

[Qemu-devel] [PATCH 15/23] pxa2xx: avoid buffer overrun on incoming migration

[Michael S. Tsirkin]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

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 introducing a constant, RX_FIFO_SZ, that defines the length
of s->rx_fifo[], and taking the wire value modulo RX_FIFO_SZ (as is done
elsewhere in the emulation code when s->rx_level exceeds RX_FIFO_SZ).

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/arm/pxa2xx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 02b7016..41d3c39 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -457,6 +457,8 @@ static const VMStateDescription vmstate_pxa2xx_mm = {
     }
 };
 
+#define RX_FIFO_SZ 16
+
 #define TYPE_PXA2XX_SSP "pxa2xx-ssp"
 #define PXA2XX_SSP(obj) \
     OBJECT_CHECK(PXA2xxSSPState, (obj), TYPE_PXA2XX_SSP)
@@ -481,7 +483,7 @@ typedef struct {
     uint8_t ssrsa;
     uint8_t ssacd;
 
-    uint32_t rx_fifo[16];
+    uint32_t rx_fifo[RX_FIFO_SZ];
     int rx_level;
     int rx_start;
 } PXA2xxSSPState;
@@ -756,7 +758,7 @@ 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);
+    s->rx_level = qemu_get_byte(f) % RX_FIFO_SZ;
     s->rx_start = 0;
     for (i = 0; i < s->rx_level; i ++)
         s->rx_fifo[i] = qemu_get_byte(f);
-- 
MST

[Qemu-devel] [PATCH 16/23] virtio: validate num_sg when mapping

[Michael S. Tsirkin]

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 <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index b3c38f7..edc2ce7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -427,6 +427,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: %d > %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);
-- 
MST

[Qemu-devel] [PATCH 17/23] ssi-sd: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

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 <mst@redhat.com>
---
 hw/sd/ssi-sd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 1bb56c4..bb7bd69 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -230,6 +230,9 @@ 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 > ARRAY_SIZE(s->cmdarg)) {
+        return -EINVAL;
+    }
     s->response_pos = qemu_get_be32(f);
     s->stopping = qemu_get_be32(f);
 
-- 
MST

[Qemu-devel] [PATCH 18/23] ssd0323: fix buffer overun on invalid state load

[Michael S. Tsirkin]

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.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/display/ssd0323.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
index c3231c6..2c82598 100644
--- a/hw/display/ssd0323.c
+++ b/hw/display/ssd0323.c
@@ -312,6 +312,9 @@ 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);
-- 
MST

[Qemu-devel] [PATCH 19/23] tsc210x: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

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.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/input/tsc210x.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c
index 485c9e5..c7513c7 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 > ARRAY_SIZE(mode_regs)) {
+        return -EINVAL;
+    }
     s->nextfunction = qemu_get_byte(f);
+    if (s->nextfunction > ARRAY_SIZE(mode_regs)) {
+        return -EINVAL;
+    }
     s->precision = qemu_get_byte(f);
+    if (s->precision > ARRAY_SIZE(resolution)) {
+        return -EINVAL;
+    }
     s->nextprecision = qemu_get_byte(f);
+    if (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);
-- 
MST

[Qemu-devel] [PATCH 20/23] zaurus: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

CVE-2013-4540

Within scoop_gpio_handler_update, if prev_level has a high bit set, then
we get bit > 16 and that does a buffer overrun.

Since prev_level comes from wire indirectly, this can
happen on invalid state load.

To fix, limit to 16 bit.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/gpio/zaurus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/gpio/zaurus.c b/hw/gpio/zaurus.c
index dc79a8b..f3b02c3 100644
--- a/hw/gpio/zaurus.c
+++ b/hw/gpio/zaurus.c
@@ -60,7 +60,7 @@ struct ScoopInfo {
 #define SCOOP_GPRR	0x28
 
 static inline void scoop_gpio_handler_update(ScoopInfo *s) {
-    uint32_t level, diff;
+    uint16_t level, diff;
     int bit;
     level = s->gpio_level & s->gpio_dir;
 
-- 
MST

[Qemu-devel] [PATCH 21/23] usb: sanity check setup_index+setup_len in post_load

[Michael S. Tsirkin]

From: Gerd Hoffmann <kraxel@redhat.com>

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 against data_buf size.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/usb/bus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index ca329be..4ed1c3b 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -51,6 +51,10 @@ static int usb_device_post_load(void *opaque, int version_id)
         dev->setup_len >= sizeof(dev->data_buf)) {
         return -EINVAL;
     }
+    if (dev->setup_index >= sizeof(dev->data_buf) ||
+        dev->setup_len >= sizeof(dev->data_buf)) {
+        return -EINVAL;
+    }
     return 0;
 }
 
-- 
MST

[Qemu-devel] [PATCH 22/23] virtio-scsi: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

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.
suggested patch:

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/scsi/virtio-scsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 26d95a1..51cc929 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -147,6 +147,8 @@ 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));
+    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);
-- 
MST

[Qemu-devel] [PATCH 23/23] savevm: fix potential segfault on invalid state

[Michael S. Tsirkin]

savevm will segfault if version_id < vmsd->minimum_version_id &&
version_id >= vmsd->minimum_version_id_old

This calls through a NULL pointer.  This is a bug (should
exit not crash).

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 savevm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/savevm.c b/savevm.c
index 3f912dd..04349f6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1686,6 +1686,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         return -EINVAL;
     }
     if  (version_id < vmsd->minimum_version_id) {
+        if (!vmsd->load_state_old) {
+            return -EINVAL;
+        }
         return vmsd->load_state_old(f, opaque, version_id);
     }
     if (vmsd->pre_load) {
-- 
MST

Re: [Qemu-devel] [PATCH 09/23] target-arm/machine.c: fix buffer overflow on invalid state load

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4531
>
> cpreg_vmstate_indexes is a VARRAY_INT32. A negative value for
> cpreg_vmstate_array_len will cause a buffer overflow.
>
> Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  target-arm/machine.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 74f010f..d46b7e8 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -178,6 +178,10 @@ static int cpu_post_load(void *opaque, int version_id)
>      ARMCPU *cpu = opaque;
>      int i, v;
>
> +    if (cpu->cpreg_vmstate_array_len < 0) {
> +        return -1;
> +    }
> +

I think this is the wrong way to fix this bug. The intent of the
code is that using VMSTATE_INT32_LE() for the array_len
makes the migration vmstate code do a check and reject
array lengths which overflow the array. We should fix this
check rather than attempting to catch the cases where it
didn't work in the post_load hook, not least because by the
time we've reached the post-load hook we'll have already
overrun the buffer...

Given the uses of VMSTATE_INT32_LE I suspect we
could safely redefine its semantics to mean "only OK
if less-than-or-equal to X and non-negative". Or we could
have a new VMSTATE_ macro with those semantics,
for array-length checks.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 22/23] virtio-scsi: fix buffer overrun on invalid state load

[Peter Maydell]

On 3 December 2013 16:29, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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.
> suggested patch:
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 26d95a1..51cc929 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -147,6 +147,8 @@ 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));
> +    assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
> +    assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));

Wouldn't it be better to fail migration, as other patches in
this series do? "Silent security hole if you compile with
-DNDEBUG" is a little bit unfriendly...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/23] qemu state loading issues

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
>   For example:
>
>   https://bugzilla.redhat.com/show_bug.cgi?id=588133#c8
>   https://bugzilla.redhat.com/show_bug.cgi?id=588133#c9

I get "not authorized" errors on both of those.

> This patchset is the result of that audit: it addresses this set of
> security issues by adding input validation and failing migration on
> invalid input.

I notice that some but not all of these patches have CVE numbers
in the commit messages -- what's the distinction that meant some
of them get CVEs and some don't?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/23] hw/pci/pcie_aer.c: fix buffer overruns on invalid state load

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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 corrution
> again with an overflow but not adversary controlled.
>
> Fix both issues.
>
> Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/pci/pcie_aer.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index 991502e..92f3f20 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -795,15 +795,26 @@ static const VMStateDescription vmstate_pcie_aer_err = {
>      }
>  };
>
> +static int pcie_aer_state_post_load(void *opaque, int version_id)
> +{
> +    PCIEAERLog *s = opaque;
> +
> +    if (s->log_num > s->log_max) {
> +        return -1;
> +    }
> +    return 0;
> +}
> +
>  const VMStateDescription vmstate_pcie_aer_log = {
>      .name = "PCIE_AER_ERROR_LOG",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
> +    .post_load = pcie_aer_state_post_load,
>      .fields     = (VMStateField[]) {
>          VMSTATE_UINT16(log_num, PCIEAERLog),
> -        VMSTATE_UINT16(log_max, PCIEAERLog),
> -        VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num,
> +        VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog),
> +        VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_max,
>                                vmstate_pcie_aer_err, PCIEAERErr),
>          VMSTATE_END_OF_LIST()
>      }

Isn't this a migration compability break? If so the version ID
needs to be bumped.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 11/23] stellaris_enet: avoid buffer overrun on incoming migration (part 2)

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> CVE-2013-4532
>
> s->tx_fifo_len is read from the wire and later used as an index into
> s->tx_fifo[] when a DATA command is issued by the guest. If
> s->tx_fifo_len is greater than the length of s->tx_fifo[], or less
> than 0, the buffer can be overrun/underrun by arbitrary data written out
> by the guest upon resuming it's execution.

"its".

thanks
-- PMM

Re: [Qemu-devel] [PATCH 06/23] hpet: fix buffer overrun on invalid state load

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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 post_load hook.
>
> Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/timer/hpet.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 2eb75ea..acdc874 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -211,6 +211,15 @@ static void update_irq(struct HPETTimer *timer, int set)
>      }
>  }
>
> +static void hpet_fix_num_timers(HPETState *s)
> +{
> +    if (s->num_timers < HPET_MIN_TIMERS) {
> +        s->num_timers = HPET_MIN_TIMERS;
> +    } else if (s->num_timers > HPET_MAX_TIMERS) {
> +        s->num_timers = HPET_MAX_TIMERS;
> +    }
> +}
> +
>  static void hpet_pre_save(void *opaque)
>  {
>      HPETState *s = opaque;
> @@ -232,6 +241,8 @@ static int hpet_post_load(void *opaque, int version_id)
>  {
>      HPETState *s = opaque;
>
> +    hpet_fix_num_timers(s);

Haven't we already overrun the buffer at this point?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 01/23] virtio-net: fix buffer overflow on invalid state load

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4148 QEMU 1.0 integer conversion in
> virtio_net_load()@hw/net/virtio-net.c
>
> Deals with loading a corrupted savevm image.
>
>>         n->mac_table.in_use = qemu_get_be32(f);
>
> in_use is int so it can get negative when assigned 32bit unsigned value.
>
>>         /* MAC_TABLE_ENTRIES may be different from the saved image */
>>         if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) {
>
> passing this check ^^^
>
>>             qemu_get_buffer(f, n->mac_table.macs,
>>                             n->mac_table.in_use * ETH_ALEN);
>
> with good in_use value, "n->mac_table.in_use * ETH_ALEN" can get
> positive and bigger than mac_table.macs. For example 0x81000000
> satisfies this condition when ETH_ALEN is 6.
>
> A similar problem exists with is_multi.

You mean "first_multi" (though given how the load function
sets first_multi I'm not sure how it can ever get a negative
value.)

> Fix both by making the value unsigned.

Did you audit all the uses of these fields to confirm that
making them unsigned didn't cause any issues due to
arithmetic, comparisons, etc becoming unsigned rather than
signed operations?

A safer fix would seem to be to just fix the check in the
load function to refuse negative values as well as overlarge ones.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 22/23] virtio-scsi: fix buffer overrun on invalid state load

[Paolo Bonzini]

Il 03/12/2013 19:19, Peter Maydell ha scritto:
> On 3 December 2013 16:29, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 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.
>> suggested patch:
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  hw/scsi/virtio-scsi.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>> index 26d95a1..51cc929 100644
>> --- a/hw/scsi/virtio-scsi.c
>> +++ b/hw/scsi/virtio-scsi.c
>> @@ -147,6 +147,8 @@ 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));
>> +    assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
>> +    assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));
> 
> Wouldn't it be better to fail migration, as other patches in
> this series do? "Silent security hole if you compile with
> -DNDEBUG" is a little bit unfriendly...

The problem is that SCSIBusInfo's load_request cannot fail.  I can look
at fixing it on top of this series.

Paolo

Re: [Qemu-devel] [PATCH 02/23] virtio-net: out-of-bounds buffer write on load

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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 <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/virtio-net.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index b75c753..2b92640 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1337,9 +1337,11 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>              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);
> +            int i;
> +
> +            for (i = 0; i < 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;
>          }

This code could use a comment specifically saying that we just
throw away the incoming table data.

NB: if you accept my suggestion of leaving in_use as a signed
value, watch out that signed arithmetic overflow is undefined
behavior.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 18/23] ssd0323: fix buffer overun on invalid state load

[Peter Maydell]

On 3 December 2013 16:29, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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.

The commit message says it validates the row/col fields...

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/display/ssd0323.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
> index c3231c6..2c82598 100644
> --- a/hw/display/ssd0323.c
> +++ b/hw/display/ssd0323.c
> @@ -312,6 +312,9 @@ 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);
> --

...but the patch only checks the cmd_len ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 20/23] zaurus: fix buffer overrun on invalid state load

[Peter Maydell]

On 3 December 2013 16:29, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4540
>
> Within scoop_gpio_handler_update, if prev_level has a high bit set, then
> we get bit > 16 and that does a buffer overrun.
>
> Since prev_level comes from wire indirectly, this can
> happen on invalid state load.
>
> To fix, limit to 16 bit.

I feel like it would be more robust to sanitize on state load
rather than hoping that all future updates to the device continue
to work with bogus values in the state struct.

Alternatively we could make gpio_dir, gpio_level and prev_level
uint16_t in the state struct and vmstate (we don't care about
migration cross-version compat for this device).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 15/23] pxa2xx: avoid buffer overrun on incoming migration

[Don Koch]

On 12/03/2013 11:29 AM, Michael S. Tsirkin wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> 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 introducing a constant, RX_FIFO_SZ, that defines the length
> of s->rx_fifo[], and taking the wire value modulo RX_FIFO_SZ (as is done
> elsewhere in the emulation code when s->rx_level exceeds RX_FIFO_SZ).
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/arm/pxa2xx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index 02b7016..41d3c39 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -457,6 +457,8 @@ static const VMStateDescription vmstate_pxa2xx_mm = {
>      }
>  };
>  
> +#define RX_FIFO_SZ 16
> +
>  #define TYPE_PXA2XX_SSP "pxa2xx-ssp"
>  #define PXA2XX_SSP(obj) \
>      OBJECT_CHECK(PXA2xxSSPState, (obj), TYPE_PXA2XX_SSP)
> @@ -481,7 +483,7 @@ typedef struct {
>      uint8_t ssrsa;
>      uint8_t ssacd;
>  
> -    uint32_t rx_fifo[16];
> +    uint32_t rx_fifo[RX_FIFO_SZ];
>      int rx_level;
>      int rx_start;
>  } PXA2xxSSPState;
> @@ -756,7 +758,7 @@ 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);
> +    s->rx_level = qemu_get_byte(f) % RX_FIFO_SZ;

This looks like it could leave garbage to be read in later. Why not
check for s->rx_level > RX_FIFO_SZ and return an error like the others?

>      s->rx_start = 0;
>      for (i = 0; i < s->rx_level; i ++)
>          s->rx_fifo[i] = qemu_get_byte(f);
> 

-d

Re: [Qemu-devel] [PATCH 15/23] pxa2xx: avoid buffer overrun on incoming migration

[Peter Maydell]

On 3 December 2013 16:29, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> 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 introducing a constant, RX_FIFO_SZ, that defines the length
> of s->rx_fifo[], and taking the wire value modulo RX_FIFO_SZ (as is done
> elsewhere in the emulation code when s->rx_level exceeds RX_FIFO_SZ).
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/arm/pxa2xx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index 02b7016..41d3c39 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -457,6 +457,8 @@ static const VMStateDescription vmstate_pxa2xx_mm = {
>      }
>  };
>
> +#define RX_FIFO_SZ 16
> +
>  #define TYPE_PXA2XX_SSP "pxa2xx-ssp"
>  #define PXA2XX_SSP(obj) \
>      OBJECT_CHECK(PXA2xxSSPState, (obj), TYPE_PXA2XX_SSP)
> @@ -481,7 +483,7 @@ typedef struct {
>      uint8_t ssrsa;
>      uint8_t ssacd;
>
> -    uint32_t rx_fifo[16];
> +    uint32_t rx_fifo[RX_FIFO_SZ];
>      int rx_level;
>      int rx_start;
>  } PXA2xxSSPState;
> @@ -756,7 +758,7 @@ 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);
> +    s->rx_level = qemu_get_byte(f) % RX_FIFO_SZ;
>      s->rx_start = 0;
>      for (i = 0; i < s->rx_level; i ++)
>          s->rx_fifo[i] = qemu_get_byte(f);

An rx_level of 16 is OK, but this change will incorrectly read
it as zero, won't it?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 11/23] stellaris_enet: avoid buffer overrun on incoming migration (part 2)

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> CVE-2013-4532
>
> s->tx_fifo_len is read from the wire and later used as an index into
> s->tx_fifo[] when a DATA command is issued by the guest. If
> s->tx_fifo_len is greater than the length of s->tx_fifo[], or less
> than 0, the buffer can be overrun/underrun by arbitrary data written out
> by the guest upon resuming it's execution.
>
> Fix this by introducing a constant that defines the length of
> s->tx_fifo[] and failing migration if the value from the wire exceeds
> this, or is less than 0.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/stellaris_enet.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index db12a99..65f0ba8 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -63,10 +63,11 @@ typedef struct {
>      int tx_fifo_len;
>      /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
>         We implement a full 31 packet fifo.  */
> -    uint8_t tx_fifo[2048];
> +#define SE_FIFO_LEN 2048
> +    uint8_t tx_fifo[SE_FIFO_LEN];
>  #define SE_RX_BUF_LEN 31
>      struct {
> -        uint8_t data[2048];
> +        uint8_t data[SE_FIFO_LEN];
>          int len;
>      } rx[SE_RX_BUF_LEN];
>      uint8_t *rx_fifo;
> @@ -375,6 +376,9 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>      s->np = qemu_get_be32(f);
>      s->tx_frame_len = qemu_get_be32(f);
>      s->tx_fifo_len = qemu_get_be32(f);
> +    if (s->tx_fifo_len < 0 || s->tx_fifo_len > SE_FIFO_LEN) {
> +        return -EINVAL;
> +    }

Actually this isn't quite right, is it? tx_fifo_len should only be
allowed to be SE_FIFO_LEN-3 .. SE_FIFO_LEN if tx_frame_len
is -1. Otherwise the guest can write up to 4 bytes off the end of
the array. (Admittedly that's pretty harmless as it's just into the
rx fifo at the moment, but still.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 12/23] stellaris_enet: avoid buffer orerrun on incoming migration (part 3)

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> CVE-2013-4532
>
> s->tx_frame_len is read from the wire and can later used as an index
> into s->tx_fifo[] for memset() when a DATA command is issued by the guest.
>
> In this case s->tx_frame_len is checked to avoid an overrun, but if the
> value is negative a subsequently executed guest can underrun the buffer
> with zeros via the memset() call.
>
> Fix this by failing migration if the incoming value is of s->tx_frame_len
> is less than -1 (the emulation code allows from -1 as a special case)
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/stellaris_enet.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 65f0ba8..6eca31e 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -375,6 +375,9 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>      s->mrxd = qemu_get_be32(f);
>      s->np = qemu_get_be32(f);
>      s->tx_frame_len = qemu_get_be32(f);
> +    if (s->tx_frame_len < -1) {
> +        return -EINVAL;
> +    }

Don't we need to also check that tx_frame_len <= 2032?

Otherwise the migration stream could set it to some large
positive number and use this to bypass the check on whether
writing to tx_fifo[tx_fifo_len] has hit the end of the fifo buffer,
I think.

>      s->tx_fifo_len = qemu_get_be32(f);
>      if (s->tx_fifo_len < 0 || s->tx_fifo_len > SE_FIFO_LEN) {
>          return -EINVAL;
> --
> MST
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 10/23] stellaris_enet: avoid buffer overrun on incoming migration

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> CVE-2013-4532

(Is this device even used in any board which supports migration,
incidentally? Personally I would be surprised if migrating a stellaris
board worked since I don't expect anybody's ever tested it.)

> s->next_packet is read from wire as an index into s->rx[]. If
> s->next_packet exceeds the length of s->rx[], the buffer can be
> subsequently overrun with arbitrary data from the wire.
>
> Fix this by introducing a constant that defines the length of
> s->rx[], and fail migration if the s->next_packet we read from
> the wire exceeds this.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/stellaris_enet.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 9dd77f7..db12a99 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -61,13 +61,14 @@ typedef struct {
>      uint32_t np;
>      int tx_frame_len;
>      int tx_fifo_len;
> -    uint8_t tx_fifo[2048];
>      /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
>         We implement a full 31 packet fifo.  */
> +    uint8_t tx_fifo[2048];

...why have you moved this TX fifo buffer below the comment about
the RX fifo?

> +#define SE_RX_BUF_LEN 31
>      struct {
>          uint8_t data[2048];
>          int len;
> -    } rx[31];
> +    } rx[SE_RX_BUF_LEN];
>      uint8_t *rx_fifo;
>      int rx_fifo_len;
>      int next_packet;
> @@ -92,15 +93,15 @@ static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, si
>
>      if ((s->rctl & SE_RCTL_RXEN) == 0)
>          return -1;
> -    if (s->np >= 31) {
> +    if (s->np >= SE_RX_BUF_LEN) {
>          DPRINTF("Packet dropped\n");
>          return -1;
>      }
>
>      DPRINTF("Received packet len=%d\n", size);
>      n = s->next_packet + s->np;
> -    if (n >= 31)
> -        n -= 31;
> +    if (n >= SE_RX_BUF_LEN)
> +        n -= SE_RX_BUF_LEN;

Fixing these hardcoded constants is nice, but coding style
demands braces if you're touching the code.

>      s->np++;
>
>      s->rx[n].len = size + 6;
> @@ -132,7 +133,7 @@ static int stellaris_enet_can_receive(NetClientState *nc)
>      if ((s->rctl & SE_RCTL_RXEN) == 0)
>          return 1;
>
> -    return (s->np < 31);
> +    return (s->np < SE_RX_BUF_LEN);
>  }
>
>  static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
> @@ -168,7 +169,7 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
>          if (s->rx_fifo_len <= 0) {
>              s->rx_fifo_len = 0;
>              s->next_packet++;
> -            if (s->next_packet >= 31)
> +            if (s->next_packet >= SE_RX_BUF_LEN)
>                  s->next_packet = 0;
>              s->np--;
>              DPRINTF("RX done np=%d\n", s->np);
> @@ -344,7 +345,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
>      qemu_put_be32(f, s->tx_frame_len);
>      qemu_put_be32(f, s->tx_fifo_len);
>      qemu_put_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
> -    for (i = 0; i < 31; i++) {
> +    for (i = 0; i < SE_RX_BUF_LEN; i++) {
>          qemu_put_be32(f, s->rx[i].len);
>          qemu_put_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
>
> @@ -375,12 +376,15 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>      s->tx_frame_len = qemu_get_be32(f);
>      s->tx_fifo_len = qemu_get_be32(f);

>      qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
> -    for (i = 0; i < 31; i++) {
> +    for (i = 0; i < SE_RX_BUF_LEN; i++) {
>          s->rx[i].len = qemu_get_be32(f);
>          qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));

If you don't constrain the rx[i].len to be something sensible
the device model will subsequently happily read off the end
of the data buffer.

>
>      }
>      s->next_packet = qemu_get_be32(f);
> +    if (s->next_packet >= SE_RX_BUF_LEN) {
> +        return -EINVAL;
> +    }
>      s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);

It seems like a bad idea to let the incoming migration stream
completely determine the value of the rx_fifo pointer, which will
let the guest read arbitrary qemu process memory...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/23] hw/pci/pcie_aer.c: fix buffer overruns on invalid state load

[Michael S. Tsirkin]

On Tue, Dec 03, 2013 at 06:30:46PM +0000, Peter Maydell wrote:
> On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 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 corrution
> > again with an overflow but not adversary controlled.
> >
> > Fix both issues.
> >
> > Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> > Reported-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/pci/pcie_aer.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> > index 991502e..92f3f20 100644
> > --- a/hw/pci/pcie_aer.c
> > +++ b/hw/pci/pcie_aer.c
> > @@ -795,15 +795,26 @@ static const VMStateDescription vmstate_pcie_aer_err = {
> >      }
> >  };
> >
> > +static int pcie_aer_state_post_load(void *opaque, int version_id)
> > +{
> > +    PCIEAERLog *s = opaque;
> > +
> > +    if (s->log_num > s->log_max) {
> > +        return -1;
> > +    }
> > +    return 0;
> > +}
> > +
> >  const VMStateDescription vmstate_pcie_aer_log = {
> >      .name = "PCIE_AER_ERROR_LOG",
> >      .version_id = 1,
> >      .minimum_version_id = 1,
> >      .minimum_version_id_old = 1,
> > +    .post_load = pcie_aer_state_post_load,
> >      .fields     = (VMStateField[]) {
> >          VMSTATE_UINT16(log_num, PCIEAERLog),
> > -        VMSTATE_UINT16(log_max, PCIEAERLog),
> > -        VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num,
> > +        VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog),
> > +        VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_max,
> >                                vmstate_pcie_aer_err, PCIEAERErr),
> >          VMSTATE_END_OF_LIST()
> >      }
> 
> Isn't this a migration compability break?

How is it a break?

> If so the version ID
> needs to be bumped.
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 15/23] pxa2xx: avoid buffer overrun on incoming migration

[Michael Roth]

Quoting Don Koch (2013-12-03 13:46:24)
> On 12/03/2013 11:29 AM, Michael S. Tsirkin wrote:
> > From: Michael Roth <mdroth@linux.vnet.ibm.com>
> > 
> > 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 introducing a constant, RX_FIFO_SZ, that defines the length
> > of s->rx_fifo[], and taking the wire value modulo RX_FIFO_SZ (as is done
> > elsewhere in the emulation code when s->rx_level exceeds RX_FIFO_SZ).
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/arm/pxa2xx.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> > index 02b7016..41d3c39 100644
> > --- a/hw/arm/pxa2xx.c
> > +++ b/hw/arm/pxa2xx.c
> > @@ -457,6 +457,8 @@ static const VMStateDescription vmstate_pxa2xx_mm = {
> >      }
> >  };
> >  
> > +#define RX_FIFO_SZ 16
> > +
> >  #define TYPE_PXA2XX_SSP "pxa2xx-ssp"
> >  #define PXA2XX_SSP(obj) \
> >      OBJECT_CHECK(PXA2xxSSPState, (obj), TYPE_PXA2XX_SSP)
> > @@ -481,7 +483,7 @@ typedef struct {
> >      uint8_t ssrsa;
> >      uint8_t ssacd;
> >  
> > -    uint32_t rx_fifo[16];
> > +    uint32_t rx_fifo[RX_FIFO_SZ];
> >      int rx_level;
> >      int rx_start;
> >  } PXA2xxSSPState;
> > @@ -756,7 +758,7 @@ 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);
> > +    s->rx_level = qemu_get_byte(f) % RX_FIFO_SZ;
> 
> This looks like it could leave garbage to be read in later. Why not
> check for s->rx_level > RX_FIFO_SZ and return an error like the others?

When I looked at the code before it seemed like s->rx_level was a running index
into a circular buffer, but I see now it never gets incremented beyond 16:

        if (s->enable) {
            uint32_t readval;
            readval = ssi_transfer(s->bus, value);
            if (s->rx_level < 0x10) {
                s->rx_fifo[(s->rx_start + s->rx_level ++) & 0xf] = readval;
            } else {
                s->sssr |= SSSR_ROR;
            }
        }

So it probably makes more sense to just fail migration if it exceeds 16. I
think that would also address the issue Peter pointed out.

> 
> >      s->rx_start = 0;
> >      for (i = 0; i < s->rx_level; i ++)
> >          s->rx_fifo[i] = qemu_get_byte(f);
> > 
> 
> -d

Re: [Qemu-devel] [PATCH 07/23] hw/pci/pcie_aer.c: fix buffer overruns on invalid state load

[Peter Maydell]

On 3 December 2013 20:41, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Dec 03, 2013 at 06:30:46PM +0000, Peter Maydell wrote:
>> On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >  const VMStateDescription vmstate_pcie_aer_log = {
>> >      .name = "PCIE_AER_ERROR_LOG",
>> >      .version_id = 1,
>> >      .minimum_version_id = 1,
>> >      .minimum_version_id_old = 1,
>> > +    .post_load = pcie_aer_state_post_load,
>> >      .fields     = (VMStateField[]) {
>> >          VMSTATE_UINT16(log_num, PCIEAERLog),
>> > -        VMSTATE_UINT16(log_max, PCIEAERLog),
>> > -        VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num,
>> > +        VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog),
>> > +        VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_max,
>> >                                vmstate_pcie_aer_err, PCIEAERErr),
>> >          VMSTATE_END_OF_LIST()
>> >      }
>>
>> Isn't this a migration compability break?
>
> How is it a break?

If a QEMU with this patch sends data to a QEMU without it, then the
receiving end will think it should expect log_num array entries but the
sending end is going to send log_max of them. Conversely, an old->new
migration is going to send fewer array entries than the destination
expects. Or have I misinterpreted how the VARRAY entries work?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/23] hw/pci/pcie_aer.c: fix buffer overruns on invalid state load

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

On 12/03/2013 01:59 PM, Peter Maydell wrote:

> 
> If a QEMU with this patch sends data to a QEMU without it, then the
> receiving end will think it should expect log_num array entries but the
> sending end is going to send log_max of them. Conversely, an old->new
> migration is going to send fewer array entries than the destination
> expects. Or have I misinterpreted how the VARRAY entries work?

If a qemu sends data larger than the field, the source side is already
compromised.  All we care about is that the destination fails
gracefully, rather than acting on the bogus information from the
compromised source.  Versioning is only necessary for correct migration
data, and doesn't matter when the source is already compromised.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 07/23] hw/pci/pcie_aer.c: fix buffer overruns on invalid state load

[Peter Maydell]

On 3 December 2013 21:19, Eric Blake <eblake@redhat.com> wrote:
> On 12/03/2013 01:59 PM, Peter Maydell wrote:
>
>>
>> If a QEMU with this patch sends data to a QEMU without it, then the
>> receiving end will think it should expect log_num array entries but the
>> sending end is going to send log_max of them. Conversely, an old->new
>> migration is going to send fewer array entries than the destination
>> expects. Or have I misinterpreted how the VARRAY entries work?
>
> If a qemu sends data larger than the field, the source side is already
> compromised.

Not if the reason it's sending data larger than the field is because
it's a non-compromised QEMU with this patch which makes it send
log_max entries regardless of log_num, surely?

-- PMM

Re: [Qemu-devel] [PATCH 07/23] hw/pci/pcie_aer.c: fix buffer overruns on invalid state load

[Michael S. Tsirkin]

On Tue, Dec 03, 2013 at 08:59:52PM +0000, Peter Maydell wrote:
> On 3 December 2013 20:41, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Dec 03, 2013 at 06:30:46PM +0000, Peter Maydell wrote:
> >> On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >  const VMStateDescription vmstate_pcie_aer_log = {
> >> >      .name = "PCIE_AER_ERROR_LOG",
> >> >      .version_id = 1,
> >> >      .minimum_version_id = 1,
> >> >      .minimum_version_id_old = 1,
> >> > +    .post_load = pcie_aer_state_post_load,
> >> >      .fields     = (VMStateField[]) {
> >> >          VMSTATE_UINT16(log_num, PCIEAERLog),
> >> > -        VMSTATE_UINT16(log_max, PCIEAERLog),
> >> > -        VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num,
> >> > +        VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog),
> >> > +        VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_max,
> >> >                                vmstate_pcie_aer_err, PCIEAERErr),
> >> >          VMSTATE_END_OF_LIST()
> >> >      }
> >>
> >> Isn't this a migration compability break?
> >
> > How is it a break?
> 
> If a QEMU with this patch sends data to a QEMU without it, then the
> receiving end will think it should expect log_num array entries but the
> sending end is going to send log_max of them. Conversely, an old->new
> migration is going to send fewer array entries than the destination
> expects. Or have I misinterpreted how the VARRAY entries work?
> 
> thanks
> -- PMM

Ah, got it. You are right, good catch. I think we need VMSTATE_UINT16_LE, this will
make sure log_num <= log_max without changing
VMSTATE_STRUCT_VARRAY size.

Re: [Qemu-devel] [PATCH 00/23] qemu state loading issues

[Michael S. Tsirkin]

On Tue, Dec 03, 2013 at 06:24:24PM +0000, Peter Maydell wrote:
> On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote:
> >   For example:
> >
> >   https://bugzilla.redhat.com/show_bug.cgi?id=588133#c8
> >   https://bugzilla.redhat.com/show_bug.cgi?id=588133#c9
> 
> I get "not authorized" errors on both of those.

Oh, sorry :(
You'll have to take my word on it that what happened there
was that a bug reporter saved state and suggested that
developer attempt to load it to reproduce the bug.

> > This patchset is the result of that audit: it addresses this set of
> > security issues by adding input validation and failing migration on
> > invalid input.
> 
> I notice that some but not all of these patches have CVE numbers
> in the commit messages -- what's the distinction that meant some
> of them get CVEs and some don't?
> 
> thanks
> -- PMM

The one that does not have a CVE is 23/23, this is
a NULL pointer dereference on invalid input, which
is not nice but probably not exploitable.

-- 
MST

Re: [Qemu-devel] [PATCH 23/23] savevm: fix potential segfault on invalid state

[Andreas Färber]

Am 03.12.2013 17:29, schrieb Michael S. Tsirkin:
> savevm will segfault if version_id < vmsd->minimum_version_id &&
> version_id >= vmsd->minimum_version_id_old
> 
> This calls through a NULL pointer.  This is a bug (should
> exit not crash).
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 22/23] virtio-scsi: fix buffer overrun on invalid state load

[Andreas Färber]

Am 03.12.2013 20:24, schrieb Paolo Bonzini:
> Il 03/12/2013 19:19, Peter Maydell ha scritto:
>> On 3 December 2013 16:29, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 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.
>>> suggested patch:
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  hw/scsi/virtio-scsi.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>>> index 26d95a1..51cc929 100644
>>> --- a/hw/scsi/virtio-scsi.c
>>> +++ b/hw/scsi/virtio-scsi.c
>>> @@ -147,6 +147,8 @@ 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));
>>> +    assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
>>> +    assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));
>>
>> Wouldn't it be better to fail migration, as other patches in
>> this series do? "Silent security hole if you compile with
>> -DNDEBUG" is a little bit unfriendly...
> 
> The problem is that SCSIBusInfo's load_request cannot fail.  I can look
> at fixing it on top of this series.

Paolo, with this series not yet in, am I seeing correctly that no
changes to allow failing this have been committed? Can you consider
coordinating this with mst?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 22/23] virtio-scsi: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

On Thu, Mar 06, 2014 at 07:30:17PM +0100, Andreas Färber wrote:
> Am 03.12.2013 20:24, schrieb Paolo Bonzini:
> > Il 03/12/2013 19:19, Peter Maydell ha scritto:
> >> On 3 December 2013 16:29, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> 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.
> >>> suggested patch:
> >>>
> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>> ---
> >>>  hw/scsi/virtio-scsi.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> >>> index 26d95a1..51cc929 100644
> >>> --- a/hw/scsi/virtio-scsi.c
> >>> +++ b/hw/scsi/virtio-scsi.c
> >>> @@ -147,6 +147,8 @@ 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));
> >>> +    assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
> >>> +    assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));
> >>
> >> Wouldn't it be better to fail migration, as other patches in
> >> this series do? "Silent security hole if you compile with
> >> -DNDEBUG" is a little bit unfriendly...
> > 
> > The problem is that SCSIBusInfo's load_request cannot fail.  I can look
> > at fixing it on top of this series.
> 
> Paolo, with this series not yet in, am I seeing correctly that no
> changes to allow failing this have been committed? Can you consider
> coordinating this with mst?
> 
> Regards,
> Andreas

BTW I don't see a lot of value in supporting NDEBUG
and a bunch of other things will probably break if we do.
For now I think we should just stick
#ifdef NDEBUG
#error building with NDEBUG is not supported
#endif


> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 22/23] virtio-scsi: fix buffer overrun on invalid state load

[Paolo Bonzini]

Il 06/03/2014 19:36, Michael S. Tsirkin ha scritto:
> BTW I don't see a lot of value in supporting NDEBUG
> and a bunch of other things will probably break if we do.
> For now I think we should just stick
> #ifdef NDEBUG
> #error building with NDEBUG is not supported
> #endif

I would love to answer the Gentoo bug reports that would ensue...

Paolo

Re: [Qemu-devel] [PATCH 19/23] tsc210x: fix buffer overrun on invalid state load

[Andreas Färber]

Am 03.12.2013 17:29, schrieb Michael S. Tsirkin:
> 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.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/input/tsc210x.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c
> index 485c9e5..c7513c7 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 > ARRAY_SIZE(mode_regs)) {

Don't these need to be >= according to the commit message?

Regards,
Andreas

> +        return -EINVAL;
> +    }
>      s->nextfunction = qemu_get_byte(f);
> +    if (s->nextfunction > ARRAY_SIZE(mode_regs)) {
> +        return -EINVAL;
> +    }
>      s->precision = qemu_get_byte(f);
> +    if (s->precision > ARRAY_SIZE(resolution)) {
> +        return -EINVAL;
> +    }
>      s->nextprecision = qemu_get_byte(f);
> +    if (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);
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 22/23] virtio-scsi: fix buffer overrun on invalid state load

[Peter Maydell]

On 6 March 2014 19:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 06/03/2014 19:36, Michael S. Tsirkin ha scritto:
>
>> BTW I don't see a lot of value in supporting NDEBUG
>> and a bunch of other things will probably break if we do.
>> For now I think we should just stick
>> #ifdef NDEBUG
>> #error building with NDEBUG is not supported
>> #endif
>
>
> I would love to answer the Gentoo bug reports that would ensue...

#ifdef NDEBUG
#error building with NDEBUG is not supported
#ifdef GENTOO
#error No really, we mean it
#endif
#endif

-- PMM