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

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

[Michael S. Tsirkin]

Changes from v4:
    Addressed comments by Peter, David, Amit, Laszlo
    dropped vmxnet3 patches: will re-add when author
        addresses comments raised
    dropped stellaris patches: superceded by Peter's rewrite
    Added Peter's better fix for savevm crash

Changes from v3:
    Rewritten input validation in multiple patches using the new
    VMSTATE_VALIDATE macro.
    Addressed review comments from Peter Maydell,
    Andreas Färber, Don Koch and Dr. David Alan Gilbert.

The following is the list of patches unmodified from v4:
 vmstate: add VMS_MUST_EXIST
 vmstate: add VMSTATE_VALIDATE
 virtio-net: fix buffer overflow on invalid state load
    [patch unchanged, comment tweaked as suggested by Laszlo]
 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
 virtio: avoid buffer overrun on incoming migration
 pxa2xx: avoid buffer overrun on incoming migration
 zaurus: fix buffer overrun on invalid state load
 virtio-scsi: fix buffer overrun on invalid state load

The following is the list of patches unmodified from v1:
 virtio: out-of-bounds buffer write on invalid state load
 ahci: fix buffer overrun on invalid state load
 virtio: avoid buffer overrun on incoming migration


In some cases CVEs have been created to track specific issues.
Where available, CVE # is listed in the commit log.

I doubt it makes sense to push this urgently into 2.0.

Let's fix for 2.1, and backport as appropriate.

Cover latter from v1:
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, some bugzilla entries supply a savevm file
  and ask developer to load that to reproduce.
  
  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 master and stable.

Michael Roth (2):
  virtio: avoid buffer overrun on incoming migration
  openpic: avoid buffer overrun on incoming migration

Michael S. Tsirkin (21):
  vmstate: reduce code duplication
  vmstate: add VMS_MUST_EXIST
  vmstate: add VMSTATE_VALIDATE
  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
  vmstate: fix buffer overflow in target-arm/machine.c
  virtio: validate num_sg when mapping
  pxa2xx: avoid buffer overrun on incoming migration
  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
  vmstate: s/VMSTATE_INT32_LE/VMSTATE_INT32_POSITIVE_LE/
  usb: sanity check setup_index+setup_len in post_load

Peter Maydell (1):
  savevm: Ignore minimum_version_id_old if there is no load_state_old

 include/hw/virtio/virtio-net.h |   4 +-
 include/migration/vmstate.h    |  11 +++-
 hw/arm/pxa2xx.c                |   8 ++-
 hw/display/ssd0323.c           |  24 ++++++++
 hw/gpio/zaurus.c               |  10 ++++
 hw/ide/ahci.c                  |   2 +-
 hw/input/tsc210x.c             |  12 ++++
 hw/intc/openpic.c              |  14 ++++-
 hw/net/virtio-net.c            |  19 +++++--
 hw/pci/pci.c                   |   4 +-
 hw/pci/pcie_aer.c              |  10 +++-
 hw/scsi/virtio-scsi.c          |   9 +++
 hw/sd/ssi-sd.c                 |   8 +++
 hw/ssi/pl022.c                 |  14 +++++
 hw/timer/hpet.c                |  13 +++++
 hw/usb/bus.c                   |   4 +-
 hw/virtio/virtio.c             |  17 +++++-
 target-arm/machine.c           |   2 +-
 vmstate.c                      | 126 +++++++++++++++++++++++------------------
 docs/migration.txt             |  12 ++--
 20 files changed, 243 insertions(+), 80 deletions(-)

-- 
MST

[Qemu-devel] [PATCH v5 01/24] vmstate: reduce code duplication

[Michael S. Tsirkin]

move size offset and number of elements math out
to functions, to reduce code duplication.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
---
 vmstate.c | 100 ++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 52 insertions(+), 48 deletions(-)

diff --git a/vmstate.c b/vmstate.c
index b689f2f..dd6f834 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -10,6 +10,50 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
                                    void *opaque);
 
+static int vmstate_n_elems(void *opaque, VMStateField *field)
+{
+    int n_elems = 1;
+
+    if (field->flags & VMS_ARRAY) {
+        n_elems = field->num;
+    } else if (field->flags & VMS_VARRAY_INT32) {
+        n_elems = *(int32_t *)(opaque+field->num_offset);
+    } else if (field->flags & VMS_VARRAY_UINT32) {
+        n_elems = *(uint32_t *)(opaque+field->num_offset);
+    } else if (field->flags & VMS_VARRAY_UINT16) {
+        n_elems = *(uint16_t *)(opaque+field->num_offset);
+    } else if (field->flags & VMS_VARRAY_UINT8) {
+        n_elems = *(uint8_t *)(opaque+field->num_offset);
+    }
+
+    return n_elems;
+}
+
+static int vmstate_size(void *opaque, VMStateField *field)
+{
+    int size = field->size;
+
+    if (field->flags & VMS_VBUFFER) {
+        size = *(int32_t *)(opaque+field->size_offset);
+        if (field->flags & VMS_MULTIPLY) {
+            size *= field->size;
+        }
+    }
+
+    return size;
+}
+
+static void *vmstate_base_addr(void *opaque, VMStateField *field)
+{
+    void *base_addr = opaque + field->offset;
+
+    if (field->flags & VMS_POINTER) {
+        base_addr = *(void **)base_addr + field->start;
+    }
+
+    return base_addr;
+}
+
 int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, int version_id)
 {
@@ -36,30 +80,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
              field->field_exists(opaque, version_id)) ||
             (!field->field_exists &&
              field->version_id <= version_id)) {
-            void *base_addr = opaque + field->offset;
-            int i, n_elems = 1;
-            int size = field->size;
-
-            if (field->flags & VMS_VBUFFER) {
-                size = *(int32_t *)(opaque+field->size_offset);
-                if (field->flags & VMS_MULTIPLY) {
-                    size *= field->size;
-                }
-            }
-            if (field->flags & VMS_ARRAY) {
-                n_elems = field->num;
-            } else if (field->flags & VMS_VARRAY_INT32) {
-                n_elems = *(int32_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT32) {
-                n_elems = *(uint32_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT16) {
-                n_elems = *(uint16_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT8) {
-                n_elems = *(uint8_t *)(opaque+field->num_offset);
-            }
-            if (field->flags & VMS_POINTER) {
-                base_addr = *(void **)base_addr + field->start;
-            }
+            void *base_addr = vmstate_base_addr(opaque, field);
+            int i, n_elems = vmstate_n_elems(opaque, field);
+            int size = vmstate_size(opaque, field);
+
             for (i = 0; i < n_elems; i++) {
                 void *addr = base_addr + size * i;
 
@@ -102,30 +126,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
     while (field->name) {
         if (!field->field_exists ||
             field->field_exists(opaque, vmsd->version_id)) {
-            void *base_addr = opaque + field->offset;
-            int i, n_elems = 1;
-            int size = field->size;
-
-            if (field->flags & VMS_VBUFFER) {
-                size = *(int32_t *)(opaque+field->size_offset);
-                if (field->flags & VMS_MULTIPLY) {
-                    size *= field->size;
-                }
-            }
-            if (field->flags & VMS_ARRAY) {
-                n_elems = field->num;
-            } else if (field->flags & VMS_VARRAY_INT32) {
-                n_elems = *(int32_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT32) {
-                n_elems = *(uint32_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT16) {
-                n_elems = *(uint16_t *)(opaque+field->num_offset);
-            } else if (field->flags & VMS_VARRAY_UINT8) {
-                n_elems = *(uint8_t *)(opaque+field->num_offset);
-            }
-            if (field->flags & VMS_POINTER) {
-                base_addr = *(void **)base_addr + field->start;
-            }
+            void *base_addr = vmstate_base_addr(opaque, field);
+            int i, n_elems = vmstate_n_elems(opaque, field);
+            int size = vmstate_size(opaque, field);
+
             for (i = 0; i < n_elems; i++) {
                 void *addr = base_addr + size * i;
 
-- 
MST

[Qemu-devel] [PATCH v5 03/24] vmstate: add VMSTATE_VALIDATE

[Michael S. Tsirkin]

Validate state using VMS_ARRAY with num = 0 and VMS_MUST_EXIST

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/migration/vmstate.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index de970ab..5b71370 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -204,6 +204,14 @@ extern const VMStateInfo vmstate_info_bitmap;
     .offset       = vmstate_offset_value(_state, _field, _type),     \
 }
 
+/* Validate state using a boolean predicate. */
+#define VMSTATE_VALIDATE(_name, _test) { \
+    .name         = (_name),                                         \
+    .field_exists = (_test),                                         \
+    .flags        = VMS_ARRAY | VMS_MUST_EXIST,                      \
+    .num          = 0, /* 0 elements: no data, only run _test */     \
+}
+
 #define VMSTATE_POINTER(_field, _state, _version, _info, _type) {    \
     .name       = (stringify(_field)),                               \
     .version_id = (_version),                                        \
-- 
MST

[Qemu-devel] [PATCH v5 04/24] 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.

Fix it by making the value unsigned.
For consistency, change first_multi as well.

Note: all call sites were audited to confirm that
making them unsigned didn't cause any issues:
it turns out we actually never do math on them,
so it's easy to validate because both values are
always <= MAC_TABLE_ENTRIES.

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@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 v5 05/24] 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 | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 439477b..c247529 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1362,10 +1362,16 @@ 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 {
+            int 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.
+             */
+            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 v5 06/24] 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 c247529..2a702e3 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1413,6 +1413,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 v5 07/24] 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 aeabf3a..9008430 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) {
-- 
MST

[Qemu-devel] [PATCH v5 08/24] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 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 bfe633f..457a7a1 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()
     },
 };
-- 
MST

[Qemu-devel] [PATCH v5 09/24] 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 VMSTATE_VALID hook.

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

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()
-- 
MST

[Qemu-devel] [PATCH v5 10/24] 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 corruption
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>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/pci/pcie_aer.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

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()
-- 
MST

[Qemu-devel] [PATCH v5 11/24] 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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

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),
-- 
MST

[Qemu-devel] [PATCH v5 12/24] vmstate: fix buffer overflow in target-arm/machine.c

[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.

VMSTATE_INT32_LE was supposed to protect against this
but doesn't because it doesn't validate that input is
non-negative.

Fix this macro to valide the value appropriately.

The only other user of VMSTATE_INT32_LE doesn't
ever use negative numbers so it doesn't care.

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

diff --git a/vmstate.c b/vmstate.c
index f019228..dbb7666 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -337,8 +337,9 @@ const VMStateInfo vmstate_info_int32_equal = {
     .put  = put_int32,
 };
 
-/* 32 bit int. Check that the received value is less than or equal to
-   the one in the field */
+/* 32 bit int. Check that the received value is non-negative
+ * and less than or equal to the one in the field.
+ */
 
 static int get_int32_le(QEMUFile *f, void *pv, size_t size)
 {
@@ -346,7 +347,7 @@ static int get_int32_le(QEMUFile *f, void *pv, size_t size)
     int32_t loaded;
     qemu_get_sbe32s(f, &loaded);
 
-    if (loaded <= *cur) {
+    if (loaded >= 0 && loaded <= *cur) {
         *cur = loaded;
         return 0;
     }
-- 
MST

[Qemu-devel] [PATCH v5 13/24] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/virtio/virtio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9008430..bcbfbb2 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) {
-- 
MST

[Qemu-devel] [PATCH v5 14/24] 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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index be76fbd..e63ccf2 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -1416,7 +1416,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 +1428,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 +1571,12 @@ 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", 0, MAX_CPU);
+        return;
+    }
+
     switch (opp->model) {
     case OPENPIC_MODEL_FSL_MPIC_20:
     default:
-- 
MST

[Qemu-devel] [PATCH v5 15/24] 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>
Cc: Amit Shah <amit.shah@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 bcbfbb2..3bad71e 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);
-- 
MST

[Qemu-devel] [PATCH v5 16/24] pxa2xx: avoid buffer overrun on incoming migration

[Michael S. Tsirkin]

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 <dkoch@verizon.com>
Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Don Koch <dkoch@verizon.com>
---
 hw/arm/pxa2xx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

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);
-- 
MST

[Qemu-devel] [PATCH v5 17/24] 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 3273c8a..2fa2b2b 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -230,6 +230,14 @@ 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;
+    }
+    if (s->mode == SSI_SD_RESPONSE &&
+        (s->response_pos < 0 || s->response_pos > ARRAY_SIZE(s->response))) {
+        return -EINVAL;
+    }
     s->response_pos = qemu_get_be32(f);
     s->stopping = qemu_get_be32(f);
 
-- 
MST

[Qemu-devel] [PATCH v5 18/24] 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.

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

diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
index 971152e..d4b9ea3 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);
-- 
MST

[Qemu-devel] [PATCH v5 19/24] 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.

Cc: Andreas Färber <afaerber@suse.de>
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..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);
-- 
MST

[Qemu-devel] [PATCH v5 20/24] 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 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 <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/gpio/zaurus.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/gpio/zaurus.c b/hw/gpio/zaurus.c
index dc79a8b..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),
-- 
MST

[Qemu-devel] [PATCH v5 21/24] 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.

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 <afaerber@suse.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/scsi/virtio-scsi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

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);
-- 
MST

[Qemu-devel] [PATCH v5 22/24] vmstate: s/VMSTATE_INT32_LE/VMSTATE_INT32_POSITIVE_LE/

[Michael S. Tsirkin]

As the macro verifies the value is positive, rename it
to make the function clearer.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/migration/vmstate.h | 2 +-
 hw/pci/pci.c                | 4 ++--
 target-arm/machine.c        | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 5b71370..7e45048 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -601,7 +601,7 @@ extern const VMStateInfo vmstate_info_bitmap;
 #define VMSTATE_UINT64_EQUAL(_f, _s)                                  \
     VMSTATE_UINT64_EQUAL_V(_f, _s, 0)
 
-#define VMSTATE_INT32_LE(_f, _s)                                   \
+#define VMSTATE_INT32_POSITIVE_LE(_f, _s)                             \
     VMSTATE_SINGLE(_f, _s, 0, vmstate_info_int32_le, int32_t)
 
 #define VMSTATE_UINT8_TEST(_f, _s, _t)                               \
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),
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 7ced87a..5746ffd 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -246,7 +246,7 @@ const VMStateDescription vmstate_arm_cpu = {
         /* The length-check must come before the arrays to avoid
          * incoming data possibly overflowing the array.
          */
-        VMSTATE_INT32_LE(cpreg_vmstate_array_len, ARMCPU),
+        VMSTATE_INT32_POSITIVE_LE(cpreg_vmstate_array_len, ARMCPU),
         VMSTATE_VARRAY_INT32(cpreg_vmstate_indexes, ARMCPU,
                              cpreg_vmstate_array_len,
                              0, vmstate_info_uint64, uint64_t),
-- 
MST

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

[Michael S. Tsirkin]

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 <kraxel@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/usb/bus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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;
     }
-- 
MST

[Qemu-devel] [PATCH v5 24/24] savevm: Ignore minimum_version_id_old if there is no load_state_old

[Michael S. Tsirkin]

From: Peter Maydell <peter.maydell@linaro.org>

At the moment we require vmstate definitions to set minimum_version_id_old
to the same value as minimum_version_id if they do not provide a
load_state_old handler. Since the load_state_old functionality is
required only for a handful of devices that need to retain migration
compatibility with a pre-vmstate implementation, this means the bulk
of devices have pointless boilerplate. Relax the definition so that
minimum_version_id_old is ignored if there is no load_state_old handler.

Note that under the old scheme we would segfault if the vmstate
specified a minimum_version_id_old that was less than minimum_version_id
but did not provide a load_state_old function, and the incoming state
specified a version number between minimum_version_id_old and
minimum_version_id. Under the new scheme this will just result in
our failing the migration.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 vmstate.c          |  9 +++++----
 docs/migration.txt | 12 +++++-------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/vmstate.c b/vmstate.c
index dbb7666..b5882fa 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -63,11 +63,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
     if (version_id > vmsd->version_id) {
         return -EINVAL;
     }
-    if (version_id < vmsd->minimum_version_id_old) {
-        return -EINVAL;
-    }
     if  (version_id < vmsd->minimum_version_id) {
-        return vmsd->load_state_old(f, opaque, version_id);
+        if (vmsd->load_state_old &&
+            version_id >= vmsd->minimum_version_id_old) {
+            return vmsd->load_state_old(f, opaque, version_id);
+        }
+        return -EINVAL;
     }
     if (vmsd->pre_load) {
         int ret = vmsd->pre_load(opaque);
diff --git a/docs/migration.txt b/docs/migration.txt
index 0e0a1d4..fe1f2bb 100644
--- a/docs/migration.txt
+++ b/docs/migration.txt
@@ -139,7 +139,6 @@ static const VMStateDescription vmstate_kbd = {
     .name = "pckbd",
     .version_id = 3,
     .minimum_version_id = 3,
-    .minimum_version_id_old = 3,
     .fields      = (VMStateField []) {
         VMSTATE_UINT8(write_cmd, KBDState),
         VMSTATE_UINT8(status, KBDState),
@@ -168,12 +167,13 @@ You can see that there are several version fields:
 - minimum_version_id: the minimum version_id that VMState is able to understand
   for that device.
 - minimum_version_id_old: For devices that were not able to port to vmstate, we can
-  assign a function that knows how to read this old state.
+  assign a function that knows how to read this old state. This field is
+  ignored if there is no load_state_old handler.
 
 So, VMState is able to read versions from minimum_version_id to
-version_id.  And the function load_state_old() is able to load state
-from minimum_version_id_old to minimum_version_id.  This function is
-deprecated and will be removed when no more users are left.
+version_id.  And the function load_state_old() (if present) is able to
+load state from minimum_version_id_old to minimum_version_id.  This
+function is deprecated and will be removed when no more users are left.
 
 ===  Massaging functions ===
 
@@ -255,7 +255,6 @@ const VMStateDescription vmstate_ide_drive_pio_state = {
     .name = "ide_drive/pio_state",
     .version_id = 1,
     .minimum_version_id = 1,
-    .minimum_version_id_old = 1,
     .pre_save = ide_drive_pio_pre_save,
     .post_load = ide_drive_pio_post_load,
     .fields      = (VMStateField []) {
@@ -275,7 +274,6 @@ const VMStateDescription vmstate_ide_drive = {
     .name = "ide_drive",
     .version_id = 3,
     .minimum_version_id = 0,
-    .minimum_version_id_old = 0,
     .post_load = ide_drive_post_load,
     .fields      = (VMStateField []) {
         .... several fields ....
-- 
MST

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

[Peter Maydell]

On 3 April 2014 17:52, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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 | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 3273c8a..2fa2b2b 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -230,6 +230,14 @@ 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;
> +    }
> +    if (s->mode == SSI_SD_RESPONSE &&
> +        (s->response_pos < 0 || s->response_pos > ARRAY_SIZE(s->response))) {
> +        return -EINVAL;
> +    }
>      s->response_pos = qemu_get_be32(f);
>      s->stopping = qemu_get_be32(f);

Surely we should read s->response_pos off the wire before
validating it rather than after?

Also, your checks on arglen aren't sufficient. Consider
the case where the attacker:
 * sets mode to SSI_SD_RESPONSE
 * sets arglen to something huge
 * sets response_pos to 0
 * gets the guest to repeatedly provoke calls to ssi_sd_transfer

We'll happily read off the end of the s->response[] buffer,
because our "when do we stop returning response bytes" check
is "s->response_pos >= s->arglen".

thanks
-- PMM

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

[Peter Maydell]

On 3 April 2014 17:52, 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.
>
> 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 <mst@redhat.com>
> ---
>  hw/display/ssd0323.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
> index 971152e..d4b9ea3 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 ) {

Spurious space before ')' (here and below).

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

though
#define SSD0323_ROWS 128
#define SSD0323_ROWBYTES (SSD0323_ROWS / 2)
#define SSD0323_COLS 64

and using the symbolic constants here and elsewhere
in the device wouldn't be a bad idea at some point.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 05/24] virtio-net: out-of-bounds buffer write on load

[Peter Maydell]

On 3 April 2014 17:50, 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 | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 439477b..c247529 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1362,10 +1362,16 @@ 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 {
> +            int 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.
> +             */
> +            for (i = 0; i < n->mac_table.in_use * ETH_ALEN; ++i) {
> +                qemu_get_byte(f);
> +            }

If the incoming data sets the in_use field to INT_MAX then
we get integer overflow on the multiply here. That's
undefined behaviour in C and we probably shouldn't allow
that to happen.

thanks
-- PMM

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

[Michael S. Tsirkin]

On Thu, Apr 03, 2014 at 06:05:03PM +0100, Peter Maydell wrote:
> On 3 April 2014 17:52, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 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 | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> > index 3273c8a..2fa2b2b 100644
> > --- a/hw/sd/ssi-sd.c
> > +++ b/hw/sd/ssi-sd.c
> > @@ -230,6 +230,14 @@ 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;
> > +    }
> > +    if (s->mode == SSI_SD_RESPONSE &&
> > +        (s->response_pos < 0 || s->response_pos > ARRAY_SIZE(s->response))) {

And actually it should be s->response_pos >= ARRAY_SIZE(s->response),
right?

> > +        return -EINVAL;
> > +    }
> >      s->response_pos = qemu_get_be32(f);
> >      s->stopping = qemu_get_be32(f);
> 
> Surely we should read s->response_pos off the wire before
> validating it rather than after?
> 
> Also, your checks on arglen aren't sufficient. Consider
> the case where the attacker:
>  * sets mode to SSI_SD_RESPONSE
>  * sets arglen to something huge
>  * sets response_pos to 0
>  * gets the guest to repeatedly provoke calls to ssi_sd_transfer
> 
> We'll happily read off the end of the s->response[] buffer,
> because our "when do we stop returning response bytes" check
> is "s->response_pos >= s->arglen".
> 
> thanks
> -- PMM

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

[Alexander Graf]

On 03.04.2014, at 18:51, Michael S. Tsirkin <mst@redhat.com> wrote:

> 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>

Reviewed-by: Alexander Graf <agraf@suse.de>

Though I don't think the nb_cpus checks are necessary, it's always nice to be safe rather than sorry :).


Alex

> ---
> hw/intc/openpic.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index be76fbd..e63ccf2 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -1416,7 +1416,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 +1428,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 +1571,12 @@ 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", 0, MAX_CPU);
> +        return;
> +    }
> +
>     switch (opp->model) {
>     case OPENPIC_MODEL_FSL_MPIC_20:
>     default:
> -- 
> MST
> 

Re: [Qemu-devel] [PATCH v5 01/24] vmstate: reduce code duplication

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> move size offset and number of elements math out
> to functions, to reduce code duplication.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Added to migration branch for 2.1

Re: [Qemu-devel] [PATCH v5 02/24] vmstate: add VMS_MUST_EXIST

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Can be used to verify a required field exists or validate
> state in some other way.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


What should this do?

We can change the semantics of ->field_exist() to add this functionality
if needed, no?

> ---
>  include/migration/vmstate.h |  1 +
>  vmstate.c                   | 10 ++++++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index e7e1705..de970ab 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -100,6 +100,7 @@ enum VMStateFlags {
>      VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
>      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
>      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
> +    VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
>  };
>  
>  typedef struct {
> diff --git a/vmstate.c b/vmstate.c
> index dd6f834..f019228 100644
> --- a/vmstate.c
> +++ b/vmstate.c
> @@ -102,6 +102,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>                      return ret;
>                  }
>              }
> +        } else if (field->flags & VMS_MUST_EXIST) {
> +            fprintf(stderr, "Input validation failed: %s/%s\n",
> +                    vmsd->name, field->name);
> +            return -1;
>          }
>          field++;
>      }
> @@ -142,6 +146,12 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>                      field->info->put(f, addr, size);
>                  }
>              }
> +        } else {
> +            if (field->flags & VMS_MUST_EXIST) {
> +                fprintf(stderr, "Output state validation failed: %s/%s\n",
> +                        vmsd->name, field->name);
> +                assert(!(field->flags & VMS_MUST_EXIST));
> +            }
>          }
>          field++;
>      }

Re: [Qemu-devel] [PATCH v5 12/24] vmstate: fix buffer overflow in target-arm/machine.c

[Juan Quintela]

"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.
>
> VMSTATE_INT32_LE was supposed to protect against this
> but doesn't because it doesn't validate that input is
> non-negative.
>
> Fix this macro to valide the value appropriately.
>
> The only other user of VMSTATE_INT32_LE doesn't
> ever use negative numbers so it doesn't care.
>
> Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Integrated.

Re: [Qemu-devel] [PATCH v5 22/24] vmstate: s/VMSTATE_INT32_LE/VMSTATE_INT32_POSITIVE_LE/

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> As the macro verifies the value is positive, rename it
> to make the function clearer.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH v5 24/24] savevm: Ignore minimum_version_id_old if there is no load_state_old

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> From: Peter Maydell <peter.maydell@linaro.org>
>
> At the moment we require vmstate definitions to set minimum_version_id_old
> to the same value as minimum_version_id if they do not provide a
> load_state_old handler. Since the load_state_old functionality is
> required only for a handful of devices that need to retain migration
> compatibility with a pre-vmstate implementation, this means the bulk
> of devices have pointless boilerplate. Relax the definition so that
> minimum_version_id_old is ignored if there is no load_state_old handler.
>
> Note that under the old scheme we would segfault if the vmstate
> specified a minimum_version_id_old that was less than minimum_version_id
> but did not provide a load_state_old function, and the incoming state
> specified a version number between minimum_version_id_old and
> minimum_version_id. Under the new scheme this will just result in
> our failing the migration.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Applied

Re: [Qemu-devel] [PATCH v5 09/24] hpet: fix buffer overrun on invalid state load

[Juan Quintela]

"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 VMSTATE_VALID hook.
>
> Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Ok, seing what you want to do with VMSTATE_VALIDATE.
Much better solution is add a ->validate() field, and use it in the
equivalent of LESS_EQUAL and rest of tests.

Will sent a patch.

Later, Juan.

> ---
>  hw/timer/hpet.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> 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()

Re: [Qemu-devel] [PATCH v5 02/24] vmstate: add VMS_MUST_EXIST

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > Can be used to verify a required field exists or validate
> > state in some other way.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> 
> What should this do?
> 
> We can change the semantics of ->field_exist() to add this functionality
> if needed, no?

Well that's what Michael is doing; he's changing the use of ->field_exist()
here, so that if this flag is set then a failure of field_exist causes a migration
failure.

Dave

> 
> > ---
> >  include/migration/vmstate.h |  1 +
> >  vmstate.c                   | 10 ++++++++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index e7e1705..de970ab 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -100,6 +100,7 @@ enum VMStateFlags {
> >      VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
> >      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
> >      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
> > +    VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
> >  };
> >  
> >  typedef struct {
> > diff --git a/vmstate.c b/vmstate.c
> > index dd6f834..f019228 100644
> > --- a/vmstate.c
> > +++ b/vmstate.c
> > @@ -102,6 +102,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >                      return ret;
> >                  }
> >              }
> > +        } else if (field->flags & VMS_MUST_EXIST) {
> > +            fprintf(stderr, "Input validation failed: %s/%s\n",
> > +                    vmsd->name, field->name);
> > +            return -1;
> >          }
> >          field++;
> >      }
> > @@ -142,6 +146,12 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >                      field->info->put(f, addr, size);
> >                  }
> >              }
> > +        } else {
> > +            if (field->flags & VMS_MUST_EXIST) {
> > +                fprintf(stderr, "Output state validation failed: %s/%s\n",
> > +                        vmsd->name, field->name);
> > +                assert(!(field->flags & VMS_MUST_EXIST));
> > +            }
> >          }
> >          field++;
> >      }
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v5 09/24] hpet: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

On Fri, Apr 04, 2014 at 11:51:52AM +0200, Juan Quintela wrote:
> "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 VMSTATE_VALID hook.
> >
> > Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> 
> Ok, seing what you want to do with VMSTATE_VALIDATE.

It does not seem clean to add a callback that is only
be used with a single type.

> Much better solution is add a ->validate() field, and use it in the
> equivalent of LESS_EQUAL and rest of tests.
> 
> Will sent a patch.
> 
> Later, Juan.

I think it's better to stick to plain C, extending macros like
LESS_EQUAL is too tricky, vmstate is already full ugly one-off macros.

> > ---
> >  hw/timer/hpet.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > 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()

Re: [Qemu-devel] [PATCH v5 09/24] hpet: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

On Fri, Apr 04, 2014 at 05:47:39PM +0300, Michael S. Tsirkin wrote:
> On Fri, Apr 04, 2014 at 11:51:52AM +0200, Juan Quintela wrote:
> > "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 VMSTATE_VALID hook.
> > >
> > > Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > 
> > Ok, seing what you want to do with VMSTATE_VALIDATE.
> 
> It does not seem clean to add a callback that is only
> be used with a single type.
> 
> > Much better solution is add a ->validate() field, and use it in the
> > equivalent of LESS_EQUAL and rest of tests.
> > 
> > Will sent a patch.
> > 
> > Later, Juan.
> 
> I think it's better to stick to plain C, extending macros like
> LESS_EQUAL is too tricky, vmstate is already full ugly one-off macros.

Sent too quick, phrase is cut off short

... let's not add any more, let's use a generic macro like
VMSTATE_VALIDATE.


> > > ---
> > >  hw/timer/hpet.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > 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()

Re: [Qemu-devel] [PATCH v5 09/24] hpet: fix buffer overrun on invalid state load

[Michael S. Tsirkin]

On Fri, Apr 04, 2014 at 06:04:50PM +0300, Michael S. Tsirkin wrote:
> On Fri, Apr 04, 2014 at 05:47:39PM +0300, Michael S. Tsirkin wrote:
> > On Fri, Apr 04, 2014 at 11:51:52AM +0200, Juan Quintela wrote:
> > > "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 VMSTATE_VALID hook.
> > > >
> > > > Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > 
> > > 
> > > Ok, seing what you want to do with VMSTATE_VALIDATE.
> > 
> > It does not seem clean to add a callback that is only
> > be used with a single type.
> > 
> > > Much better solution is add a ->validate() field, and use it in the
> > > equivalent of LESS_EQUAL and rest of tests.
> > > 
> > > Will sent a patch.
> > > 
> > > Later, Juan.
> > 
> > I think it's better to stick to plain C, extending macros like
> > LESS_EQUAL is too tricky, vmstate is already full ugly one-off macros.
> 
> Sent too quick, phrase is cut off short
> 
> ... let's not add any more, let's use a generic macro like
> VMSTATE_VALIDATE.
> 

Hmm and I notice this still sounds rude and unclear.

Pls exuse the tone and the confusion, I wrote this in a hurry, I'm sorry
about that.

It's weekend here - I'll write a clearer response next week.


> > > > ---
> > > >  hw/timer/hpet.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > 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()

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

[Gerd Hoffmann]

On Do, 2014-04-03 at 19:52 +0300, Michael S. Tsirkin wrote:
> -    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;
>      }

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>