[v3,05/10] virtio: handle virtqueue_map_desc() errors

Commit Message

Stefan Hajnoczi April 12, 2016, 1:25 p.m. UTC

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 <[e-mail]>
---
 hw/virtio/virtio.c | 62 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 13 deletions(-)

Comments

Cornelia Huck Sept. 20, 2016, 11:41 a.m. UTC | #1

On Tue, 12 Apr 2016 14:25:29 +0100
Stefan Hajnoczi <[e-mail]> 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 <[e-mail]>
> ---
>  hw/virtio/virtio.c | 62 ++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 13 deletions(-)
> 

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

Hm... didn't such an undo function float around in another context as
well? Is it worth factoring out?

> +static void virtqueue_undo_map_desc(unsigned out_num, unsigned in_num,
> +                                    struct iovec *iov)
> +{
> +    unsigned i;

Personally, I don't like pure 'unsigned'. Matter of taste, I guess.

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

Otherwise, looks good.

Patch

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6e6b968..0c0d333 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -460,10 +460,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);
 
@@ -471,8 +473,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);
@@ -483,7 +486,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,
@@ -582,8 +606,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 */
@@ -595,22 +619,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);
 
@@ -630,6 +662,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