[Qemu-devel] [PATCH v4 0/9] virtio: avoid exit() when device enters invalid states

[Qemu-devel] [PATCH v4 0/9] virtio: avoid exit() when device enters invalid states

[Stefan Hajnoczi]

v4:
 * Rebase to qemu.git/master
 * Use "unsigned int" instead of "unsigned" in virtqueue_undo_map_desc() [Cornelia]

v3:
 * Patch 1: Fix typo and clarify commit description [Markus]
 * Use virtio_set_status() instead of open coding assignment [Cornelia]
 * Add live migration

v2:
 * Add VIRTIO_CONFIG_S_NEEDS_RESET notification for VIRTIO 1.0 [Cornelia]
   (Note I've sent a Linux virtio_config.h patch to get the constant added to
   the headers.)
 * Split int -> unsigned int change into separate commit [Fam]
 * Fix double "index" typo in commit description [Fam]

The virtio code calls exit() when the device enters an invalid state.  This
means invalid vring indices and descriptor chains kill the VM.  See the patch
descriptions for why this is a bad thing.

When the virtio device is in the broken state calls to virtqueue_pop() and
friends will pretend the virtqueue is empty.  This means the device will become
isolated from guest activity until it is reset again.

Stefan Hajnoczi (9):
  virtio: fix stray tab character
  virtio: stop virtqueue processing if device is broken
  virtio: migrate vdev->broken flag
  virtio: handle virtqueue_map_desc() errors
  virtio: handle virtqueue_get_avail_bytes() errors
  virtio: use unsigned int for virtqueue_get_avail_bytes() index
  virtio: handle virtqueue_read_next_desc() errors
  virtio: handle virtqueue_num_heads() errors
  virtio: handle virtqueue_get_head() errors

 hw/virtio/virtio.c         | 232 +++++++++++++++++++++++++++++++++++----------
 include/hw/virtio/virtio.h |   3 +
 2 files changed, 184 insertions(+), 51 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v4 1/9] virtio: fix stray tab character

[Stefan Hajnoczi]

Fix a single occurrence of a tab character in a file that otherwise uses
spaces for indentation.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v3:
 * Fix typo and clarify commit description [Markus]
---
 hw/virtio/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index fcf3358..9e25a91 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1608,7 +1608,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
                          "inconsistent with Host index 0x%x",
                          i, vdev->vq[i].last_avail_idx);
                 return -1;
-	}
+        }
         if (k->load_queue) {
             ret = k->load_queue(qbus->parent, i, f);
             if (ret)
-- 
2.7.4

[Qemu-devel] [PATCH v4 2/9] virtio: stop virtqueue processing if device is broken

[Stefan Hajnoczi]

QEMU prints an error message and exits when the device enters an invalid
state.  Terminating the process is heavy-handed.  The guest may still be
able to function even if there is a bug in a virtio guest driver.

Moreover, exiting is a bug in nested virtualization where a nested guest
could DoS other nested guests by killing a pass-through virtio device.
I don't think this configuration is possible today but it is likely in
the future.

If the broken flag is set, do not process virtqueues or write back used
descriptors.  The broken flag can be cleared again by resetting the
device.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c         | 39 +++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h |  3 +++
 2 files changed, 42 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9e25a91..448f789 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -303,6 +303,10 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 
     virtqueue_unmap_sg(vq, elem, len);
 
+    if (unlikely(vq->vdev->broken)) {
+        return;
+    }
+
     idx = (idx + vq->used_idx) % vq->vring.num;
 
     uelem.id = elem->index;
@@ -313,6 +317,12 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 void virtqueue_flush(VirtQueue *vq, unsigned int count)
 {
     uint16_t old, new;
+
+    if (unlikely(vq->vdev->broken)) {
+        vq->inuse -= count;
+        return;
+    }
+
     /* Make sure buffer is written before we update index. */
     smp_wmb();
     trace_virtqueue_flush(vq, count);
@@ -578,6 +588,9 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     struct iovec iov[VIRTQUEUE_MAX_SIZE];
     VRingDesc desc;
 
+    if (unlikely(vdev->broken)) {
+        return NULL;
+    }
     if (virtio_queue_empty(vq)) {
         return NULL;
     }
@@ -742,6 +755,10 @@ static void virtio_notify_vector(VirtIODevice *vdev, uint16_t vector)
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
 
+    if (unlikely(vdev->broken)) {
+        return;
+    }
+
     if (k->notify) {
         k->notify(qbus->parent, vector);
     }
@@ -825,6 +842,7 @@ void virtio_reset(void *opaque)
         k->reset(vdev);
     }
 
+    vdev->broken = false;
     vdev->guest_features = 0;
     vdev->queue_sel = 0;
     vdev->status = 0;
@@ -1132,6 +1150,10 @@ static void virtio_queue_notify_vq(VirtQueue *vq)
     if (vq->vring.desc && vq->handle_output) {
         VirtIODevice *vdev = vq->vdev;
 
+        if (unlikely(vdev->broken)) {
+            return;
+        }
+
         trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
         vq->handle_output(vdev, vq);
     }
@@ -1753,6 +1775,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
     vdev->config_vector = VIRTIO_NO_VECTOR;
     vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_QUEUE_MAX);
     vdev->vm_running = runstate_is_running();
+    vdev->broken = false;
     for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
         vdev->vq[i].vector = VIRTIO_NO_VECTOR;
         vdev->vq[i].vdev = vdev;
@@ -1939,6 +1962,22 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
     vdev->bus_name = g_strdup(bus_name);
 }
 
+void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_vreport(fmt, ap);
+    va_end(ap);
+
+    vdev->broken = true;
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+        virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
+        virtio_notify_config(vdev);
+    }
+}
+
 static void virtio_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f05559d..888c8de 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -87,6 +87,7 @@ struct VirtIODevice
     VirtQueue *vq;
     uint16_t device_id;
     bool vm_running;
+    bool broken; /* device in invalid state, needs reset */
     VMChangeStateEntry *vmstate;
     char *bus_name;
     uint8_t device_endian;
@@ -135,6 +136,8 @@ void virtio_init(VirtIODevice *vdev, const char *name,
                          uint16_t device_id, size_t config_size);
 void virtio_cleanup(VirtIODevice *vdev);
 
+void virtio_error(VirtIODevice *vdev, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+
 /* Set the child bus name. */
 void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
 
-- 
2.7.4

[Qemu-devel] [PATCH v4 3/9] virtio: migrate vdev->broken flag

[Stefan Hajnoczi]

Send a subsection if the vdev->broken flag is set.  This allows live
migration of broken virtio devices.

The subsection is only sent if vdev->broken has been set.  In most cases
the flag will be clear and no subsection will be sent.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 448f789..a841640 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1338,6 +1338,13 @@ static bool virtio_extra_state_needed(void *opaque)
         k->has_extra_state(qbus->parent);
 }
 
+static bool virtio_broken_needed(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+
+    return vdev->broken;
+}
+
 static const VMStateDescription vmstate_virtqueue = {
     .name = "virtqueue_state",
     .version_id = 1,
@@ -1452,6 +1459,17 @@ static const VMStateDescription vmstate_virtio_64bit_features = {
     }
 };
 
+static const VMStateDescription vmstate_virtio_broken = {
+    .name = "virtio/broken",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = &virtio_broken_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(broken, VirtIODevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio = {
     .name = "virtio",
     .version_id = 1,
@@ -1465,6 +1483,7 @@ static const VMStateDescription vmstate_virtio = {
         &vmstate_virtio_64bit_features,
         &vmstate_virtio_virtqueues,
         &vmstate_virtio_ringsize,
+        &vmstate_virtio_broken,
         &vmstate_virtio_extra_state,
         NULL
     }
-- 
2.7.4

[Qemu-devel] [PATCH v4 4/9] virtio: handle virtqueue_map_desc() errors

[Stefan Hajnoczi]

Errors can occur during virtqueue_pop(), especially in
virtqueue_map_desc().  In order to handle this we must unmap iov[]
before returning NULL.  The caller will consider the virtqueue empty and
the virtio_error() call will have marked the device broken.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 70 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 53 insertions(+), 17 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a841640..c499028 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -484,24 +484,27 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
     return in_bytes <= in_total && out_bytes <= out_total;
 }
 
-static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iovec *iov,
+static bool virtqueue_map_desc(VirtIODevice *vdev, unsigned int *p_num_sg,
+                               hwaddr *addr, struct iovec *iov,
                                unsigned int max_num_sg, bool is_write,
                                hwaddr pa, size_t sz)
 {
+    bool ok = false;
     unsigned num_sg = *p_num_sg;
     assert(num_sg <= max_num_sg);
 
     if (!sz) {
-        error_report("virtio: zero sized buffers are not allowed");
-        exit(1);
+        virtio_error(vdev, "virtio: zero sized buffers are not allowed");
+        goto out;
     }
 
     while (sz) {
         hwaddr len = sz;
 
         if (num_sg == max_num_sg) {
-            error_report("virtio: too many write descriptors in indirect table");
-            exit(1);
+            virtio_error(vdev, "virtio: too many write descriptors in "
+                               "indirect table");
+            goto out;
         }
 
         iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write);
@@ -512,7 +515,28 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
         pa += len;
         num_sg++;
     }
+    ok = true;
+
+out:
     *p_num_sg = num_sg;
+    return ok;
+}
+
+/* Only used by error code paths before we have a VirtQueueElement (therefore
+ * virtqueue_unmap_sg() can't be used).  Assumes buffers weren't written to
+ * yet.
+ */
+static void virtqueue_undo_map_desc(unsigned out_num, unsigned in_num,
+                                    struct iovec *iov)
+{
+    unsigned int i;
+
+    for (i = 0; i < out_num + in_num; i++) {
+        int is_write = i >= out_num;
+
+        cpu_physical_memory_unmap(iov->iov_base, iov->iov_len, is_write, 0);
+        iov++;
+    }
 }
 
 static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
@@ -604,8 +628,8 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     max = vq->vring.num;
 
     if (vq->inuse >= vq->vring.num) {
-        error_report("Virtqueue size exceeded");
-        exit(1);
+        virtio_error(vdev, "Virtqueue size exceeded");
+        return NULL;
     }
 
     i = head = virtqueue_get_head(vq, vq->last_avail_idx++);
@@ -616,8 +640,8 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     vring_desc_read(vdev, &desc, desc_pa, i);
     if (desc.flags & VRING_DESC_F_INDIRECT) {
         if (desc.len % sizeof(VRingDesc)) {
-            error_report("Invalid size for indirect buffer table");
-            exit(1);
+            virtio_error(vdev, "Invalid size for indirect buffer table");
+            return NULL;
         }
 
         /* loop over the indirect descriptor table */
@@ -629,22 +653,30 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 
     /* Collect all the descriptors */
     do {
+        bool map_ok;
+
         if (desc.flags & VRING_DESC_F_WRITE) {
-            virtqueue_map_desc(&in_num, addr + out_num, iov + out_num,
-                               VIRTQUEUE_MAX_SIZE - out_num, true, desc.addr, desc.len);
+            map_ok = virtqueue_map_desc(vdev, &in_num, addr + out_num,
+                                        iov + out_num,
+                                        VIRTQUEUE_MAX_SIZE - out_num, true,
+                                        desc.addr, desc.len);
         } else {
             if (in_num) {
-                error_report("Incorrect order for descriptors");
-                exit(1);
+                virtio_error(vdev, "Incorrect order for descriptors");
+                goto err_undo_map;
             }
-            virtqueue_map_desc(&out_num, addr, iov,
-                               VIRTQUEUE_MAX_SIZE, false, desc.addr, desc.len);
+            map_ok = virtqueue_map_desc(vdev, &out_num, addr, iov,
+                                        VIRTQUEUE_MAX_SIZE, false,
+                                        desc.addr, desc.len);
+        }
+        if (!map_ok) {
+            goto err_undo_map;
         }
 
         /* If we've got too many, that implies a descriptor loop. */
         if ((in_num + out_num) > max) {
-            error_report("Looped descriptor");
-            exit(1);
+            virtio_error(vdev, "Looped descriptor");
+            goto err_undo_map;
         }
     } while ((i = virtqueue_read_next_desc(vdev, &desc, desc_pa, max)) != max);
 
@@ -664,6 +696,10 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 
     trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
     return elem;
+
+err_undo_map:
+    virtqueue_undo_map_desc(out_num, in_num, iov);
+    return NULL;
 }
 
 /* Reading and writing a structure directly to QEMUFile is *awful*, but
-- 
2.7.4

[Qemu-devel] [PATCH v4 5/9] virtio: handle virtqueue_get_avail_bytes() errors

[Stefan Hajnoczi]

If the vring is invalid, tell the caller no bytes are available and mark
the device broken.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index c499028..f378f9c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -426,14 +426,14 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 
         if (desc.flags & VRING_DESC_F_INDIRECT) {
             if (desc.len % sizeof(VRingDesc)) {
-                error_report("Invalid size for indirect buffer table");
-                exit(1);
+                virtio_error(vdev, "Invalid size for indirect buffer table");
+                goto err;
             }
 
             /* If we've got too many, that implies a descriptor loop. */
             if (num_bufs >= max) {
-                error_report("Looped descriptor");
-                exit(1);
+                virtio_error(vdev, "Looped descriptor");
+                goto err;
             }
 
             /* loop over the indirect descriptor table */
@@ -447,8 +447,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
         do {
             /* If we've got too many, that implies a descriptor loop. */
             if (++num_bufs > max) {
-                error_report("Looped descriptor");
-                exit(1);
+                virtio_error(vdev, "Looped descriptor");
+                goto err;
             }
 
             if (desc.flags & VRING_DESC_F_WRITE) {
@@ -473,6 +473,11 @@ done:
     if (out_bytes) {
         *out_bytes = out_total;
     }
+    return;
+
+err:
+    in_total = out_total = 0;
+    goto done;
 }
 
 int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
-- 
2.7.4

[Qemu-devel] [PATCH v4 6/9] virtio: use unsigned int for virtqueue_get_avail_bytes() index

[Stefan Hajnoczi]

The virtio code uses int, unsigned int, and uint16_t for virtqueue
indices.  The uint16_t is used for the low-level descriptor layout in
virtio_ring.h while code that isn't concerned with descriptor layout can
use unsigned int.

Use of int is problematic because it can result in signed/unsigned
comparison and incompatible int*/unsigned int* pointer types.

Make the virtqueue_get_avail_bytes() 'i' variable unsigned int.  This
eliminates the need to introduce casts and modify code further in the
patches that follow.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f378f9c..44d013e 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -416,7 +416,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
         unsigned int max, num_bufs, indirect = 0;
         VRingDesc desc;
         hwaddr desc_pa;
-        int i;
+        unsigned int i;
 
         max = vq->vring.num;
         num_bufs = total_bufs;
-- 
2.7.4

[Qemu-devel] [PATCH v4 7/9] virtio: handle virtqueue_read_next_desc() errors

[Stefan Hajnoczi]

Stop processing the vring if an avail ring index is invalid.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 44d013e..4d25af4 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -377,28 +377,33 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
     return head;
 }
 
-static unsigned virtqueue_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
-                                         hwaddr desc_pa, unsigned int max)
-{
-    unsigned int next;
+enum {
+    VIRTQUEUE_READ_DESC_ERROR = -1,
+    VIRTQUEUE_READ_DESC_DONE = 0,   /* end of chain */
+    VIRTQUEUE_READ_DESC_MORE = 1,   /* more buffers in chain */
+};
 
+static int virtqueue_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
+                                    hwaddr desc_pa, unsigned int max,
+                                    unsigned int *next)
+{
     /* If this descriptor says it doesn't chain, we're done. */
     if (!(desc->flags & VRING_DESC_F_NEXT)) {
-        return max;
+        return VIRTQUEUE_READ_DESC_DONE;
     }
 
     /* Check they're not leading us off end of descriptors. */
-    next = desc->next;
+    *next = desc->next;
     /* Make sure compiler knows to grab that: we don't want it changing! */
     smp_wmb();
 
-    if (next >= max) {
-        error_report("Desc next is %u", next);
-        exit(1);
+    if (*next >= max) {
+        virtio_error(vdev, "Desc next is %u", *next);
+        return VIRTQUEUE_READ_DESC_ERROR;
     }
 
-    vring_desc_read(vdev, desc, desc_pa, next);
-    return next;
+    vring_desc_read(vdev, desc, desc_pa, *next);
+    return VIRTQUEUE_READ_DESC_MORE;
 }
 
 void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
@@ -407,6 +412,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 {
     unsigned int idx;
     unsigned int total_bufs, in_total, out_total;
+    int rc;
 
     idx = vq->last_avail_idx;
 
@@ -459,7 +465,13 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
             if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
                 goto done;
             }
-        } while ((i = virtqueue_read_next_desc(vdev, &desc, desc_pa, max)) != max);
+
+            rc = virtqueue_read_next_desc(vdev, &desc, desc_pa, max, &i);
+        } while (rc == VIRTQUEUE_READ_DESC_MORE);
+
+        if (rc == VIRTQUEUE_READ_DESC_ERROR) {
+            goto err;
+        }
 
         if (!indirect)
             total_bufs = num_bufs;
@@ -616,6 +628,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     hwaddr addr[VIRTQUEUE_MAX_SIZE];
     struct iovec iov[VIRTQUEUE_MAX_SIZE];
     VRingDesc desc;
+    int rc;
 
     if (unlikely(vdev->broken)) {
         return NULL;
@@ -683,7 +696,13 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
             virtio_error(vdev, "Looped descriptor");
             goto err_undo_map;
         }
-    } while ((i = virtqueue_read_next_desc(vdev, &desc, desc_pa, max)) != max);
+
+        rc = virtqueue_read_next_desc(vdev, &desc, desc_pa, max, &i);
+    } while (rc == VIRTQUEUE_READ_DESC_MORE);
+
+    if (rc == VIRTQUEUE_READ_DESC_ERROR) {
+        goto err_undo_map;
+    }
 
     /* Now copy what we have collected and mapped */
     elem = virtqueue_alloc_element(sz, out_num, in_num);
-- 
2.7.4

[Qemu-devel] [PATCH v4 8/9] virtio: handle virtqueue_num_heads() errors

[Stefan Hajnoczi]

If the avail ring index is bogus virtqueue_num_heads() must return
-EINVAL.

The only caller is virtqueue_get_avail_bytes().  Return saying no bytes
are available when virtqueue_num_heads() fails.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 4d25af4..6635ce4 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -347,9 +347,9 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int idx)
 
     /* Check it isn't doing very strange things with descriptor numbers. */
     if (num_heads > vq->vring.num) {
-        error_report("Guest moved used index from %u to %u",
+        virtio_error(vq->vdev, "Guest moved used index from %u to %u",
                      idx, vq->shadow_avail_idx);
-        exit(1);
+        return -EINVAL;
     }
     /* On success, callers read a descriptor at vq->last_avail_idx.
      * Make sure descriptor read does not bypass avail index read. */
@@ -417,7 +417,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
     idx = vq->last_avail_idx;
 
     total_bufs = in_total = out_total = 0;
-    while (virtqueue_num_heads(vq, idx)) {
+    while ((rc = virtqueue_num_heads(vq, idx)) > 0) {
         VirtIODevice *vdev = vq->vdev;
         unsigned int max, num_bufs, indirect = 0;
         VRingDesc desc;
@@ -478,6 +478,11 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
         else
             total_bufs++;
     }
+
+    if (rc < 0) {
+        goto err;
+    }
+
 done:
     if (in_bytes) {
         *in_bytes = in_total;
-- 
2.7.4

[Qemu-devel] [PATCH v4 9/9] virtio: handle virtqueue_get_head() errors

[Stefan Hajnoczi]

Stop processing the vring if virtqueue_get_head() fetches an
out-of-bounds head index.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6635ce4..21adf41 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -360,21 +360,20 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int idx)
     return num_heads;
 }
 
-static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
+static bool virtqueue_get_head(VirtQueue *vq, unsigned int idx,
+                               unsigned int *head)
 {
-    unsigned int head;
-
     /* Grab the next descriptor number they're advertising, and increment
      * the index we've seen. */
-    head = vring_avail_ring(vq, idx % vq->vring.num);
+    *head = vring_avail_ring(vq, idx % vq->vring.num);
 
     /* If their number is silly, that's a fatal mistake. */
-    if (head >= vq->vring.num) {
-        error_report("Guest says index %u is available", head);
-        exit(1);
+    if (*head >= vq->vring.num) {
+        virtio_error(vq->vdev, "Guest says index %u is available", *head);
+        return false;
     }
 
-    return head;
+    return true;
 }
 
 enum {
@@ -426,7 +425,11 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 
         max = vq->vring.num;
         num_bufs = total_bufs;
-        i = virtqueue_get_head(vq, idx++);
+
+        if (!virtqueue_get_head(vq, idx++, &i)) {
+            goto err;
+        }
+
         desc_pa = vq->vring.desc;
         vring_desc_read(vdev, &desc, desc_pa, i);
 
@@ -655,11 +658,15 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
         return NULL;
     }
 
-    i = head = virtqueue_get_head(vq, vq->last_avail_idx++);
+    if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) {
+        return NULL;
+    }
+
     if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
         vring_set_avail_event(vq, vq->last_avail_idx);
     }
 
+    i = head;
     vring_desc_read(vdev, &desc, desc_pa, i);
     if (desc.flags & VRING_DESC_F_INDIRECT) {
         if (desc.len % sizeof(VRingDesc)) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 4/9] virtio: handle virtqueue_map_desc() errors

[Greg Kurz]

On Tue, 20 Sep 2016 15:49:33 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Errors can occur during virtqueue_pop(), especially in
> virtqueue_map_desc().  In order to handle this we must unmap iov[]
> before returning NULL.  The caller will consider the virtqueue empty and
> the virtio_error() call will have marked the device broken.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

Hi Stefan,

FWIW, Prasad's "virtio: add check for descriptor's mapped address" is already
in Michael's tree:

https://git.kernel.org/cgit/virt/kvm/mst/qemu.git/commit/?h=pci&id=13c9ed60de6faaed325804620d13e7be37ea8183

I guess this patch should take it into account (as already suggested by Laszlo).

Cheers.

--
Greg

>  hw/virtio/virtio.c | 70 +++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 53 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index a841640..c499028 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -484,24 +484,27 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
>      return in_bytes <= in_total && out_bytes <= out_total;
>  }
>  
> -static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iovec *iov,
> +static bool virtqueue_map_desc(VirtIODevice *vdev, unsigned int *p_num_sg,
> +                               hwaddr *addr, struct iovec *iov,
>                                 unsigned int max_num_sg, bool is_write,
>                                 hwaddr pa, size_t sz)
>  {
> +    bool ok = false;
>      unsigned num_sg = *p_num_sg;
>      assert(num_sg <= max_num_sg);
>  
>      if (!sz) {
> -        error_report("virtio: zero sized buffers are not allowed");
> -        exit(1);
> +        virtio_error(vdev, "virtio: zero sized buffers are not allowed");
> +        goto out;
>      }
>  
>      while (sz) {
>          hwaddr len = sz;
>  
>          if (num_sg == max_num_sg) {
> -            error_report("virtio: too many write descriptors in indirect table");
> -            exit(1);
> +            virtio_error(vdev, "virtio: too many write descriptors in "
> +                               "indirect table");
> +            goto out;
>          }
>  
>          iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write);
> @@ -512,7 +515,28 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
>          pa += len;
>          num_sg++;
>      }
> +    ok = true;
> +
> +out:
>      *p_num_sg = num_sg;
> +    return ok;
> +}
> +
> +/* Only used by error code paths before we have a VirtQueueElement (therefore
> + * virtqueue_unmap_sg() can't be used).  Assumes buffers weren't written to
> + * yet.
> + */
> +static void virtqueue_undo_map_desc(unsigned out_num, unsigned in_num,
> +                                    struct iovec *iov)
> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < out_num + in_num; i++) {
> +        int is_write = i >= out_num;
> +
> +        cpu_physical_memory_unmap(iov->iov_base, iov->iov_len, is_write, 0);
> +        iov++;
> +    }
>  }
>  
>  static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
> @@ -604,8 +628,8 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>      max = vq->vring.num;
>  
>      if (vq->inuse >= vq->vring.num) {
> -        error_report("Virtqueue size exceeded");
> -        exit(1);
> +        virtio_error(vdev, "Virtqueue size exceeded");
> +        return NULL;
>      }
>  
>      i = head = virtqueue_get_head(vq, vq->last_avail_idx++);
> @@ -616,8 +640,8 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>      vring_desc_read(vdev, &desc, desc_pa, i);
>      if (desc.flags & VRING_DESC_F_INDIRECT) {
>          if (desc.len % sizeof(VRingDesc)) {
> -            error_report("Invalid size for indirect buffer table");
> -            exit(1);
> +            virtio_error(vdev, "Invalid size for indirect buffer table");
> +            return NULL;
>          }
>  
>          /* loop over the indirect descriptor table */
> @@ -629,22 +653,30 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>  
>      /* Collect all the descriptors */
>      do {
> +        bool map_ok;
> +
>          if (desc.flags & VRING_DESC_F_WRITE) {
> -            virtqueue_map_desc(&in_num, addr + out_num, iov + out_num,
> -                               VIRTQUEUE_MAX_SIZE - out_num, true, desc.addr, desc.len);
> +            map_ok = virtqueue_map_desc(vdev, &in_num, addr + out_num,
> +                                        iov + out_num,
> +                                        VIRTQUEUE_MAX_SIZE - out_num, true,
> +                                        desc.addr, desc.len);
>          } else {
>              if (in_num) {
> -                error_report("Incorrect order for descriptors");
> -                exit(1);
> +                virtio_error(vdev, "Incorrect order for descriptors");
> +                goto err_undo_map;
>              }
> -            virtqueue_map_desc(&out_num, addr, iov,
> -                               VIRTQUEUE_MAX_SIZE, false, desc.addr, desc.len);
> +            map_ok = virtqueue_map_desc(vdev, &out_num, addr, iov,
> +                                        VIRTQUEUE_MAX_SIZE, false,
> +                                        desc.addr, desc.len);
> +        }
> +        if (!map_ok) {
> +            goto err_undo_map;
>          }
>  
>          /* If we've got too many, that implies a descriptor loop. */
>          if ((in_num + out_num) > max) {
> -            error_report("Looped descriptor");
> -            exit(1);
> +            virtio_error(vdev, "Looped descriptor");
> +            goto err_undo_map;
>          }
>      } while ((i = virtqueue_read_next_desc(vdev, &desc, desc_pa, max)) != max);
>  
> @@ -664,6 +696,10 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>  
>      trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
>      return elem;
> +
> +err_undo_map:
> +    virtqueue_undo_map_desc(out_num, in_num, iov);
> +    return NULL;
>  }
>  
>  /* Reading and writing a structure directly to QEMUFile is *awful*, but

Re: [Qemu-devel] [PATCH v4 0/9] virtio: avoid exit() when device enters invalid states

[Cornelia Huck]

On Tue, 20 Sep 2016 15:49:29 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> v4:
>  * Rebase to qemu.git/master
>  * Use "unsigned int" instead of "unsigned" in virtqueue_undo_map_desc() [Cornelia]
> 

FWIW, any unchanged or only trivially-rebased patch retains my r-b or
a-b.

> v3:
>  * Patch 1: Fix typo and clarify commit description [Markus]
>  * Use virtio_set_status() instead of open coding assignment [Cornelia]
>  * Add live migration
> 
> v2:
>  * Add VIRTIO_CONFIG_S_NEEDS_RESET notification for VIRTIO 1.0 [Cornelia]
>    (Note I've sent a Linux virtio_config.h patch to get the constant added to
>    the headers.)
>  * Split int -> unsigned int change into separate commit [Fam]
>  * Fix double "index" typo in commit description [Fam]
> 
> The virtio code calls exit() when the device enters an invalid state.  This
> means invalid vring indices and descriptor chains kill the VM.  See the patch
> descriptions for why this is a bad thing.
> 
> When the virtio device is in the broken state calls to virtqueue_pop() and
> friends will pretend the virtqueue is empty.  This means the device will become
> isolated from guest activity until it is reset again.
> 
> Stefan Hajnoczi (9):
>   virtio: fix stray tab character
>   virtio: stop virtqueue processing if device is broken
>   virtio: migrate vdev->broken flag
>   virtio: handle virtqueue_map_desc() errors
>   virtio: handle virtqueue_get_avail_bytes() errors
>   virtio: use unsigned int for virtqueue_get_avail_bytes() index
>   virtio: handle virtqueue_read_next_desc() errors
>   virtio: handle virtqueue_num_heads() errors
>   virtio: handle virtqueue_get_head() errors
> 
>  hw/virtio/virtio.c         | 232 +++++++++++++++++++++++++++++++++++----------
>  include/hw/virtio/virtio.h |   3 +
>  2 files changed, 184 insertions(+), 51 deletions(-)
> 

Re: [Qemu-devel] [PATCH v4 4/9] virtio: handle virtqueue_map_desc() errors

[Cornelia Huck]

On Wed, 21 Sep 2016 09:02:35 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Tue, 20 Sep 2016 15:49:33 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > Errors can occur during virtqueue_pop(), especially in
> > virtqueue_map_desc().  In order to handle this we must unmap iov[]
> > before returning NULL.  The caller will consider the virtqueue empty and
> > the virtio_error() call will have marked the device broken.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> 
> Hi Stefan,
> 
> FWIW, Prasad's "virtio: add check for descriptor's mapped address" is already
> in Michael's tree:
> 
> https://git.kernel.org/cgit/virt/kvm/mst/qemu.git/commit/?h=pci&id=13c9ed60de6faaed325804620d13e7be37ea8183
> 
> I guess this patch should take it into account (as already suggested by Laszlo).

Agreed.

(...)

> > +/* Only used by error code paths before we have a VirtQueueElement (therefore
> > + * virtqueue_unmap_sg() can't be used).  Assumes buffers weren't written to
> > + * yet.
> > + */
> > +static void virtqueue_undo_map_desc(unsigned out_num, unsigned in_num,

Should the arguments use 'unsigned int' as well, for consistency's sake?

> > +                                    struct iovec *iov)
> > +{
> > +    unsigned int i;
> > +
> > +    for (i = 0; i < out_num + in_num; i++) {
> > +        int is_write = i >= out_num;
> > +
> > +        cpu_physical_memory_unmap(iov->iov_base, iov->iov_len, is_write, 0);
> > +        iov++;
> > +    }
> >  }