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

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

[Stefan Hajnoczi]

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 (7):
  virtio: fix stray tab character
  virtio: stop virtqueue processing if device is broken
  virtio: handle virtqueue_map_desc() errors
  virtio: handle virtqueue_get_avail_bytes() errors
  virtio: handle virtqueue_read_next_desc() errors
  virtio: handle virtqueue_num_heads() errors
  virtio: handle virtqueue_get_head() errors

 hw/virtio/virtio.c         | 199 ++++++++++++++++++++++++++++++++++-----------
 include/hw/virtio/virtio.h |   3 +
 2 files changed, 155 insertions(+), 47 deletions(-)

-- 
2.5.5

[Qemu-devel] [RFC 1/7] 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>
---
 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

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

[Fam Zheng]

On Thu, 03/24 17:56, Stefan Hajnoczi wrote:
> The patches fixes a single occurrence of a tab character that resulted
> in mis-aligned indentation.
> 
> 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 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
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [RFC 2/7] 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         | 34 ++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h |  3 +++
 2 files changed, 37 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index de8a3b3..8fac47c 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,17 @@ 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;
+}
+
 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

Re: [Qemu-devel] [RFC 2/7] virtio: stop virtqueue processing if device is broken

[Fam Zheng]

On Thu, 03/24 17:56, Stefan Hajnoczi 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         | 34 ++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h |  3 +++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index de8a3b3..8fac47c 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,17 @@ 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;
> +}
> +

Since it's not used by the rest of the patch and it is independent, Probably
split this change to a separate patch?

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

Re: [Qemu-devel] [RFC 2/7] virtio: stop virtqueue processing if device is broken

[Stefan Hajnoczi]

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

On Fri, Mar 25, 2016 at 02:48:37PM +0800, Fam Zheng wrote:
> On Thu, 03/24 17:56, Stefan Hajnoczi 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         | 34 ++++++++++++++++++++++++++++++++++
> >  include/hw/virtio/virtio.h |  3 +++
> >  2 files changed, 37 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index de8a3b3..8fac47c 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,17 @@ 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;
> > +}
> > +
> 
> Since it's not used by the rest of the patch and it is independent, Probably
> split this change to a separate patch?

The broken flag is introduced here and without this hunk nothing ever
writes to it.  If you had a static analysis tool it might even warn
about the unused field without virtoi_error().

I did consider introducing virtio_error() in another patch before
sending but I think having it here makes this patch more
self-explanatory.  It doesn't keep the reviewer guessing about how the
broken flag will be set in later patches.

Stefan

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

Re: [Qemu-devel] [RFC 2/7] virtio: stop virtqueue processing if device is broken

[Cornelia Huck]

On Thu, 24 Mar 2016 17:56:49 +0000
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         | 34 ++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h |  3 +++
>  2 files changed, 37 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;

We should set the NEEDS_RESET status flag for virtio-1 devices here, I
think.

> +}
> +

Re: [Qemu-devel] [RFC 2/7] virtio: stop virtqueue processing if device is broken

[Stefan Hajnoczi]

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

On Tue, Mar 29, 2016 at 09:52:11AM +0200, Cornelia Huck wrote:
> On Thu, 24 Mar 2016 17:56:49 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > +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;
> 
> We should set the NEEDS_RESET status flag for virtio-1 devices here, I
> think.

Will do, thanks.

Stefan

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

[Qemu-devel] [RFC 3/7] 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 8fac47c..86352c8 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 4/7] 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 86352c8..4758fe3 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 5/7] 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 | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 4758fe3..f845df2 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;
 
@@ -389,7 +395,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;
@@ -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

Re: [Qemu-devel] [RFC 5/7] virtio: handle virtqueue_read_next_desc() errors

[Fam Zheng]

On Thu, 03/24 17:56, Stefan Hajnoczi wrote:
> Stop processing the vring if an avail ring index is invalid.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 47 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 4758fe3..f845df2 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;
>  
> @@ -389,7 +395,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;

This change seems a candicate for a separate patch, otherwise looks good to me!


>  
>          max = vq->vring.num;
>          num_bufs = total_bufs;
> @@ -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
> 

Re: [Qemu-devel] [RFC 5/7] virtio: handle virtqueue_read_next_desc() errors

[Stefan Hajnoczi]

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

On Fri, Mar 25, 2016 at 03:01:00PM +0800, Fam Zheng wrote:
> On Thu, 03/24 17:56, Stefan Hajnoczi wrote:
> > @@ -389,7 +395,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;
> 
> This change seems a candicate for a separate patch, otherwise looks good to me!

Will fix in the next revision.

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

[Qemu-devel] [RFC 6/7] virtio: handle virtqueue_num_heads() errors

[Stefan Hajnoczi]

If the index 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 f845df2..525af8b 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

Re: [Qemu-devel] [RFC 6/7] virtio: handle virtqueue_num_heads() errors

[Fam Zheng]

On Thu, 03/24 17:56, Stefan Hajnoczi wrote:
> If the index avail ring index is bogus virtqueue_num_heads() must return

s/index avail/avail/ ?

> -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 f845df2..525af8b 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 7/7] 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 525af8b..bf234eb 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