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

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

[Stefan Hajnoczi]

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.

RFC because two things are missing:
1. Live migration support (subsection for broken flag?)
2. Auditing devices and replacing exit() calls there too

Stefan Hajnoczi (9):
  virtio: fix stray tab character
  include: update virtio_config.h Linux header
  virtio: stop virtqueue processing if device is broken
  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                             | 204 +++++++++++++++++++------
 include/hw/virtio/virtio.h                     |   3 +
 include/standard-headers/linux/virtio_config.h |   2 +
 3 files changed, 162 insertions(+), 47 deletions(-)

-- 
2.5.5

[Qemu-devel] [RFC v2 1/9] virtio: fix stray tab character

[Stefan Hajnoczi]

The patches fixes a single occurrence of a tab character that resulted
in mis-aligned indentation.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@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 08275a9..de8a3b3 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1531,7 +1531,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.5.5

[Qemu-devel] [RFC v2 2/9] include: update virtio_config.h Linux header

[Stefan Hajnoczi]

Bring in the VIRTIO_CONFIG_S_NEEDS_RESET device status bit definition.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/standard-headers/linux/virtio_config.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/standard-headers/linux/virtio_config.h b/include/standard-headers/linux/virtio_config.h
index bcc445b..b30d0cb 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -40,6 +40,8 @@
 #define VIRTIO_CONFIG_S_DRIVER_OK	4
 /* Driver has finished configuring features */
 #define VIRTIO_CONFIG_S_FEATURES_OK	8
+/* Device entered invalid state, driver must reset it */
+#define VIRTIO_CONFIG_S_NEEDS_RESET	0x40
 /* We've given up on this device. */
 #define VIRTIO_CONFIG_S_FAILED		0x80
 
-- 
2.5.5

[Qemu-devel] [RFC v2 3/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 de8a3b3..ff2c736 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -276,6 +276,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;
@@ -286,6 +290,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);
@@ -546,6 +556,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;
     }
@@ -705,6 +718,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);
     }
@@ -788,6 +805,7 @@ void virtio_reset(void *opaque)
         k->reset(vdev);
     }
 
+    vdev->broken = false;
     vdev->guest_features = 0;
     vdev->queue_sel = 0;
     vdev->status = 0;
@@ -1091,6 +1109,10 @@ 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);
     }
@@ -1661,6 +1683,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;
@@ -1829,6 +1852,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)) {
+        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 2b5b248..1565e53 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.5.5

[Qemu-devel] [RFC v2 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 | 62 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 13 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ff2c736..c07b451 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -457,10 +457,12 @@ 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);
 
@@ -468,8 +470,9 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
         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);
@@ -480,7 +483,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 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,
@@ -579,8 +603,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 */
@@ -592,22 +616,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);
 
@@ -627,6 +659,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.5.5

[Qemu-devel] [RFC v2 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 c07b451..a7459c2 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -399,14 +399,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 */
@@ -420,8 +420,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) {
@@ -446,6 +446,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.5.5

[Qemu-devel] [RFC v2 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 a7459c2..07ce02b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -389,7 +389,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.5.5

[Qemu-devel] [RFC v2 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 07ce02b..de90824 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -350,28 +350,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,
@@ -380,6 +385,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;
 
@@ -432,7 +438,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;
@@ -584,6 +596,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;
@@ -646,7 +659,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.5.5

[Qemu-devel] [RFC v2 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 de90824..8ffcddc 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -320,9 +320,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. */
@@ -390,7 +390,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;
@@ -451,6 +451,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.5.5

[Qemu-devel] [RFC v2 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 | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 8ffcddc..0dd48d4 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -333,21 +333,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 {
@@ -399,7 +398,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);
 
@@ -618,11 +621,14 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 
     max = vq->vring.num;
 
-    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.5.5

Re: [Qemu-devel] [RFC v2 3/9] virtio: stop virtqueue processing if device is broken

[Cornelia Huck]

On Tue, 29 Mar 2016 17:12:55 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

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

> +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)) {
> +        vdev->status |= VIRTIO_CONFIG_S_NEEDS_RESET;

virtio_set_status()?

> +        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 2b5b248..1565e53 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 */

I'm wondering whether there's a sane way to track the broken state via
the NEEDS_RESET status bit instead. We'd probably want to filter out
this bit and not expose it to legacy drivers; but as the status field
is migrated anyway, we might be able to avoid a subsection for
migration.

Re: [Qemu-devel] [RFC v2 3/9] virtio: stop virtqueue processing if device is broken

[Stefan Hajnoczi]

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

On Tue, Mar 29, 2016 at 06:58:03PM +0200, Cornelia Huck wrote:
> On Tue, 29 Mar 2016 17:12:55 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 2b5b248..1565e53 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 */
> 
> I'm wondering whether there's a sane way to track the broken state via
> the NEEDS_RESET status bit instead. We'd probably want to filter out
> this bit and not expose it to legacy drivers; but as the status field
> is migrated anyway, we might be able to avoid a subsection for
> migration.

If we set the non-VIRTIO 1.0 bit and migrate to an old QEMU that doesn't
filter the bit then the guest will see it.

Therefore I'm in favor of keeping vdev->broken separate from the status
bit.  The subsection only needs to be sent when the bit is set.  Only
migration of a broken device to an old QEMU will fail.  All other cases
continue to work so the subsection doesn't impose much incompatibility.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [RFC v2 3/9] virtio: stop virtqueue processing if device is broken

[Stefan Hajnoczi]

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

On Tue, Mar 29, 2016 at 06:58:03PM +0200, Cornelia Huck wrote:
> On Tue, 29 Mar 2016 17:12:55 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > 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(+)
> 
> > +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)) {
> > +        vdev->status |= VIRTIO_CONFIG_S_NEEDS_RESET;
> 
> virtio_set_status()?

Will fix in next revision

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [RFC v2 1/9] virtio: fix stray tab character

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> The patches fixes a single occurrence of a tab character that resulted

"The patch fixes" or "Fix", or drop the sentence.

> in mis-aligned indentation.

Really?  It comes out aligned in my editor.

> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [RFC v2 1/9] virtio: fix stray tab character

[Stefan Hajnoczi]

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

On Thu, Apr 07, 2016 at 04:50:18PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > The patches fixes a single occurrence of a tab character that resulted
> 
> "The patch fixes" or "Fix", or drop the sentence.
> 
> > in mis-aligned indentation.
> 
> Really?  It comes out aligned in my editor.

If your editor uses an unholy 8 space tab width setting then it will
appear correct.  But deep down we'll both know it's wrong :).

(I'm using tab width 4.)

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [RFC v2 1/9] virtio: fix stray tab character

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Thu, Apr 07, 2016 at 04:50:18PM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>> > The patches fixes a single occurrence of a tab character that resulted
>> 
>> "The patch fixes" or "Fix", or drop the sentence.
>> 
>> > in mis-aligned indentation.
>> 
>> Really?  It comes out aligned in my editor.
>
> If your editor uses an unholy 8 space tab width setting then it will
> appear correct.  But deep down we'll both know it's wrong :).
>
> (I'm using tab width 4.)

Heretic!