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

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

[Michael S. Tsirkin]

Changes from previous version:
    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 v1:
 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
 pl022: fix buffer overun on invalid state load
 virtio: avoid buffer overrun on incoming migration
 openpic: avoid buffer overrun on incoming migration
 virtio: validate num_sg when mapping
 ssi-sd: fix buffer overrun on invalid state load
 usb: sanity check setup_index+setup_len in post_load
 savevm: fix potential segfault on invalid state

New patches for vmxnet3 have been added.

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.

The patches is still under test. However, I'm unlikely to
be able to properly test all affected hardware.
Testing reports, review, acks will be appreciated.

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.




Dmitry Fleytman (4):
  vmxnet3: validate interrupt indices coming from guest
  vmxnet3: validate interrupt indices read on migration
  vmxnet3: validate queues configuration coming from quest
  vmxnet3: validate queues configuration read on migration

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

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

Michael S. Tsirkin (23):
  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
  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: 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
  savevm: fix potential segfault on invalid state

 include/hw/virtio/virtio-net.h |   4 +-
 include/migration/vmstate.h    |   9 ++++
 hw/arm/pxa2xx.c                |   8 ++-
 hw/display/ssd0323.c           |   9 ++++
 hw/gpio/zaurus.c               |  10 ++++
 hw/ide/ahci.c                  |   2 +-
 hw/input/tsc210x.c             |  12 +++++
 hw/intc/openpic.c              |   3 ++
 hw/net/stellaris_enet.c        |  38 ++++++++++---
 hw/net/virtio-net.c            |  17 ++++--
 hw/net/vmxnet3.c               |  52 ++++++++++++++++--
 hw/pci/pcie_aer.c              |  10 +++-
 hw/scsi/virtio-scsi.c          |   9 ++++
 hw/sd/ssi-sd.c                 |   3 ++
 hw/ssi/pl022.c                 |  12 +++++
 hw/timer/hpet.c                |  13 +++++
 hw/usb/bus.c                   |   4 ++
 hw/virtio/virtio.c             |  17 +++++-
 vmstate.c                      | 117 ++++++++++++++++++++++++-----------------
 19 files changed, 282 insertions(+), 67 deletions(-)

-- 
MST

[Qemu-devel] [PATCH v4 01/30] 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>
---
 vmstate.c | 97 ++++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 52 insertions(+), 45 deletions(-)

diff --git a/vmstate.c b/vmstate.c
index b689f2f..dc99e1a 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,27 +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);
-            }
+            void *base_addr = vmstate_base_addr(opaque, field);
+            int i, n_elems = vmstate_n_elems(opaque, field);
+            int size = vmstate_size(opaque, field);
+
             if (field->flags & VMS_POINTER) {
                 base_addr = *(void **)base_addr + field->start;
             }
-- 
MST

[Qemu-devel] [PATCH v4 02/30] vmstate: add VMS_MUST_EXIST

[Michael S. Tsirkin]

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>
---
 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 dc99e1a..02d5d94 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++;
     }
@@ -145,6 +149,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++;
     }
-- 
MST

[Qemu-devel] [PATCH v4 03/30] 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 v4 04/30] 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>
Edit: "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.
---
 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 v4 05/30] 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 | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 439477b..8d037b1 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1363,9 +1363,15 @@ 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;
+
+            /* 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 v4 06/30] 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 8d037b1..c811fbd 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 v4 07/30] 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 v4 08/30] 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 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 v4 09/30] 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 v4 10/30] 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>
---
 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 v4 11/30] 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 v4 12/30] 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 02d5d94..e1e9cae 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -340,8 +340,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)
 {
@@ -349,7 +350,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 v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration

[Michael S. Tsirkin]

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 failing migration if s->next_packet we read from
the wire exceeds this.

Similarly, validate rx_fifo against sizeof(s->rx[].data).

Finally, constrain rx len to a sensibly small positive
value, to avoid integer overruns when data model
later uses this value.

Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index d04e6a4..182fd3e 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
 static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
 {
     stellaris_enet_state *s = (stellaris_enet_state *)opaque;
-    int i;
+    int i, v;
 
     if (version_id != 1)
         return -EINVAL;
@@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
         qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
 
     }
-    s->next_packet = qemu_get_be32(f);
-    s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
-    s->rx_fifo_len = qemu_get_be32(f);
+    v = qemu_get_be32(f);
+    if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
+        return -EINVAL;
+    }
+    s->next_packet = v;
+    v = qemu_get_be32(f);
+    if (v < 0 || v >= sizeof(s->rx[i].data)) {
+        return -EINVAL;
+    }
+    s->rx_fifo = s->rx[s->next_packet].data + v;
+    v = qemu_get_be32(f);
+    /* Set limit low enough to avoid integer overflow when
+     * we do math on len later, but high enough to avoid
+     * truncating any packets.
+     */
+    if (v < 0 || v >= 0x100000) {
+        return -EINVAL;
+    }
+    s->rx_fifo_len = v;
 
     return 0;
 }
-- 
MST

[Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2)

[Michael S. Tsirkin]

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 its execution.

Fix this by failing migration if the value from the wire would make
guest access the array out of bounds.

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

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 182fd3e..aed00fd 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
 static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
 {
     stellaris_enet_state *s = (stellaris_enet_state *)opaque;
-    int i, v;
+    int i, v, sz;
 
     if (version_id != 1)
         return -EINVAL;
@@ -374,7 +374,13 @@ 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);
-    s->tx_fifo_len = qemu_get_be32(f);
+    v = qemu_get_be32(f);
+    /* How many bytes does data use in tx fifo. */
+    sz = s->tx_frame_len == -1 ? 2 : 4;
+    if (v < 0 || v >= ARRAY_SIZE(s->tx_fifo) - sz) {
+        return -EINVAL;
+    }
+    s->tx_fifo_len = v;
     qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
     for (i = 0; i < 31; i++) {
         s->rx[i].len = qemu_get_be32(f);
-- 
MST

[Qemu-devel] [PATCH v4 15/30] stellaris_enet: avoid buffer orerrun on incoming migration (part 3)

[Michael S. Tsirkin]

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.

Additionally, tx_frame_len is used to validate that tx_fifo_len
doesn't exceed the fifo bounds - the assumption being that data model
never makes it exceed 2032.

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

Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/stellaris_enet.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index aed00fd..90ff950 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -373,7 +373,11 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
     s->mtxd = qemu_get_be32(f);
     s->mrxd = qemu_get_be32(f);
     s->np = qemu_get_be32(f);
-    s->tx_frame_len = qemu_get_be32(f);
+    v = qemu_get_be32(f);
+    if (v < -1 || s->tx_frame_len > 2032) {
+        return -EINVAL;
+    }
+    s->tx_frame_len = v;
     v = qemu_get_be32(f);
     /* How many bytes does data use in tx fifo. */
     sz = s->tx_frame_len == -1 ? 2 : 4;
-- 
MST

[Qemu-devel] [PATCH v4 16/30] 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 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 v4 17/30] 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 be76fbd..8cb16da 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 v4 18/30] 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 bcbfbb2..003b6ad 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: %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 v4 19/30] 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>
---
 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 v4 20/30] 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 3273c8a..d9c4aee 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 v4 21/30] 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
index 971152e..b520c69 100644
--- a/hw/display/ssd0323.c
+++ b/hw/display/ssd0323.c
@@ -312,13 +312,22 @@ 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);
     s->row_end = qemu_get_be32(f);
     s->col = qemu_get_be32(f);
+    if (s->col < 0 || s->col >= 64 ) {
+        return -EINVAL;
+    }
     s->col_start = qemu_get_be32(f);
     s->col_end = qemu_get_be32(f);
     s->redraw = qemu_get_be32(f);
-- 
MST

[Qemu-devel] [PATCH v4 22/30] 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..65a0d08 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 < 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 v4 23/30] 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>
---
 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 v4 24/30] 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 fe70429..8052bf1 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -53,6 +53,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 v4 25/30] 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 v4 26/30] 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).

Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 vmstate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/vmstate.c b/vmstate.c
index e1e9cae..5451fd2 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -67,6 +67,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

[Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest

[Michael S. Tsirkin]

From: Dmitry Fleytman <dmitry@daynix.com>

CVE-2013-4544

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 5be807c..a4b5c11 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -52,6 +52,9 @@
 #define VMXNET3_DEVICE_VERSION    0x1
 #define VMXNET3_DEVICE_REVISION   0x1
 
+/* Number of interrupt vectors for INTx/MSI */
+#define VMXNET3_MAX_NMSIX_INTRS   (1)
+
 /* Macros for rings descriptors access */
 #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
     (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
@@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
            (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
 }
 
+static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
+{
+    int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
+    if (idx >= max_ints) {
+        hw_error("Bad interrupt index: %d\n", idx);
+    }
+}
+
+static void vmxnet3_validate_interrupts(VMXNET3State *s)
+{
+    int i;
+
+    VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
+    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
+
+    for (i = 0; i < s->txq_num; i++) {
+        int idx = s->txq_descr[i].intr_idx;
+        VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
+        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
+    }
+
+    for (i = 0; i < s->rxq_num; i++) {
+        int idx = s->rxq_descr[i].intr_idx;
+        VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
+        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
+    }
+}
+
 static void vmxnet3_activate_device(VMXNET3State *s)
 {
     int i;
@@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
                sizeof(s->rxq_descr[i].rxq_stats));
     }
 
+    vmxnet3_validate_interrupts(s);
+
     /* Make sure everything is in place before device activation */
     smp_wmb();
 
@@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
     }
 }
 
-#define VMXNET3_MSI_NUM_VECTORS   (1)
 #define VMXNET3_MSI_OFFSET        (0x50)
 #define VMXNET3_USE_64BIT         (true)
 #define VMXNET3_PER_VECTOR_MASK   (false)
@@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
     PCIDevice *d = PCI_DEVICE(s);
     int res;
 
-    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
+    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
                    VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
     if (0 > res) {
         VMW_WRPRN("Failed to initialize MSI, error %d", res);
-- 
MST

[Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration

[Michael S. Tsirkin]

From: Dmitry Fleytman <dmitry@daynix.com>

CVE-2013-4544

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/vmxnet3.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index a4b5c11..8c6df05 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2374,6 +2374,8 @@ static int vmxnet3_post_load(void *opaque, int version_id)
         }
     }
 
+    vmxnet3_validate_interrupts(s);
+
     return 0;
 }
 
-- 
MST

[Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest

[Michael S. Tsirkin]

From: Dmitry Fleytman <dmitry@daynix.com>

CVE-2013-4544

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/vmxnet3.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 8c6df05..0fa54e7 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts(VMXNET3State *s)
     }
 }
 
+static void vmxnet3_validate_queues(VMXNET3State *s)
+{
+    if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
+        hw_error("Bad TX queues number: %d\n", s->txq_num);
+    }
+
+    if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
+        hw_error("Bad RX queues number: %d\n", s->rxq_num);
+    }
+}
+
 static void vmxnet3_activate_device(VMXNET3State *s)
 {
     int i;
@@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
         VMXNET3_READ_DRV_SHARED8(s->drv_shmem, devRead.misc.numRxQueues);
 
     VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
-    assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
+    vmxnet3_validate_queues(s);
 
     qdescr_table_pa =
         VMXNET3_READ_DRV_SHARED64(s->drv_shmem, devRead.misc.queueDescPA);
-- 
MST

[Qemu-devel] [PATCH v4 30/30] vmxnet3: validate queues configuration read on migration

[Michael S. Tsirkin]

From: Dmitry Fleytman <dmitry@daynix.com>

CVE-2013-4544

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/vmxnet3.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 0fa54e7..d062049 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2385,6 +2385,7 @@ static int vmxnet3_post_load(void *opaque, int version_id)
         }
     }
 
+    vmxnet3_validate_queues(s);
     vmxnet3_validate_interrupts(s);
 
     return 0;
-- 
MST

Re: [Qemu-devel] [PATCH v4 01/30] vmstate: reduce code duplication

[Dr. David Alan Gilbert]

* 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>
> ---
>  vmstate.c | 97 ++++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 52 insertions(+), 45 deletions(-)
> 
> diff --git a/vmstate.c b/vmstate.c
> index b689f2f..dc99e1a 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,27 +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);
> -            }
> +            void *base_addr = vmstate_base_addr(opaque, field);
> +            int i, n_elems = vmstate_n_elems(opaque, field);
> +            int size = vmstate_size(opaque, field);
> +
>              if (field->flags & VMS_POINTER) {
>                  base_addr = *(void **)base_addr + field->start;
>              }

Hmm, shouldn't those last 3 lines be deleted as well - the logic is
now in vmstate_base_addr?


Dave

> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 11/30] pl022: fix buffer overun on invalid state load

[Peter Maydell]

On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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;

Shouldn't these be '>=' checks? Also if the
incoming values are negative then we'll go
wrong in the other direction.

Given the way we do calculations involving *_fifo_len
as well, it might be best to also sanitize those,
though I think it's not possible currently to provoke
a buffer overrun with them. (NB that the _fifo_len
fields can validly be equal to the ARRAY_SIZE, unlike
the _fifo_head fields.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 01/30] vmstate: reduce code duplication

[Michael S. Tsirkin]

On Mon, Mar 31, 2014 at 04:01:34PM +0100, Dr. David Alan Gilbert wrote:
> * 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>
> > ---
> >  vmstate.c | 97 ++++++++++++++++++++++++++++++++++-----------------------------
> >  1 file changed, 52 insertions(+), 45 deletions(-)
> > 
> > diff --git a/vmstate.c b/vmstate.c
> > index b689f2f..dc99e1a 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,27 +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);
> > -            }
> > +            void *base_addr = vmstate_base_addr(opaque, field);
> > +            int i, n_elems = vmstate_n_elems(opaque, field);
> > +            int size = vmstate_size(opaque, field);
> > +
> >              if (field->flags & VMS_POINTER) {
> >                  base_addr = *(void **)base_addr + field->start;
> >              }
> 
> Hmm, shouldn't those last 3 lines be deleted as well - the logic is
> now in vmstate_base_addr?
> 
> 
> Dave

So it should, thanks.


> > -- 
> > MST
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 19/30] pxa2xx: avoid buffer overrun on incoming migration

[Peter Maydell]

On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 08/30] ahci: fix buffer overrun on invalid state load

[Peter Maydell]

On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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>

-- PMM

Re: [Qemu-devel] [PATCH v4 21/30] ssd0323: fix buffer overun on invalid state load

[Peter Maydell]

On 31 March 2014 15:17, 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.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/display/ssd0323.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
> index 971152e..b520c69 100644
> --- a/hw/display/ssd0323.c
> +++ b/hw/display/ssd0323.c
> @@ -312,13 +312,22 @@ 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);
>      s->row_end = qemu_get_be32(f);
>      s->col = qemu_get_be32(f);
> +    if (s->col < 0 || s->col >= 64 ) {
> +        return -EINVAL;
> +    }
>      s->col_start = qemu_get_be32(f);
>      s->col_end = qemu_get_be32(f);
>      s->redraw = qemu_get_be32(f);

This isn't sufficient. You also need to 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.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 22/30] tsc210x: fix buffer overrun on invalid state load

[Peter Maydell]

On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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..65a0d08 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;
> +    }

Why no check for negative values? Also, shouldn't
this be >=, like the checks below?

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


thanks
-- PMM

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

[Peter Maydell]

On 31 March 2014 15:16, 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.

This fixes the bug, but it's rather unintuitive semantics
for an INT32_LE not to simply do a signed comparison...
Maybe rename the macro?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 20/30] ssi-sd: fix buffer overrun on invalid state load

[Peter Maydell]

On 31 March 2014 15:17, 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 | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 3273c8a..d9c4aee 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);

This is insufficient. You don't sanity check response_pos, which
means it can be used to overrun a buffer in the SSI_SD_RESPONSE
case if it's set to a negative value.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> From: Dmitry Fleytman <dmitry@daynix.com>
> 
> CVE-2013-4544
> 
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 5be807c..a4b5c11 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -52,6 +52,9 @@
>  #define VMXNET3_DEVICE_VERSION    0x1
>  #define VMXNET3_DEVICE_REVISION   0x1
>  
> +/* Number of interrupt vectors for INTx/MSI */
> +#define VMXNET3_MAX_NMSIX_INTRS   (1)
> +
>  /* Macros for rings descriptors access */
>  #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
>      (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
>             (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
>  }
>  
> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> +{
> +    int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;

Is that ?: the right way around?

> +    if (idx >= max_ints) {
> +        hw_error("Bad interrupt index: %d\n", idx);
> +    }
> +}
> +
> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
> +{
> +    int i;
> +
> +    VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> +    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> +
> +    for (i = 0; i < s->txq_num; i++) {
> +        int idx = s->txq_descr[i].intr_idx;
> +        VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> +    }
> +
> +    for (i = 0; i < s->rxq_num; i++) {
> +        int idx = s->rxq_descr[i].intr_idx;
> +        VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> +    }
> +}
> +
>  static void vmxnet3_activate_device(VMXNET3State *s)
>  {
>      int i;
> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>                 sizeof(s->rxq_descr[i].rxq_stats));
>      }
>  
> +    vmxnet3_validate_interrupts(s);
> +
>      /* Make sure everything is in place before device activation */
>      smp_wmb();
>  
> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>      }
>  }
>  
> -#define VMXNET3_MSI_NUM_VECTORS   (1)
>  #define VMXNET3_MSI_OFFSET        (0x50)
>  #define VMXNET3_USE_64BIT         (true)
>  #define VMXNET3_PER_VECTOR_MASK   (false)
> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
>      PCIDevice *d = PCI_DEVICE(s);
>      int res;
>  
> -    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
> +    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>                     VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>      if (0 > res) {
>          VMW_WRPRN("Failed to initialize MSI, error %d", res);
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Peter Maydell]

On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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 fe70429..8052bf1 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -53,6 +53,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;
>  }

(1) This patch has already been applied; looks like a rebase
merge error meant you failed to drop it.
(2) Shouldn't we be checking for setup_index and setup_len
being negative as well?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> From: Dmitry Fleytman <dmitry@daynix.com>
> 
> CVE-2013-4544
> 
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/vmxnet3.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 8c6df05..0fa54e7 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts(VMXNET3State *s)
>      }
>  }
>  
> +static void vmxnet3_validate_queues(VMXNET3State *s)
> +{
> +    if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
> +        hw_error("Bad TX queues number: %d\n", s->txq_num);
> +    }
> +
> +    if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
> +        hw_error("Bad RX queues number: %d\n", s->rxq_num);
> +    }

Why isn't that >= ?
(I agree it matches the original assert).

        Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
        Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];

static void vmxnet3_fill_stats(VMXNET3State *s)
{
    int i;
    for (i = 0; i < s->txq_num; i++) {
        cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
                                  &s->txq_descr[i].txq_stats,
                                  sizeof(s->txq_descr[i].txq_stats));
    }

so that looks like it's 0 indexed.

Dave

> +}
> +
>  static void vmxnet3_activate_device(VMXNET3State *s)
>  {
>      int i;
> @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>          VMXNET3_READ_DRV_SHARED8(s->drv_shmem, devRead.misc.numRxQueues);
>  
>      VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
> -    assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
> +    vmxnet3_validate_queues(s);
>  
>      qdescr_table_pa =
>          VMXNET3_READ_DRV_SHARED64(s->drv_shmem, devRead.misc.queueDescPA);
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 17/30] openpic: avoid buffer overrun on incoming migration

[Peter Maydell]

On 31 March 2014 15:17, 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>
> ---
>  hw/intc/openpic.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index be76fbd..8cb16da 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;
> +    }

nb_cpus is a device property so we shouldn't be reading
it off the wire at all. I would suggest treating it as
a "read but ignore value", assuming we care about migration
back-compatibility for this device, or possibly "read and
confirm it matches the value we already have".

thanks
-- PMM

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

[Peter Maydell]

On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote:
> savevm will segfault if version_id < vmsd->minimum_version_id &&
> version_id >= vmsd->minimum_version_id_old

...and the vmstate has no load_state_old handler.

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

I'd previously assumed that this was a vmstate description
bug if it happened (ie that a vmstate with
minimum_version_id_old < minimum_version_id but no
load_state_old wasn't allowed).

Rather than failing migration here, wouldn't it be better
to either:
 (a) diagnose the bug, by asserting at the earliest
     opportunity
 (b) define that the value of minimum_version_id_old is not
     relevant unless load_state_old is set

I would strongly prefer (b) -- this would allow us to
remove the now-pointless setting of minimum_version_id_old
in huge numbers of vmstate structures. (Only five devices
make use of load_state_old: acpi, apic, i440fx, pit and
the ppc cpu).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 16/30] virtio: avoid buffer overrun on incoming migration

[Peter Maydell]

On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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 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) {

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

We should probably rename VIRTIO_PCI_QUEUE_MAX at some
point -- it's not PCI transport specific.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> From: Dmitry Fleytman <dmitry@daynix.com>
> 
> CVE-2013-4544
> 
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/vmxnet3.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index a4b5c11..8c6df05 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2374,6 +2374,8 @@ static int vmxnet3_post_load(void *opaque, int version_id)
>          }
>      }
>  
> +    vmxnet3_validate_interrupts(s);
> +

When you repost to fix the other stuff, why not merge this with what
is currently #30.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> 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 failing migration if s->next_packet we read from
> the wire exceeds this.
> 
> Similarly, validate rx_fifo against sizeof(s->rx[].data).
> 
> Finally, constrain rx len to a sensibly small positive
> value, to avoid integer overruns when data model
> later uses this value.
> 
> Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index d04e6a4..182fd3e 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
>  static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>  {
>      stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> -    int i;
> +    int i, v;
>  
>      if (version_id != 1)
>          return -EINVAL;
> @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>          qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
>  
>      }

The loop that's just off the top here is:
    for (i = 0; i < 31; i++) {
        s->rx[i].len = qemu_get_be32(f);
        qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));

    }

Doesn't that 'len' need validating? I assume it's the size of the
packet in the fixed sized buffer? (??)

> -    s->next_packet = qemu_get_be32(f);
> -    s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
> -    s->rx_fifo_len = qemu_get_be32(f);
> +    v = qemu_get_be32(f);
> +    if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
> +        return -EINVAL;
> +    }
> +    s->next_packet = v;
> +    v = qemu_get_be32(f);
> +    if (v < 0 || v >= sizeof(s->rx[i].data)) {
> +        return -EINVAL;
> +    }
> +    s->rx_fifo = s->rx[s->next_packet].data + v;
> +    v = qemu_get_be32(f);
> +    /* Set limit low enough to avoid integer overflow when
> +     * we do math on len later, but high enough to avoid
> +     * truncating any packets.
> +     */
> +    if (v < 0 || v >= 0x100000) {
> +        return -EINVAL;
> +    }
> +    s->rx_fifo_len = v;

I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be within
the buffer (or I think it might be legal for the sum to point to the byte after the
buffer)?
My (quick first ever look at this code) reading is that rx_fifo and rx_fifo_len
related to the current packet in-flight; although I've not quite convinced myself
about what is supposed to happen at the end of the packet (which is why
I say rx_fifo might point just at? the end.

Dave

>  
>      return 0;
>  }
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load

[Laszlo Ersek]

On 03/31/14 16:16, Michael S. Tsirkin 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.

("first_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>
> Edit: "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.
> ---
>  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;
> 

I ran

  git grep -EHn '\<(in_use|first_multi)\>'

Many hits, hard to audit (esp. because I'm unfamiliar with the code).
Several loops with signed int loop variables. I checked cursorily.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH v4 19/30] pxa2xx: avoid buffer overrun on incoming migration

[Don Koch]

On Mon, 31 Mar 2014 17:17:08 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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: Don Koch <dkoch@verizon.com>

-d

Re: [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load

[Michael S. Tsirkin]

On Mon, Mar 31, 2014 at 07:21:30PM +0200, Laszlo Ersek wrote:
> On 03/31/14 16:16, Michael S. Tsirkin 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.
> 
> ("first_multi")

Actually it doesn't even exist: Peter commented on this in v1
and I forgot to fix up the commit log :(
I really wanted to fix the commit log to say
"make first_multi unsigned for consistency" but
forgot.

> > 
> > 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>
> > Edit: "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.
> > ---
> >  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;
> > 
> 
> I ran
> 
>   git grep -EHn '\<(in_use|first_multi)\>'
> 
> Many hits, hard to audit (esp. because I'm unfamiliar with the code).
> Several loops with signed int loop variables. I checked cursorily.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 

Re: [Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration

[Michael S. Tsirkin]

On Mon, Mar 31, 2014 at 05:33:44PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > From: Dmitry Fleytman <dmitry@daynix.com>
> > 
> > CVE-2013-4544
> > 
> > Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> > Reported-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/net/vmxnet3.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> > index a4b5c11..8c6df05 100644
> > --- a/hw/net/vmxnet3.c
> > +++ b/hw/net/vmxnet3.c
> > @@ -2374,6 +2374,8 @@ static int vmxnet3_post_load(void *opaque, int version_id)
> >          }
> >      }
> >  
> > +    vmxnet3_validate_interrupts(s);
> > +
> 
> When you repost to fix the other stuff, why not merge this with what
> is currently #30.
> 
> Dave

I generally try to avoid making random changes to other's patches,
unless absolutely necessary.
Dmitry?

> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration

[Michael S. Tsirkin]

On Mon, Mar 31, 2014 at 06:11:22PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > 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 failing migration if s->next_packet we read from
> > the wire exceeds this.
> > 
> > Similarly, validate rx_fifo against sizeof(s->rx[].data).
> > 
> > Finally, constrain rx len to a sensibly small positive
> > value, to avoid integer overruns when data model
> > later uses this value.
> > 
> > Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> > index d04e6a4..182fd3e 100644
> > --- a/hw/net/stellaris_enet.c
> > +++ b/hw/net/stellaris_enet.c
> > @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
> >  static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> >  {
> >      stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> > -    int i;
> > +    int i, v;
> >  
> >      if (version_id != 1)
> >          return -EINVAL;
> > @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> >          qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> >  
> >      }
> 
> The loop that's just off the top here is:
>     for (i = 0; i < 31; i++) {
>         s->rx[i].len = qemu_get_be32(f);
>         qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> 
>     }
> 
> Doesn't that 'len' need validating? I assume it's the size of the
> packet in the fixed sized buffer? (??)

Not that I see where it's used as such.

> > -    s->next_packet = qemu_get_be32(f);
> > -    s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
> > -    s->rx_fifo_len = qemu_get_be32(f);
> > +    v = qemu_get_be32(f);
> > +    if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
> > +        return -EINVAL;
> > +    }
> > +    s->next_packet = v;
> > +    v = qemu_get_be32(f);
> > +    if (v < 0 || v >= sizeof(s->rx[i].data)) {
> > +        return -EINVAL;
> > +    }
> > +    s->rx_fifo = s->rx[s->next_packet].data + v;
> > +    v = qemu_get_be32(f);
> > +    /* Set limit low enough to avoid integer overflow when
> > +     * we do math on len later, but high enough to avoid
> > +     * truncating any packets.
> > +     */
> > +    if (v < 0 || v >= 0x100000) {
> > +        return -EINVAL;
> > +    }
> > +    s->rx_fifo_len = v;
> 
> I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be within
> the buffer (or I think it might be legal for the sum to point to the byte after the
> buffer)?
> My (quick first ever look at this code) reading is that rx_fifo and rx_fifo_len
> related to the current packet in-flight; although I've not quite convinced myself
> about what is supposed to happen at the end of the packet (which is why
> I say rx_fifo might point just at? the end.
> 
> Dave

Actually I forgot why I wrote this last check.
Peter said we should and I thought I see the issue ...
But I no longer see what kind of damage can rx_fifo_len cause
unless validated.

I never see it used as a length - merely a counter to
detect when to increment next_packet.
And that seems to be done in a safe way ...

Peter, Dave, maybe one of you can confirm what's the
possible attack here?

> >  
> >      return 0;
> >  }
> > -- 
> > MST
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration

[Peter Maydell]

On 31 March 2014 21:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Mar 31, 2014 at 06:11:22PM +0100, Dr. David Alan Gilbert wrote:
>> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> > 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 failing migration if s->next_packet we read from
>> > the wire exceeds this.
>> >
>> > Similarly, validate rx_fifo against sizeof(s->rx[].data).
>> >
>> > Finally, constrain rx len to a sensibly small positive
>> > value, to avoid integer overruns when data model
>> > later uses this value.
>> >
>> > Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > ---
>> >  hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
>> >  1 file changed, 20 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
>> > index d04e6a4..182fd3e 100644
>> > --- a/hw/net/stellaris_enet.c
>> > +++ b/hw/net/stellaris_enet.c
>> > @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
>> >  static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>> >  {
>> >      stellaris_enet_state *s = (stellaris_enet_state *)opaque;
>> > -    int i;
>> > +    int i, v;
>> >
>> >      if (version_id != 1)
>> >          return -EINVAL;
>> > @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>> >          qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
>> >
>> >      }
>>
>> The loop that's just off the top here is:
>>     for (i = 0; i < 31; i++) {
>>         s->rx[i].len = qemu_get_be32(f);
>>         qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
>>
>>     }
>>
>> Doesn't that 'len' need validating? I assume it's the size of the
>> packet in the fixed sized buffer? (??)
>
> Not that I see where it's used as such.

In the DATA case of stellaris_enet_read() -- when the current
rx_fifo_len goes to zero we will uncritically set rx_fifo_len to
s->rx[s->next_packet].len. So we must validate that it's between
0 and 2048 (the size of the rx[].data array), otherwise further
reads from DATA will be able to run off the end of the data array
for the following packet.

>> > -    s->next_packet = qemu_get_be32(f);
>> > -    s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
>> > -    s->rx_fifo_len = qemu_get_be32(f);
>> > +    v = qemu_get_be32(f);
>> > +    if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
>> > +        return -EINVAL;
>> > +    }
>> > +    s->next_packet = v;
>> > +    v = qemu_get_be32(f);
>> > +    if (v < 0 || v >= sizeof(s->rx[i].data)) {
>> > +        return -EINVAL;
>> > +    }
>> > +    s->rx_fifo = s->rx[s->next_packet].data + v;
>> > +    v = qemu_get_be32(f);
>> > +    /* Set limit low enough to avoid integer overflow when
>> > +     * we do math on len later, but high enough to avoid
>> > +     * truncating any packets.
>> > +     */
>> > +    if (v < 0 || v >= 0x100000) {
>> > +        return -EINVAL;
>> > +    }
>> > +    s->rx_fifo_len = v;
>>
>> I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be within
>> the buffer (or I think it might be legal for the sum to point to the byte after the
>> buffer)?
>> My (quick first ever look at this code) reading is that rx_fifo and rx_fifo_len
>> related to the current packet in-flight; although I've not quite convinced myself
>> about what is supposed to happen at the end of the packet (which is why
>> I say rx_fifo might point just at? the end.

> Actually I forgot why I wrote this last check.
> Peter said we should and I thought I see the issue ...
> But I no longer see what kind of damage can rx_fifo_len cause
> unless validated.

Again, look at the DATA read logic. Every time the guest does a
DATA read, we read from the four bytes at s->rx_fifo, increment
rx_fifo by 4 and decrement rx_fifo_len by 4. When rx_fifo_len
eventually goes to zero we will (on the subsequent read) reset
both rx_fifo and rx_fifo_len from the next packet in the rx queue.
So if the incoming data sets rx_fifo_len to (let us say) 0x10000,
then the guest can cause us to read well off the end of the rx data
array. This means your check isn't tight enough -- we need to
ensure that rx_fifo and rx_fifo_len between them define a window
into the rx data and nowhere else. As David says this means you
need:

    v1 = qemu_get_be32(f);
    if (v1 < 0 || v1 > sizeof(s->rx[i].data)) {
        return -EINVAL;
    }
    s->rx_fifo = s->rx[s->next_packet].data + v1;
    v2 = qemu_get_be32(f);
    if (v2 < 0 || v1 + v2 > sizeof(s->rx[i].data)) {
        return -EINVAL;
    }
    s->rx_fifo_len = v2;

The max check on v1 is actually only there to ensure that we
don't have to think about integer overflow when we do the
upper-bound check on v1 + v2. Note that v1 == sizeof(array)
is OK if (and only if) v2 == 0.

An assert in stellaris_enet_receive() that the net code never
hands us a packet we can't fit in the array wouldn't go amiss
either, but that's a separate issue.

thanks
-- PMM

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

[Gerd Hoffmann]

> > +    if (dev->setup_index >= sizeof(dev->data_buf) ||
> > +        dev->setup_len >= sizeof(dev->data_buf)) {
> > +        return -EINVAL;
> > +    }
> >      return 0;
> >  }
> 
> (2) Shouldn't we be checking for setup_index and setup_len
> being negative as well?

Oops, they are signed, so yes, I guess we should.  Or we can just make
them unsigned, they should never ever be negative.  But I'm not fully
sure we can do that without breaking migration ...

cheers,
  Gerd

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

[Dr. David Alan Gilbert]

* 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 | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 439477b..8d037b1 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1363,9 +1363,15 @@ 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) {

You can lose that 'else if' test; 

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

to get to the else  in_use > MAC_TABLE_ENTRIES.

Dave

> -            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;
> +
> +            /* 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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 18/30] virtio: validate num_sg when mapping

[Amit Shah]

On (Mon) 31 Mar 2014 [17:17:05], Michael S. Tsirkin wrote:
> 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 bcbfbb2..003b6ad 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: %d > %d",
> +                     num_sg, VIRTQUEUE_MAX_SIZE);
> +        exit(1);

Doesn't compile; needs to be %zd because num_sg is size_t

		Amit

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

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> 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 its execution.
> 
> Fix this by failing migration if the value from the wire would make
> guest access the array out of bounds.
> 
> Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/stellaris_enet.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 182fd3e..aed00fd 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
>  static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>  {
>      stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> -    int i, v;
> +    int i, v, sz;
>  
>      if (version_id != 1)
>          return -EINVAL;
> @@ -374,7 +374,13 @@ 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);
> -    s->tx_fifo_len = qemu_get_be32(f);
> +    v = qemu_get_be32(f);
> +    /* How many bytes does data use in tx fifo. */
> +    sz = s->tx_frame_len == -1 ? 2 : 4;
> +    if (v < 0 || v >= ARRAY_SIZE(s->tx_fifo) - sz) {
> +        return -EINVAL;
> +    }
> +    s->tx_fifo_len = v;
>      qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));

I don't understand the magic '2' in that ?2:4 - but there again the 'DATA' write
case is pretty hairy.

Dave

>      for (i = 0; i < 31; i++) {
>          s->rx[i].len = qemu_get_be32(f);
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> 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.
> 
> Additionally, tx_frame_len is used to validate that tx_fifo_len
> doesn't exceed the fifo bounds - the assumption being that data model
> never makes it exceed 2032.
> 
> Fix this by failing migration if the incoming value of s->tx_frame_len
> is less than -1 (the emulation code allows from -1 as a special case)
> or if it exceeds 2032.
> 
> Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/stellaris_enet.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index aed00fd..90ff950 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -373,7 +373,11 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>      s->mtxd = qemu_get_be32(f);
>      s->mrxd = qemu_get_be32(f);
>      s->np = qemu_get_be32(f);
> -    s->tx_frame_len = qemu_get_be32(f);
> +    v = qemu_get_be32(f);
> +    if (v < -1 || s->tx_frame_len > 2032) {
> +        return -EINVAL;
> +    }
> +    s->tx_frame_len = v;

I think that's wrong; 'stellaris_enet_write' does the following:

            s->tx_frame_len = value & 0xffff;
            if (s->tx_frame_len > 2032) {
                DPRINTF("TX frame too long (%d)\n", s->tx_frame_len);
                s->tx_frame_len = 0;
                s->ris |= SE_INT_TXER;
                stellaris_enet_update(s);
            } else {
                DPRINTF("Start TX frame len=%d\n", s->tx_frame_len);
                /* The value written does not include the ethernet header.  */
                s->tx_frame_len += 14;
                if ((s->tctl & SE_TCTL_CRC) == 0)
                    s->tx_frame_len += 4;

So lets say that tx_frame_len is initially 2032 when written; 14 is added to it
at this point, and if the CRC flag is set then another 4.   Thus it seems a user
can set the value in tx_frame_len to 2032+14+4=2050  - which is a bit worrying
given the buffer is only 2048 bytes.

Dave

>      v = qemu_get_be32(f);
>      /* How many bytes does data use in tx fifo. */
>      sz = s->tx_frame_len == -1 ? 2 : 4;
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest

[Dmitry Fleytman]

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


On Mar 31, 2014, at 18:45 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:

> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> From: Dmitry Fleytman <dmitry@daynix.com>
>> 
>> CVE-2013-4544
>> 
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
>> 1 file changed, 34 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 5be807c..a4b5c11 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -52,6 +52,9 @@
>> #define VMXNET3_DEVICE_VERSION    0x1
>> #define VMXNET3_DEVICE_REVISION   0x1
>> 
>> +/* Number of interrupt vectors for INTx/MSI */
>> +#define VMXNET3_MAX_NMSIX_INTRS   (1)
>> +
>> /* Macros for rings descriptors access */
>> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
>>     (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
>> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
>>            (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
>> }
>> 
>> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
>> +{
>> +    int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
> 
> Is that ?: the right way around?

Yes, this is the right way around.
VMXNET3_MAX_NMSIX_INTRS - NMSIX is for Non-MSIx

> 
>> +    if (idx >= max_ints) {
>> +        hw_error("Bad interrupt index: %d\n", idx);
>> +    }
>> +}
>> +
>> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
>> +{
>> +    int i;
>> +
>> +    VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
>> +    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
>> +
>> +    for (i = 0; i < s->txq_num; i++) {
>> +        int idx = s->txq_descr[i].intr_idx;
>> +        VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
>> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>> +    }
>> +
>> +    for (i = 0; i < s->rxq_num; i++) {
>> +        int idx = s->rxq_descr[i].intr_idx;
>> +        VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
>> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>> +    }
>> +}
>> +
>> static void vmxnet3_activate_device(VMXNET3State *s)
>> {
>>     int i;
>> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>>                sizeof(s->rxq_descr[i].rxq_stats));
>>     }
>> 
>> +    vmxnet3_validate_interrupts(s);
>> +
>>     /* Make sure everything is in place before device activation */
>>     smp_wmb();
>> 
>> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>>     }
>> }
>> 
>> -#define VMXNET3_MSI_NUM_VECTORS   (1)
>> #define VMXNET3_MSI_OFFSET        (0x50)
>> #define VMXNET3_USE_64BIT         (true)
>> #define VMXNET3_PER_VECTOR_MASK   (false)
>> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
>>     PCIDevice *d = PCI_DEVICE(s);
>>     int res;
>> 
>> -    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
>> +    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>>                    VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>>     if (0 > res) {
>>         VMW_WRPRN("Failed to initialize MSI, error %d", res);
>> -- 
>> MST
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


[-- Attachment #2: Type: text/html, Size: 5978 bytes --]

Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest

[Dr. David Alan Gilbert]

* Dmitry Fleytman (dmitry@daynix.com) wrote:
> 
> On Mar 31, 2014, at 18:45 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> 
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> >> From: Dmitry Fleytman <dmitry@daynix.com>
> >> 
> >> CVE-2013-4544
> >> 
> >> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> >> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 34 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >> index 5be807c..a4b5c11 100644
> >> --- a/hw/net/vmxnet3.c
> >> +++ b/hw/net/vmxnet3.c
> >> @@ -52,6 +52,9 @@
> >> #define VMXNET3_DEVICE_VERSION    0x1
> >> #define VMXNET3_DEVICE_REVISION   0x1
> >> 
> >> +/* Number of interrupt vectors for INTx/MSI */
> >> +#define VMXNET3_MAX_NMSIX_INTRS   (1)
> >> +
> >> /* Macros for rings descriptors access */
> >> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
> >>     (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
> >> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
> >>            (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
> >> }
> >> 
> >> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> >> +{
> >> +    int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
> > 
> > Is that ?: the right way around?
> 
> Yes, this is the right way around.
> VMXNET3_MAX_NMSIX_INTRS - NMSIX is for Non-MSIx

Ah, thank you - makes more sense now :-)

Dave
> 
> > 
> >> +    if (idx >= max_ints) {
> >> +        hw_error("Bad interrupt index: %d\n", idx);
> >> +    }
> >> +}
> >> +
> >> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
> >> +{
> >> +    int i;
> >> +
> >> +    VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> >> +    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> >> +
> >> +    for (i = 0; i < s->txq_num; i++) {
> >> +        int idx = s->txq_descr[i].intr_idx;
> >> +        VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> >> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> >> +    }
> >> +
> >> +    for (i = 0; i < s->rxq_num; i++) {
> >> +        int idx = s->rxq_descr[i].intr_idx;
> >> +        VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> >> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> >> +    }
> >> +}
> >> +
> >> static void vmxnet3_activate_device(VMXNET3State *s)
> >> {
> >>     int i;
> >> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> >>                sizeof(s->rxq_descr[i].rxq_stats));
> >>     }
> >> 
> >> +    vmxnet3_validate_interrupts(s);
> >> +
> >>     /* Make sure everything is in place before device activation */
> >>     smp_wmb();
> >> 
> >> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
> >>     }
> >> }
> >> 
> >> -#define VMXNET3_MSI_NUM_VECTORS   (1)
> >> #define VMXNET3_MSI_OFFSET        (0x50)
> >> #define VMXNET3_USE_64BIT         (true)
> >> #define VMXNET3_PER_VECTOR_MASK   (false)
> >> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
> >>     PCIDevice *d = PCI_DEVICE(s);
> >>     int res;
> >> 
> >> -    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
> >> +    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
> >>                    VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> >>     if (0 > res) {
> >>         VMW_WRPRN("Failed to initialize MSI, error %d", res);
> >> -- 
> >> MST
> >> 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest

[Dmitry Fleytman]

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


On Mar 31, 2014, at 18:48 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:

> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> From: Dmitry Fleytman <dmitry@daynix.com>
>> 
>> CVE-2013-4544
>> 
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> hw/net/vmxnet3.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 8c6df05..0fa54e7 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts(VMXNET3State *s)
>>     }
>> }
>> 
>> +static void vmxnet3_validate_queues(VMXNET3State *s)
>> +{
>> +    if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
>> +        hw_error("Bad TX queues number: %d\n", s->txq_num);
>> +    }
>> +
>> +    if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
>> +        hw_error("Bad RX queues number: %d\n", s->rxq_num);
>> +    }
> 
> Why isn't that >= ?
> (I agree it matches the original assert).
> 
>        Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
>        Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
> 
> static void vmxnet3_fill_stats(VMXNET3State *s)
> {
>    int i;
>    for (i = 0; i < s->txq_num; i++) {
>        cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
>                                  &s->txq_descr[i].txq_stats,
>                                  sizeof(s->txq_descr[i].txq_stats));
>    }
> 
> so that looks like it's 0 indexed.
> 
> Dave

HI Dave, thanks for the review.

The verification is ok because s->txq_num and s->rxq_num are total number of queues, not a queue index.

Dmitry.


> 
>> +}
>> +
>> static void vmxnet3_activate_device(VMXNET3State *s)
>> {
>>     int i;
>> @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>>         VMXNET3_READ_DRV_SHARED8(s->drv_shmem, devRead.misc.numRxQueues);
>> 
>>     VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
>> -    assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
>> +    vmxnet3_validate_queues(s);
>> 
>>     qdescr_table_pa =
>>         VMXNET3_READ_DRV_SHARED64(s->drv_shmem, devRead.misc.queueDescPA);
>> -- 
>> MST
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


[-- Attachment #2: Type: text/html, Size: 4798 bytes --]

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

[Peter Maydell]

On 1 April 2014 10:43, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> CVE-2013-4532
>> @@ -374,7 +374,13 @@ 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);
>> -    s->tx_fifo_len = qemu_get_be32(f);
>> +    v = qemu_get_be32(f);
>> +    /* How many bytes does data use in tx fifo. */
>> +    sz = s->tx_frame_len == -1 ? 2 : 4;
>> +    if (v < 0 || v >= ARRAY_SIZE(s->tx_fifo) - sz) {
>> +        return -EINVAL;
>> +    }
>> +    s->tx_fifo_len = v;
>>      qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
>
> I don't understand the magic '2' in that ?2:4 - but there again the 'DATA' write
> case is pretty hairy.

It's not that complicated. The first word of DATA has
the frame length in the bottom 16 bits, and the first
16 bits of actual data. All the subsequent words written
to DATA are part of the actual data.

However, I think this checking code is wrong. We
need to check tx_frame_len as well -- otherwise the
incoming data can simply set a hugely oversize
frame length and then have the guest stuff data
into DATA until we overrun the tx_fifo. As with the
rx fifo, I think the right approach here is to actually
check that the incoming migration data satisfies the
invariants:
 tx_frame_len == -1
OR
 tx_frame_len >= 0 && tx_frame_len <= ARRAY_SIZE(tx_fifo)
  && tx_fifo_len >= 0 && tx_fifo_len <= tx_frame_len - 4

(We don't have to have funny ?2:4 logic because the
case where we only write 2 bytes is when tx_frame_len
is -1, which will also reinitialize tx_fifo_len and
write strictly to the start of the fifo.)

But note that there seems to be a bug or two in
the DATA read logic: our cutoff for tx frame too
long is tx_frame_len > 2032, but for the limit
case of 2032, if we add 14 for the ethernet header
and 4 for explicit CRC then we get 2050, which is
slightly larger than the tx_fifo buffer... I think
we either need to wind that 2032 value down or
enlarge the tx_fifo buffer, or possibly just tweak
the logic not to try to actually write the CRC bytes
into the FIFO since we promptly ignore them -- need
to check the h/w datasheet to find out which.

Also a bug I think is that the PADEN handling code
is setting tx_fifo_len to 60 for short packets, which
will have no effect; I'm pretty sure it should be
setting tx_frame_len instead. That's just wrong
behaviour rather than an overrun issue though.

thanks
-- PMM

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

[Peter Maydell]

On 1 April 2014 10:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> So lets say that tx_frame_len is initially 2032 when written; 14 is added to it
> at this point, and if the CRC flag is set then another 4.   Thus it seems a user
> can set the value in tx_frame_len to 2032+14+4=2050  - which is a bit worrying
> given the buffer is only 2048 bytes.


Yep, see my equivalent remarks in the other patch.

Michael -- can we please squash these two patches into one?
It's really hard to review the code for correctness when
half the logic for dealing with the tx fifo is in a
different patch...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 28/30] vmxnet3: validate interrupt indices read on migration

[Dmitry Fleytman]

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


On Mar 31, 2014, at 22:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Mon, Mar 31, 2014 at 05:33:44PM +0100, Dr. David Alan Gilbert wrote:
>> * Michael S. Tsirkin (mst@redhat.com) wrote:
>>> From: Dmitry Fleytman <dmitry@daynix.com>
>>> 
>>> CVE-2013-4544
>>> 
>>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>> hw/net/vmxnet3.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>> 
>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>> index a4b5c11..8c6df05 100644
>>> --- a/hw/net/vmxnet3.c
>>> +++ b/hw/net/vmxnet3.c
>>> @@ -2374,6 +2374,8 @@ static int vmxnet3_post_load(void *opaque, int version_id)
>>>         }
>>>     }
>>> 
>>> +    vmxnet3_validate_interrupts(s);
>>> +
>> 
>> When you repost to fix the other stuff, why not merge this with what
>> is currently #30.
>> 
>> Dave
> 
> I generally try to avoid making random changes to other's patches,
> unless absolutely necessary.
> Dmitry?

Of course I see no problem for these patches to be merged.
My personal preference however is to keep those separated for cuter history.

> 
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


[-- Attachment #2: Type: text/html, Size: 2802 bytes --]

Re: [Qemu-devel] [PATCH v4 03/30] vmstate: add VMSTATE_VALIDATE

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> Validate state using VMS_ARRAY with num = 0 and VMS_MUST_EXIST
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* 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 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
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

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

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

(Without much understanding of the actual, apparently undocumented hardware)

> To fix, limit to 16 bit.
> 
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@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
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> From: Dmitry Fleytman <dmitry@daynix.com>
> 
> CVE-2013-4544
> 
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 5be807c..a4b5c11 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -52,6 +52,9 @@
>  #define VMXNET3_DEVICE_VERSION    0x1
>  #define VMXNET3_DEVICE_REVISION   0x1
>  
> +/* Number of interrupt vectors for INTx/MSI */
> +#define VMXNET3_MAX_NMSIX_INTRS   (1)

As per Dmitry's reply this is apparently the number of non-MSIX
interrupts; can we change the comment to make this clear.

> +
>  /* Macros for rings descriptors access */
>  #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
>      (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
>             (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
>  }
>  
> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> +{
> +    int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
> +    if (idx >= max_ints) {
> +        hw_error("Bad interrupt index: %d\n", idx);
> +    }

Can we avoid hw_error here, we're using in this in state load, so I'm
OK at the thought of error_report, but then we should make
this return a bool to say it's failed and bubble this up so
that it just fails the post_load test    like any other bad state load.

Dave

> +}
> +
> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
> +{
> +    int i;
> +
> +    VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> +    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> +
> +    for (i = 0; i < s->txq_num; i++) {
> +        int idx = s->txq_descr[i].intr_idx;
> +        VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> +    }
> +
> +    for (i = 0; i < s->rxq_num; i++) {
> +        int idx = s->rxq_descr[i].intr_idx;
> +        VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> +    }
> +}
> +
>  static void vmxnet3_activate_device(VMXNET3State *s)
>  {
>      int i;
> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>                 sizeof(s->rxq_descr[i].rxq_stats));
>      }
>  
> +    vmxnet3_validate_interrupts(s);
> +
>      /* Make sure everything is in place before device activation */
>      smp_wmb();
>  
> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>      }
>  }
>  
> -#define VMXNET3_MSI_NUM_VECTORS   (1)
>  #define VMXNET3_MSI_OFFSET        (0x50)
>  #define VMXNET3_USE_64BIT         (true)
>  #define VMXNET3_PER_VECTOR_MASK   (false)
> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
>      PCIDevice *d = PCI_DEVICE(s);
>      int res;
>  
> -    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
> +    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>                     VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>      if (0 > res) {
>          VMW_WRPRN("Failed to initialize MSI, error %d", res);
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Peter Maydell]

On 1 April 2014 11:05, Peter Maydell <peter.maydell@linaro.org> wrote:
> But note that there seems to be a bug or two in
> the DATA read logic: our cutoff for tx frame too
> long is tx_frame_len > 2032, but for the limit
> case of 2032, if we add 14 for the ethernet header
> and 4 for explicit CRC then we get 2050, which is
> slightly larger than the tx_fifo buffer... I think
> we either need to wind that 2032 value down or
> enlarge the tx_fifo buffer, or possibly just tweak
> the logic not to try to actually write the CRC bytes
> into the FIFO since we promptly ignore them -- need
> to check the h/w datasheet to find out which.

The datasheet isn't entirely clear on this point.
It is fairly definite that the TX overrun check is
done for tx_frame_len written as >2032, but it
doesn't really say what happens if there's not
enough space for the CRC. I rather suspect everybody
is using this device with auto-CRC enabled anyway,
and I have neither the hardware nor the time to
experiment with it. I'll put together a patch that
does something plausible and doesn't allow data
buffer overruns; roughly, we separate the check for
"can we put this word into the FIFO?" from the
one for "should we transmit the packet now?". This
is actually closer to the h/w behaviour, because the
h/w doesn't auto-transmit the packet on a full FIFO
in all cases -- unless you set the transmit-start
threshold you have to explicitly write to a different
register to say "now start transmitting".

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest

[Dmitry Fleytman]

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


On Apr 1, 2014, at 14:33 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:

> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> From: Dmitry Fleytman <dmitry@daynix.com>
>> 
>> CVE-2013-4544
>> 
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
>> 1 file changed, 34 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 5be807c..a4b5c11 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -52,6 +52,9 @@
>> #define VMXNET3_DEVICE_VERSION    0x1
>> #define VMXNET3_DEVICE_REVISION   0x1
>> 
>> +/* Number of interrupt vectors for INTx/MSI */
>> +#define VMXNET3_MAX_NMSIX_INTRS   (1)
> 
> As per Dmitry's reply this is apparently the number of non-MSIX
> interrupts; can we change the comment to make this clear.

Not sure how to change it.
There are three modes of operation:
1. INTx - 1 interrupt is used
2. MSI - 1 interrupt is used
3. MSIx - up to 25 interrupts are used.

This define covers 2 first modes of operation.
Would something like

/* Number of interrupt vectors for non-MSIx modes */

be better?


> 
>> +
>> /* Macros for rings descriptors access */
>> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
>>     (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
>> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
>>            (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
>> }
>> 
>> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
>> +{
>> +    int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
>> +    if (idx >= max_ints) {
>> +        hw_error("Bad interrupt index: %d\n", idx);
>> +    }
> 
> Can we avoid hw_error here, we're using in this in state load, so I'm
> OK at the thought of error_report, but then we should make
> this return a bool to say it's failed and bubble this up so
> that it just fails the post_load test    like any other bad state load.
> 
> Dave
> 
>> +}
>> +
>> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
>> +{
>> +    int i;
>> +
>> +    VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
>> +    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
>> +
>> +    for (i = 0; i < s->txq_num; i++) {
>> +        int idx = s->txq_descr[i].intr_idx;
>> +        VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
>> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>> +    }
>> +
>> +    for (i = 0; i < s->rxq_num; i++) {
>> +        int idx = s->rxq_descr[i].intr_idx;
>> +        VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
>> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>> +    }
>> +}
>> +
>> static void vmxnet3_activate_device(VMXNET3State *s)
>> {
>>     int i;
>> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>>                sizeof(s->rxq_descr[i].rxq_stats));
>>     }
>> 
>> +    vmxnet3_validate_interrupts(s);
>> +
>>     /* Make sure everything is in place before device activation */
>>     smp_wmb();
>> 
>> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>>     }
>> }
>> 
>> -#define VMXNET3_MSI_NUM_VECTORS   (1)
>> #define VMXNET3_MSI_OFFSET        (0x50)
>> #define VMXNET3_USE_64BIT         (true)
>> #define VMXNET3_PER_VECTOR_MASK   (false)
>> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
>>     PCIDevice *d = PCI_DEVICE(s);
>>     int res;
>> 
>> -    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
>> +    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>>                    VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>>     if (0 > res) {
>>         VMW_WRPRN("Failed to initialize MSI, error %d", res);
>> -- 
>> MST
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


[-- Attachment #2: Type: text/html, Size: 6755 bytes --]

Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest

[Dr. David Alan Gilbert]

* Dmitry Fleytman (dmitry@daynix.com) wrote:
> 
> On Apr 1, 2014, at 14:33 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> 
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> >> From: Dmitry Fleytman <dmitry@daynix.com>
> >> 
> >> CVE-2013-4544
> >> 
> >> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> >> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 34 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >> index 5be807c..a4b5c11 100644
> >> --- a/hw/net/vmxnet3.c
> >> +++ b/hw/net/vmxnet3.c
> >> @@ -52,6 +52,9 @@
> >> #define VMXNET3_DEVICE_VERSION    0x1
> >> #define VMXNET3_DEVICE_REVISION   0x1
> >> 
> >> +/* Number of interrupt vectors for INTx/MSI */
> >> +#define VMXNET3_MAX_NMSIX_INTRS   (1)
> > 
> > As per Dmitry's reply this is apparently the number of non-MSIX
> > interrupts; can we change the comment to make this clear.
> 
> Not sure how to change it.
> There are three modes of operation:
> 1. INTx - 1 interrupt is used
> 2. MSI - 1 interrupt is used
> 3. MSIx - up to 25 interrupts are used.
> 
> This define covers 2 first modes of operation.
> Would something like
> 
> /* Number of interrupt vectors for non-MSIx modes */
> 
> be better?

Yes - that's fine.

Dave

> 
> 
> > 
> >> +
> >> /* Macros for rings descriptors access */
> >> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
> >>     (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
> >> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
> >>            (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
> >> }
> >> 
> >> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> >> +{
> >> +    int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
> >> +    if (idx >= max_ints) {
> >> +        hw_error("Bad interrupt index: %d\n", idx);
> >> +    }
> > 
> > Can we avoid hw_error here, we're using in this in state load, so I'm
> > OK at the thought of error_report, but then we should make
> > this return a bool to say it's failed and bubble this up so
> > that it just fails the post_load test    like any other bad state load.
> > 
> > Dave
> > 
> >> +}
> >> +
> >> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
> >> +{
> >> +    int i;
> >> +
> >> +    VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> >> +    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> >> +
> >> +    for (i = 0; i < s->txq_num; i++) {
> >> +        int idx = s->txq_descr[i].intr_idx;
> >> +        VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> >> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> >> +    }
> >> +
> >> +    for (i = 0; i < s->rxq_num; i++) {
> >> +        int idx = s->rxq_descr[i].intr_idx;
> >> +        VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> >> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> >> +    }
> >> +}
> >> +
> >> static void vmxnet3_activate_device(VMXNET3State *s)
> >> {
> >>     int i;
> >> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> >>                sizeof(s->rxq_descr[i].rxq_stats));
> >>     }
> >> 
> >> +    vmxnet3_validate_interrupts(s);
> >> +
> >>     /* Make sure everything is in place before device activation */
> >>     smp_wmb();
> >> 
> >> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
> >>     }
> >> }
> >> 
> >> -#define VMXNET3_MSI_NUM_VECTORS   (1)
> >> #define VMXNET3_MSI_OFFSET        (0x50)
> >> #define VMXNET3_USE_64BIT         (true)
> >> #define VMXNET3_PER_VECTOR_MASK   (false)
> >> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
> >>     PCIDevice *d = PCI_DEVICE(s);
> >>     int res;
> >> 
> >> -    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
> >> +    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
> >>                    VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> >>     if (0 > res) {
> >>         VMW_WRPRN("Failed to initialize MSI, error %d", res);
> >> -- 
> >> MST
> >> 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Eric Blake]

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

On 03/31/2014 08:16 AM, Michael S. Tsirkin wrote:
> CVE-2013-4532

s/orerrun/overrun/ in the subject

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest

[Michael S. Tsirkin]

On Tue, Apr 01, 2014 at 01:04:12PM +0300, Dmitry Fleytman wrote:
> 
> On Mar 31, 2014, at 18:48 PM, Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
> 
> 
>     * Michael S. Tsirkin (mst@redhat.com) wrote:
> 
>         From: Dmitry Fleytman <dmitry@daynix.com>
> 
>         CVE-2013-4544
> 
>         Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>         Reported-by: Michael S. Tsirkin <mst@redhat.com>
>         Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>         ---
>         hw/net/vmxnet3.c | 13 ++++++++++++-
>         1 file changed, 12 insertions(+), 1 deletion(-)
> 
>         diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>         index 8c6df05..0fa54e7 100644
>         --- a/hw/net/vmxnet3.c
>         +++ b/hw/net/vmxnet3.c
>         @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts
>         (VMXNET3State *s)
>             }
>         }
> 
>         +static void vmxnet3_validate_queues(VMXNET3State *s)
>         +{
>         +    if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
>         +        hw_error("Bad TX queues number: %d\n", s->txq_num);
>         +    }
>         +
>         +    if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
>         +        hw_error("Bad RX queues number: %d\n", s->rxq_num);
>         +    }
> 
> 
>     Why isn't that >= ?
>     (I agree it matches the original assert).
> 
>            Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
>            Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
> 
>     static void vmxnet3_fill_stats(VMXNET3State *s)
>     {
>        int i;
>        for (i = 0; i < s->txq_num; i++) {
>            cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
>                                      &s->txq_descr[i].txq_stats,
>                                      sizeof(s->txq_descr[i].txq_stats));
>        }
> 
>     so that looks like it's 0 indexed.
> 
>     Dave
> 
> 
> HI Dave, thanks for the review.
> 
> The verification is ok because s->txq_num and s->rxq_num are total number of
> queues, not a queue index.
> 
> Dmitry.

A code comment here might be helpful.

> 
> 
> 
> 
>         +}
>         +
>         static void vmxnet3_activate_device(VMXNET3State *s)
>         {
>             int i;
>         @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State
>         *s)
>                 VMXNET3_READ_DRV_SHARED8(s->drv_shmem,
>         devRead.misc.numRxQueues);
> 
>             VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
>         -    assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
>         +    vmxnet3_validate_queues(s);
> 
>             qdescr_table_pa =
>                 VMXNET3_READ_DRV_SHARED64(s->drv_shmem,
>         devRead.misc.queueDescPA);
>         -- 
>         MST
> 
> 
>     --
>     Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 

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

[Michael S. Tsirkin]

On Mon, Mar 31, 2014 at 04:40:41PM +0100, Peter Maydell wrote:
> On 31 March 2014 15:16, 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.
> 
> This fixes the bug, but it's rather unintuitive semantics
> for an INT32_LE not to simply do a signed comparison...
> Maybe rename the macro?
> 
> thanks
> -- PMM

Sure. I'll do it in a separate patch though.

Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration

[Michael S. Tsirkin]

On Mon, Mar 31, 2014 at 10:13:04PM +0100, Peter Maydell wrote:
> On 31 March 2014 21:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Mar 31, 2014 at 06:11:22PM +0100, Dr. David Alan Gilbert wrote:
> >> * Michael S. Tsirkin (mst@redhat.com) wrote:
> >> > 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 failing migration if s->next_packet we read from
> >> > the wire exceeds this.
> >> >
> >> > Similarly, validate rx_fifo against sizeof(s->rx[].data).
> >> >
> >> > Finally, constrain rx len to a sensibly small positive
> >> > value, to avoid integer overruns when data model
> >> > later uses this value.
> >> >
> >> > Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> > ---
> >> >  hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
> >> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> >> > index d04e6a4..182fd3e 100644
> >> > --- a/hw/net/stellaris_enet.c
> >> > +++ b/hw/net/stellaris_enet.c
> >> > @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
> >> >  static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> >> >  {
> >> >      stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> >> > -    int i;
> >> > +    int i, v;
> >> >
> >> >      if (version_id != 1)
> >> >          return -EINVAL;
> >> > @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> >> >          qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> >> >
> >> >      }
> >>
> >> The loop that's just off the top here is:
> >>     for (i = 0; i < 31; i++) {
> >>         s->rx[i].len = qemu_get_be32(f);
> >>         qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> >>
> >>     }
> >>
> >> Doesn't that 'len' need validating? I assume it's the size of the
> >> packet in the fixed sized buffer? (??)
> >
> > Not that I see where it's used as such.
> 
> In the DATA case of stellaris_enet_read() -- when the current
> rx_fifo_len goes to zero we will uncritically set rx_fifo_len to
> s->rx[s->next_packet].len. So we must validate that it's between
> 0 and 2048 (the size of the rx[].data array), otherwise further
> reads from DATA will be able to run off the end of the data array
> for the following packet.
> 
> >> > -    s->next_packet = qemu_get_be32(f);
> >> > -    s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
> >> > -    s->rx_fifo_len = qemu_get_be32(f);
> >> > +    v = qemu_get_be32(f);
> >> > +    if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
> >> > +        return -EINVAL;
> >> > +    }
> >> > +    s->next_packet = v;
> >> > +    v = qemu_get_be32(f);
> >> > +    if (v < 0 || v >= sizeof(s->rx[i].data)) {
> >> > +        return -EINVAL;
> >> > +    }
> >> > +    s->rx_fifo = s->rx[s->next_packet].data + v;
> >> > +    v = qemu_get_be32(f);
> >> > +    /* Set limit low enough to avoid integer overflow when
> >> > +     * we do math on len later, but high enough to avoid
> >> > +     * truncating any packets.
> >> > +     */
> >> > +    if (v < 0 || v >= 0x100000) {
> >> > +        return -EINVAL;
> >> > +    }
> >> > +    s->rx_fifo_len = v;
> >>
> >> I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be within
> >> the buffer (or I think it might be legal for the sum to point to the byte after the
> >> buffer)?
> >> My (quick first ever look at this code) reading is that rx_fifo and rx_fifo_len
> >> related to the current packet in-flight; although I've not quite convinced myself
> >> about what is supposed to happen at the end of the packet (which is why
> >> I say rx_fifo might point just at? the end.
> 
> > Actually I forgot why I wrote this last check.
> > Peter said we should and I thought I see the issue ...
> > But I no longer see what kind of damage can rx_fifo_len cause
> > unless validated.
> 
> Again, look at the DATA read logic. Every time the guest does a
> DATA read, we read from the four bytes at s->rx_fifo, increment
> rx_fifo by 4 and decrement rx_fifo_len by 4. When rx_fifo_len
> eventually goes to zero we will (on the subsequent read) reset
> both rx_fifo and rx_fifo_len from the next packet in the rx queue.
> So if the incoming data sets rx_fifo_len to (let us say) 0x10000,
> then the guest can cause us to read well off the end of the rx data
> array. This means your check isn't tight enough -- we need to
> ensure that rx_fifo and rx_fifo_len between them define a window
> into the rx data and nowhere else. As David says this means you
> need:
> 
>     v1 = qemu_get_be32(f);
>     if (v1 < 0 || v1 > sizeof(s->rx[i].data)) {
>         return -EINVAL;
>     }
>     s->rx_fifo = s->rx[s->next_packet].data + v1;
>     v2 = qemu_get_be32(f);
>     if (v2 < 0 || v1 + v2 > sizeof(s->rx[i].data)) {
>         return -EINVAL;
>     }
>     s->rx_fifo_len = v2;
> 
> The max check on v1 is actually only there to ensure that we
> don't have to think about integer overflow when we do the
> upper-bound check on v1 + v2. Note that v1 == sizeof(array)
> is OK if (and only if) v2 == 0.
> 
> An assert in stellaris_enet_receive() that the net code never
> hands us a packet we can't fit in the array wouldn't go amiss
> either, but that's a separate issue.
> 
> thanks
> -- PMM


Got it now, thanks a lot for the explanation.

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

[Michael S. Tsirkin]

On Tue, Apr 01, 2014 at 11:06:48AM +0100, Peter Maydell wrote:
> On 1 April 2014 10:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > So lets say that tx_frame_len is initially 2032 when written; 14 is added to it
> > at this point, and if the CRC flag is set then another 4.   Thus it seems a user
> > can set the value in tx_frame_len to 2032+14+4=2050  - which is a bit worrying
> > given the buffer is only 2048 bytes.
> 
> 
> Yep, see my equivalent remarks in the other patch.
> 
> Michael -- can we please squash these two patches into one?
> It's really hard to review the code for correctness when
> half the logic for dealing with the tx fifo is in a
> different patch...
> 
> thanks
> -- PMM

Will do - part 1 as well?

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

[Peter Maydell]

On 1 April 2014 16:22, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Apr 01, 2014 at 11:06:48AM +0100, Peter Maydell wrote:
>> Michael -- can we please squash these two patches into one?
>> It's really hard to review the code for correctness when
>> half the logic for dealing with the tx fifo is in a
>> different patch...

> Will do - part 1 as well?

No, that's OK, it's dealing with the rx logic rather than
the tx logic.

However, I've been writing some patches this afternoon
which get rid of tx_frame_len completely, so you might
want to wait for those...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest

[Dmitry Fleytman]

> On Apr 1, 2014, at 5:52 PM, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> On Tue, Apr 01, 2014 at 01:04:12PM +0300, Dmitry Fleytman wrote:
>> 
>> On Mar 31, 2014, at 18:48 PM, Dr. David Alan Gilbert <dgilbert@redhat.com>
>> wrote:
>> 
>> 
>>    * Michael S. Tsirkin (mst@redhat.com) wrote:
>> 
>>        From: Dmitry Fleytman <dmitry@daynix.com>
>> 
>>        CVE-2013-4544
>> 
>>        Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>>        Reported-by: Michael S. Tsirkin <mst@redhat.com>
>>        Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>        ---
>>        hw/net/vmxnet3.c | 13 ++++++++++++-
>>        1 file changed, 12 insertions(+), 1 deletion(-)
>> 
>>        diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>        index 8c6df05..0fa54e7 100644
>>        --- a/hw/net/vmxnet3.c
>>        +++ b/hw/net/vmxnet3.c
>>        @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts
>>        (VMXNET3State *s)
>>            }
>>        }
>> 
>>        +static void vmxnet3_validate_queues(VMXNET3State *s)
>>        +{
>>        +    if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
>>        +        hw_error("Bad TX queues number: %d\n", s->txq_num);
>>        +    }
>>        +
>>        +    if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
>>        +        hw_error("Bad RX queues number: %d\n", s->rxq_num);
>>        +    }
>> 
>> 
>>    Why isn't that >= ?
>>    (I agree it matches the original assert).
>> 
>>           Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
>>           Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
>> 
>>    static void vmxnet3_fill_stats(VMXNET3State *s)
>>    {
>>       int i;
>>       for (i = 0; i < s->txq_num; i++) {
>>           cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
>>                                     &s->txq_descr[i].txq_stats,
>>                                     sizeof(s->txq_descr[i].txq_stats));
>>       }
>> 
>>    so that looks like it's 0 indexed.
>> 
>>    Dave
>> 
>> 
>> HI Dave, thanks for the review.
>> 
>> The verification is ok because s->txq_num and s->rxq_num are total number of
>> queues, not a queue index.
>> 
>> Dmitry.
> 
> A code comment here might be helpful.

No problem. I'll resend these two patches with fixed comments.

> 
>> 
>> 
>> 
>> 
>>        +}
>>        +
>>        static void vmxnet3_activate_device(VMXNET3State *s)
>>        {
>>            int i;
>>        @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State
>>        *s)
>>                VMXNET3_READ_DRV_SHARED8(s->drv_shmem,
>>        devRead.misc.numRxQueues);
>> 
>>            VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
>>        -    assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
>>        +    vmxnet3_validate_queues(s);
>> 
>>            qdescr_table_pa =
>>                VMXNET3_READ_DRV_SHARED64(s->drv_shmem,
>>        devRead.misc.queueDescPA);
>>        -- 
>>        MST
>> 
>> 
>>    --
>>    Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>> 
>> 

Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest

[Michael S. Tsirkin]

On Tue, Apr 01, 2014 at 02:07:52PM +0100, Dr. David Alan Gilbert wrote:
> * Dmitry Fleytman (dmitry@daynix.com) wrote:
> > 
> > On Apr 1, 2014, at 14:33 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > 
> > > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > >> From: Dmitry Fleytman <dmitry@daynix.com>
> > >> 
> > >> CVE-2013-4544
> > >> 
> > >> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> > >> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >> ---
> > >> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
> > >> 1 file changed, 34 insertions(+), 2 deletions(-)
> > >> 
> > >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> > >> index 5be807c..a4b5c11 100644
> > >> --- a/hw/net/vmxnet3.c
> > >> +++ b/hw/net/vmxnet3.c
> > >> @@ -52,6 +52,9 @@
> > >> #define VMXNET3_DEVICE_VERSION    0x1
> > >> #define VMXNET3_DEVICE_REVISION   0x1
> > >> 
> > >> +/* Number of interrupt vectors for INTx/MSI */
> > >> +#define VMXNET3_MAX_NMSIX_INTRS   (1)
> > > 
> > > As per Dmitry's reply this is apparently the number of non-MSIX
> > > interrupts; can we change the comment to make this clear.
> > 
> > Not sure how to change it.
> > There are three modes of operation:
> > 1. INTx - 1 interrupt is used
> > 2. MSI - 1 interrupt is used
> > 3. MSIx - up to 25 interrupts are used.
> > 
> > This define covers 2 first modes of operation.
> > Would something like
> > 
> > /* Number of interrupt vectors for non-MSIx modes */
> > 
> > be better?
> 
> Yes - that's fine.
> 
> Dave


OK Dmitry, can you please resend patches with suggested comment changes?

> > 
> > 
> > > 
> > >> +
> > >> /* Macros for rings descriptors access */
> > >> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
> > >>     (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
> > >> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
> > >>            (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
> > >> }
> > >> 
> > >> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> > >> +{
> > >> +    int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
> > >> +    if (idx >= max_ints) {
> > >> +        hw_error("Bad interrupt index: %d\n", idx);
> > >> +    }
> > > 
> > > Can we avoid hw_error here, we're using in this in state load, so I'm
> > > OK at the thought of error_report, but then we should make
> > > this return a bool to say it's failed and bubble this up so
> > > that it just fails the post_load test    like any other bad state load.
> > > 
> > > Dave
> > > 
> > >> +}
> > >> +
> > >> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
> > >> +{
> > >> +    int i;
> > >> +
> > >> +    VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> > >> +    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> > >> +
> > >> +    for (i = 0; i < s->txq_num; i++) {
> > >> +        int idx = s->txq_descr[i].intr_idx;
> > >> +        VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> > >> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> > >> +    }
> > >> +
> > >> +    for (i = 0; i < s->rxq_num; i++) {
> > >> +        int idx = s->rxq_descr[i].intr_idx;
> > >> +        VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> > >> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> > >> +    }
> > >> +}
> > >> +
> > >> static void vmxnet3_activate_device(VMXNET3State *s)
> > >> {
> > >>     int i;
> > >> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
> > >>                sizeof(s->rxq_descr[i].rxq_stats));
> > >>     }
> > >> 
> > >> +    vmxnet3_validate_interrupts(s);
> > >> +
> > >>     /* Make sure everything is in place before device activation */
> > >>     smp_wmb();
> > >> 
> > >> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
> > >>     }
> > >> }
> > >> 
> > >> -#define VMXNET3_MSI_NUM_VECTORS   (1)
> > >> #define VMXNET3_MSI_OFFSET        (0x50)
> > >> #define VMXNET3_USE_64BIT         (true)
> > >> #define VMXNET3_PER_VECTOR_MASK   (false)
> > >> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
> > >>     PCIDevice *d = PCI_DEVICE(s);
> > >>     int res;
> > >> 
> > >> -    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
> > >> +    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
> > >>                    VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
> > >>     if (0 > res) {
> > >>         VMW_WRPRN("Failed to initialize MSI, error %d", res);
> > >> -- 
> > >> MST
> > >> 
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest

[Dmitry Fleytman]

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


On Apr 3, 2014, at 19:07 PM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Tue, Apr 01, 2014 at 02:07:52PM +0100, Dr. David Alan Gilbert wrote:
>> * Dmitry Fleytman (dmitry@daynix.com) wrote:
>>> 
>>> On Apr 1, 2014, at 14:33 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>>> 
>>>> * Michael S. Tsirkin (mst@redhat.com) wrote:
>>>>> From: Dmitry Fleytman <dmitry@daynix.com>
>>>>> 
>>>>> CVE-2013-4544
>>>>> 
>>>>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>>>>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 34 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>>>> index 5be807c..a4b5c11 100644
>>>>> --- a/hw/net/vmxnet3.c
>>>>> +++ b/hw/net/vmxnet3.c
>>>>> @@ -52,6 +52,9 @@
>>>>> #define VMXNET3_DEVICE_VERSION    0x1
>>>>> #define VMXNET3_DEVICE_REVISION   0x1
>>>>> 
>>>>> +/* Number of interrupt vectors for INTx/MSI */
>>>>> +#define VMXNET3_MAX_NMSIX_INTRS   (1)
>>>> 
>>>> As per Dmitry's reply this is apparently the number of non-MSIX
>>>> interrupts; can we change the comment to make this clear.
>>> 
>>> Not sure how to change it.
>>> There are three modes of operation:
>>> 1. INTx - 1 interrupt is used
>>> 2. MSI - 1 interrupt is used
>>> 3. MSIx - up to 25 interrupts are used.
>>> 
>>> This define covers 2 first modes of operation.
>>> Would something like
>>> 
>>> /* Number of interrupt vectors for non-MSIx modes */
>>> 
>>> be better?
>> 
>> Yes - that's fine.
>> 
>> Dave
> 
> 
> OK Dmitry, can you please resend patches with suggested comment changes?

Done. Sent to the list.

> 
>>> 
>>> 
>>>> 
>>>>> +
>>>>> /* Macros for rings descriptors access */
>>>>> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \
>>>>>    (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field)))
>>>>> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
>>>>>           (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
>>>>> }
>>>>> 
>>>>> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
>>>>> +{
>>>>> +    int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
>>>>> +    if (idx >= max_ints) {
>>>>> +        hw_error("Bad interrupt index: %d\n", idx);
>>>>> +    }
>>>> 
>>>> Can we avoid hw_error here, we're using in this in state load, so I'm
>>>> OK at the thought of error_report, but then we should make
>>>> this return a bool to say it's failed and bubble this up so
>>>> that it just fails the post_load test    like any other bad state load.
>>>> 
>>>> Dave
>>>> 
>>>>> +}
>>>>> +
>>>>> +static void vmxnet3_validate_interrupts(VMXNET3State *s)
>>>>> +{
>>>>> +    int i;
>>>>> +
>>>>> +    VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
>>>>> +    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
>>>>> +
>>>>> +    for (i = 0; i < s->txq_num; i++) {
>>>>> +        int idx = s->txq_descr[i].intr_idx;
>>>>> +        VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
>>>>> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>>>>> +    }
>>>>> +
>>>>> +    for (i = 0; i < s->rxq_num; i++) {
>>>>> +        int idx = s->rxq_descr[i].intr_idx;
>>>>> +        VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
>>>>> +        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> static void vmxnet3_activate_device(VMXNET3State *s)
>>>>> {
>>>>>    int i;
>>>>> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>>>>>               sizeof(s->rxq_descr[i].rxq_stats));
>>>>>    }
>>>>> 
>>>>> +    vmxnet3_validate_interrupts(s);
>>>>> +
>>>>>    /* Make sure everything is in place before device activation */
>>>>>    smp_wmb();
>>>>> 
>>>>> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>>>>>    }
>>>>> }
>>>>> 
>>>>> -#define VMXNET3_MSI_NUM_VECTORS   (1)
>>>>> #define VMXNET3_MSI_OFFSET        (0x50)
>>>>> #define VMXNET3_USE_64BIT         (true)
>>>>> #define VMXNET3_PER_VECTOR_MASK   (false)
>>>>> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s)
>>>>>    PCIDevice *d = PCI_DEVICE(s);
>>>>>    int res;
>>>>> 
>>>>> -    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS,
>>>>> +    res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
>>>>>                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>>>>>    if (0 > res) {
>>>>>        VMW_WRPRN("Failed to initialize MSI, error %d", res);
>>>>> -- 
>>>>> MST
>>>>> 
>>>> --
>>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>> 
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


[-- Attachment #2: Type: text/html, Size: 7267 bytes --]