[v3,3/9] virtio-9p: handle handle_9p_output() error

Commit Message

Greg Kurz Sept. 26, 2016, 8:34 a.m. UTC

A broken guest may send a request without providing buffers for the reply
or for the request itself, and virtqueue_pop() will return an element with
either in_num == 0 or out_num == 0.

All 9P requests are expected to start with the following 7-byte header:

            uint32_t size_le;
            uint8_t id;
            uint16_t tag_le;

If iov_to_buf() fails to return these 7 bytes, then something is wrong in
the guest.

In both cases, it is wrong to crash QEMU, since the root cause lies in the
guest.

This patch hence does the following:
- keep the check of in_num since pdu_complete() assumes it has enough
  space to store the reply and we will send something broken to the guest
- let iov_to_buf() handle out_num == 0, since it will return 0 just like
  if the guest had provided an zero-sized buffer.
- call virtio_error() to inform the guest that the device is now broken,
  instead of aborting
- detach the request from the virtqueue and free it

Signed-off-by: Greg Kurz <[e-mail]>
---
v3: - dropped the out_num check (already covered by iov_to_buf())
    - reworded the in_num error message
    - added an error path to detach and free the virtqueue element

I haven't added the R-b tags received during v2 because of the above
changes.
---
 hw/9pfs/virtio-9p-device.c |   26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Comments

Stefan Hajnoczi Sept. 26, 2016, 4:21 p.m. UTC | #1

On Mon, Sep 26, 2016 at 10:34:15AM +0200, Greg Kurz wrote:
> A broken guest may send a request without providing buffers for the reply
> or for the request itself, and virtqueue_pop() will return an element with
> either in_num == 0 or out_num == 0.
> 
> All 9P requests are expected to start with the following 7-byte header:
> 
>             uint32_t size_le;
>             uint8_t id;
>             uint16_t tag_le;
> 
> If iov_to_buf() fails to return these 7 bytes, then something is wrong in
> the guest.
> 
> In both cases, it is wrong to crash QEMU, since the root cause lies in the
> guest.
> 
> This patch hence does the following:
> - keep the check of in_num since pdu_complete() assumes it has enough
>   space to store the reply and we will send something broken to the guest
> - let iov_to_buf() handle out_num == 0, since it will return 0 just like
>   if the guest had provided an zero-sized buffer.
> - call virtio_error() to inform the guest that the device is now broken,
>   instead of aborting
> - detach the request from the virtqueue and free it
> 
> Signed-off-by: Greg Kurz <[e-mail]>
> ---
> v3: - dropped the out_num check (already covered by iov_to_buf())
>     - reworded the in_num error message
>     - added an error path to detach and free the virtqueue element
> 
> I haven't added the R-b tags received during v2 because of the above
> changes.
> ---
>  hw/9pfs/virtio-9p-device.c |   26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <[e-mail]>

Patch

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index e7ea0e45f3dd..a338f6400264 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -41,6 +41,7 @@  static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
     V9fsState *s = &v->state;
     V9fsPDU *pdu;
     ssize_t len;
+    VirtQueueElement *elem;
 
     while ((pdu = pdu_alloc(s))) {
         struct {
@@ -48,21 +49,28 @@  static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
             uint8_t id;
             uint16_t tag_le;
         } QEMU_PACKED out;
-        VirtQueueElement *elem;
 
         elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
         if (!elem) {
-            pdu_free(pdu);
-            break;
+            goto out_free_pdu;
         }
 
-        BUG_ON(elem->out_num == 0 || elem->in_num == 0);
+        if (elem->in_num == 0) {
+            virtio_error(vdev,
+                         "The guest sent a VirtFS request without space for "
+                         "the reply");
+            goto out_free_req;
+        }
         QEMU_BUILD_BUG_ON(sizeof(out) != 7);
 
         v->elems[pdu->idx] = elem;
         len = iov_to_buf(elem->out_sg, elem->out_num, 0,
                          &out, sizeof(out));
-        BUG_ON(len != sizeof(out));
+        if (len != sizeof(out)) {
+            virtio_error(vdev, "The guest sent a malformed VirtFS request: "
+                         "header size is %zd, should be 7", len);
+            goto out_free_req;
+        }
 
         pdu->size = le32_to_cpu(out.size_le);
 
@@ -72,6 +80,14 @@  static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
         qemu_co_queue_init(&pdu->complete);
         pdu_submit(pdu);
     }
+
+    return;
+
+out_free_req:
+    virtqueue_detach_element(vq, elem, 0);
+    g_free(elem);
+out_free_pdu:
+    pdu_free(pdu);
 }
 
 static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features,