[PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back()

[PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back()

[Stefan Hajnoczi]

v2:
 * Add missing undo in virtio-blk write zeroes error path [Li Qiang]

Both virtio-blk and virtio-crypto use destructive iov_discard_front/back()
operations on elem->in/out_sg. virtqueue_push() calls dma_memory_unmap() on the
modified iovec arrays. The memory addresses may not match those originally
mapped with dma_memory_map().

This raises several issues:
1. MemoryRegion references can be leaked.
2. Dirty memory may not be tracked.
3. The non-RAM bounce buffer can be leaked.

This patch series solves the issue in two ways:
1. virtio-blk uses a new iov_discard_undo() API to restore iovec arrays.
2. virtio-crypto uses g_memdup() to avoid modifying the original iovec arrays.

The g_memdup() approach is slower than iov_discard_undo() but less
complex/fragile. I am less familiar with the virtio-crypto code and it uses
more complex sequences of iov_discard_front/back() calls than virtio-blk. If
anyone feels like optimizing virtio-crypto, please go ahead.

The virtio-blk bug was found by Alexander Bulekov's fuzzing effort. I found the
virtio-crypto bug through code inspection.

Stefan Hajnoczi (3):
  util/iov: add iov_discard_undo()
  virtio-blk: undo destructive iov_discard_*() operations
  virtio-crypto: don't modify elem->in/out_sg

 include/hw/virtio/virtio-blk.h |   2 +
 include/qemu/iov.h             |  23 +++++
 hw/block/virtio-blk.c          |  11 ++-
 hw/virtio/virtio-crypto.c      |  17 +++-
 tests/test-iov.c               | 165 +++++++++++++++++++++++++++++++++
 util/iov.c                     |  50 +++++++++-
 6 files changed, 259 insertions(+), 9 deletions(-)

-- 
2.26.2

[PATCH v2 1/3] util/iov: add iov_discard_undo()

[Stefan Hajnoczi]

The iov_discard_front/back() operations are useful for parsing iovecs
but they modify the array elements. If the original array is needed
after parsing finishes there is currently no way to restore it.

Although g_memdup() can be used before performing destructive
iov_discard_front/back() operations, this is inefficient.

Introduce iov_discard_undo() to restore the array to the state prior to
an iov_discard_front/back() operation.

Reviewed-by: Li Qiang <liq3ea@gmail.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/iov.h |  23 +++++++
 tests/test-iov.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++
 util/iov.c         |  50 ++++++++++++--
 3 files changed, 234 insertions(+), 4 deletions(-)

diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index bffc151282..b6b283a5e5 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -130,6 +130,29 @@ size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
 size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
                         size_t bytes);
 
+/* Information needed to undo an iov_discard_*() operation */
+typedef struct {
+    struct iovec *modified_iov;
+    struct iovec orig;
+} IOVDiscardUndo;
+
+/*
+ * Undo an iov_discard_front_undoable() or iov_discard_back_undoable()
+ * operation. If multiple operations are made then each one needs a separate
+ * IOVDiscardUndo and iov_discard_undo() must be called in the reverse order
+ * that the operations were made.
+ */
+void iov_discard_undo(IOVDiscardUndo *undo);
+
+/*
+ * Undoable versions of iov_discard_front() and iov_discard_back(). Use
+ * iov_discard_undo() to reset to the state before the discard operations.
+ */
+size_t iov_discard_front_undoable(struct iovec **iov, unsigned int *iov_cnt,
+                                  size_t bytes, IOVDiscardUndo *undo);
+size_t iov_discard_back_undoable(struct iovec *iov, unsigned int *iov_cnt,
+                                 size_t bytes, IOVDiscardUndo *undo);
+
 typedef struct QEMUIOVector {
     struct iovec *iov;
     int niov;
diff --git a/tests/test-iov.c b/tests/test-iov.c
index 458ca25099..9c415e2f1f 100644
--- a/tests/test-iov.c
+++ b/tests/test-iov.c
@@ -26,6 +26,12 @@ static void iov_free(struct iovec *iov, unsigned niov)
     g_free(iov);
 }
 
+static bool iov_equals(const struct iovec *a, const struct iovec *b,
+                       unsigned niov)
+{
+    return memcmp(a, b, sizeof(a[0]) * niov) == 0;
+}
+
 static void test_iov_bytes(struct iovec *iov, unsigned niov,
                            size_t offset, size_t bytes)
 {
@@ -335,6 +341,87 @@ static void test_discard_front(void)
     iov_free(iov, iov_cnt);
 }
 
+static void test_discard_front_undo(void)
+{
+    IOVDiscardUndo undo;
+    struct iovec *iov;
+    struct iovec *iov_tmp;
+    struct iovec *iov_orig;
+    unsigned int iov_cnt;
+    unsigned int iov_cnt_tmp;
+    size_t size;
+
+    /* Discard zero bytes */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, 0, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard more bytes than vector size */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    size = iov_size(iov, iov_cnt);
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size + 1, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard entire vector */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    size = iov_size(iov, iov_cnt);
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard within first element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    size = g_test_rand_int_range(1, iov->iov_len);
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard entire first element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, iov->iov_len, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard within second element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    size = iov->iov_len + g_test_rand_int_range(1, iov[1].iov_len);
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+}
+
 static void test_discard_back(void)
 {
     struct iovec *iov;
@@ -404,6 +491,82 @@ static void test_discard_back(void)
     iov_free(iov, iov_cnt);
 }
 
+static void test_discard_back_undo(void)
+{
+    IOVDiscardUndo undo;
+    struct iovec *iov;
+    struct iovec *iov_orig;
+    unsigned int iov_cnt;
+    unsigned int iov_cnt_tmp;
+    size_t size;
+
+    /* Discard zero bytes */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, 0, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard more bytes than vector size */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    size = iov_size(iov, iov_cnt);
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, size + 1, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard entire vector */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    size = iov_size(iov, iov_cnt);
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard within last element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    size = g_test_rand_int_range(1, iov[iov_cnt - 1].iov_len);
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard entire last element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    size = iov[iov_cnt - 1].iov_len;
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard within second-to-last element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    size = iov[iov_cnt - 1].iov_len +
+           g_test_rand_int_range(1, iov[iov_cnt - 2].iov_len);
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -412,5 +575,7 @@ int main(int argc, char **argv)
     g_test_add_func("/basic/iov/io", test_io);
     g_test_add_func("/basic/iov/discard-front", test_discard_front);
     g_test_add_func("/basic/iov/discard-back", test_discard_back);
+    g_test_add_func("/basic/iov/discard-front-undo", test_discard_front_undo);
+    g_test_add_func("/basic/iov/discard-back-undo", test_discard_back_undo);
     return g_test_run();
 }
diff --git a/util/iov.c b/util/iov.c
index ae61d696aa..f3a9e92a37 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -636,14 +636,33 @@ void qemu_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src, void *buf)
     }
 }
 
-size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
-                         size_t bytes)
+void iov_discard_undo(IOVDiscardUndo *undo)
+{
+    /* Restore original iovec if it was modified */
+    if (undo->modified_iov) {
+        *undo->modified_iov = undo->orig;
+    }
+}
+
+size_t iov_discard_front_undoable(struct iovec **iov,
+                                  unsigned int *iov_cnt,
+                                  size_t bytes,
+                                  IOVDiscardUndo *undo)
 {
     size_t total = 0;
     struct iovec *cur;
 
+    if (undo) {
+        undo->modified_iov = NULL;
+    }
+
     for (cur = *iov; *iov_cnt > 0; cur++) {
         if (cur->iov_len > bytes) {
+            if (undo) {
+                undo->modified_iov = cur;
+                undo->orig = *cur;
+            }
+
             cur->iov_base += bytes;
             cur->iov_len -= bytes;
             total += bytes;
@@ -659,12 +678,24 @@ size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
     return total;
 }
 
-size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
-                        size_t bytes)
+size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
+                         size_t bytes)
+{
+    return iov_discard_front_undoable(iov, iov_cnt, bytes, NULL);
+}
+
+size_t iov_discard_back_undoable(struct iovec *iov,
+                                 unsigned int *iov_cnt,
+                                 size_t bytes,
+                                 IOVDiscardUndo *undo)
 {
     size_t total = 0;
     struct iovec *cur;
 
+    if (undo) {
+        undo->modified_iov = NULL;
+    }
+
     if (*iov_cnt == 0) {
         return 0;
     }
@@ -673,6 +704,11 @@ size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
 
     while (*iov_cnt > 0) {
         if (cur->iov_len > bytes) {
+            if (undo) {
+                undo->modified_iov = cur;
+                undo->orig = *cur;
+            }
+
             cur->iov_len -= bytes;
             total += bytes;
             break;
@@ -687,6 +723,12 @@ size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
     return total;
 }
 
+size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
+                        size_t bytes)
+{
+    return iov_discard_back_undoable(iov, iov_cnt, bytes, NULL);
+}
+
 void qemu_iovec_discard_back(QEMUIOVector *qiov, size_t bytes)
 {
     size_t total;
-- 
2.26.2

[PATCH v2 2/3] virtio-blk: undo destructive iov_discard_*() operations

[Stefan Hajnoczi]

Fuzzing discovered that virtqueue_unmap_sg() is being called on modified
req->in/out_sg iovecs. This means dma_memory_map() and
dma_memory_unmap() calls do not have matching memory addresses.

Fuzzing discovered that non-RAM addresses trigger a bug:

  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
                           bool is_write, hwaddr access_len)
  {
      if (buffer != bounce.buffer) {
          ^^^^^^^^^^^^^^^^^^^^^^^

A modified iov->iov_base is no longer recognized as a bounce buffer and
the wrong branch is taken.

There are more potential bugs: dirty memory is not tracked correctly and
MemoryRegion refcounts can be leaked.

Use the new iov_discard_undo() API to restore elem->in/out_sg before
virtqueue_push() is called.

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Buglink: https://bugs.launchpad.net/qemu/+bug/1890360
Fixes: 827805a2492c1bbf1c0712ed18ee069b4ebf3dd6 ("virtio-blk: Convert VirtIOBlockReq.out to structrue")
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio-blk.h |  2 ++
 hw/block/virtio-blk.c          | 11 +++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 29c9f32353..df3876d49c 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -72,6 +72,8 @@ typedef struct VirtIOBlockReq {
     int64_t sector_num;
     VirtIOBlock *dev;
     VirtQueue *vq;
+    IOVDiscardUndo inhdr_undo;
+    IOVDiscardUndo outhdr_undo;
     struct virtio_blk_inhdr *in;
     struct virtio_blk_outhdr out;
     QEMUIOVector qiov;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 2204ba149e..bac2d6fa2b 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -80,6 +80,8 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
     trace_virtio_blk_req_complete(vdev, req, status);
 
     stb_p(&req->in->status, status);
+    iov_discard_undo(&req->inhdr_undo);
+    iov_discard_undo(&req->outhdr_undo);
     virtqueue_push(req->vq, &req->elem, req->in_len);
     if (s->dataplane_started && !s->dataplane_disabled) {
         virtio_blk_data_plane_notify(s->dataplane, req->vq);
@@ -632,10 +634,12 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
         return -1;
     }
 
-    iov_discard_front(&out_iov, &out_num, sizeof(req->out));
+    iov_discard_front_undoable(&out_iov, &out_num, sizeof(req->out),
+                               &req->outhdr_undo);
 
     if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
         virtio_error(vdev, "virtio-blk request inhdr too short");
+        iov_discard_undo(&req->outhdr_undo);
         return -1;
     }
 
@@ -644,7 +648,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     req->in = (void *)in_iov[in_num - 1].iov_base
               + in_iov[in_num - 1].iov_len
               - sizeof(struct virtio_blk_inhdr);
-    iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr));
+    iov_discard_back_undoable(in_iov, &in_num, sizeof(struct virtio_blk_inhdr),
+                              &req->inhdr_undo);
 
     type = virtio_ldl_p(vdev, &req->out.type);
 
@@ -739,6 +744,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 
         if (unlikely(iov_to_buf(out_iov, out_num, 0, &dwz_hdr,
                                 sizeof(dwz_hdr)) != sizeof(dwz_hdr))) {
+            iov_discard_undo(&req->inhdr_undo);
+            iov_discard_undo(&req->outhdr_undo);
             virtio_error(vdev, "virtio-blk discard/write_zeroes header"
                          " too short");
             return -1;
-- 
2.26.2

Re: [PATCH v2 2/3] virtio-blk: undo destructive iov_discard_*() operations

[Li Qiang]

Stefan Hajnoczi <stefanha@redhat.com> 于2020年9月17日周四 下午5:47写道：
>
> Fuzzing discovered that virtqueue_unmap_sg() is being called on modified
> req->in/out_sg iovecs. This means dma_memory_map() and
> dma_memory_unmap() calls do not have matching memory addresses.
>
> Fuzzing discovered that non-RAM addresses trigger a bug:
>
>   void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>                            bool is_write, hwaddr access_len)
>   {
>       if (buffer != bounce.buffer) {
>           ^^^^^^^^^^^^^^^^^^^^^^^
>
> A modified iov->iov_base is no longer recognized as a bounce buffer and
> the wrong branch is taken.
>
> There are more potential bugs: dirty memory is not tracked correctly and
> MemoryRegion refcounts can be leaked.
>
> Use the new iov_discard_undo() API to restore elem->in/out_sg before
> virtqueue_push() is called.
>
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1890360
> Fixes: 827805a2492c1bbf1c0712ed18ee069b4ebf3dd6 ("virtio-blk: Convert VirtIOBlockReq.out to structrue")
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  include/hw/virtio/virtio-blk.h |  2 ++
>  hw/block/virtio-blk.c          | 11 +++++++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 29c9f32353..df3876d49c 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -72,6 +72,8 @@ typedef struct VirtIOBlockReq {
>      int64_t sector_num;
>      VirtIOBlock *dev;
>      VirtQueue *vq;
> +    IOVDiscardUndo inhdr_undo;
> +    IOVDiscardUndo outhdr_undo;
>      struct virtio_blk_inhdr *in;
>      struct virtio_blk_outhdr out;
>      QEMUIOVector qiov;
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 2204ba149e..bac2d6fa2b 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -80,6 +80,8 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
>      trace_virtio_blk_req_complete(vdev, req, status);
>
>      stb_p(&req->in->status, status);
> +    iov_discard_undo(&req->inhdr_undo);
> +    iov_discard_undo(&req->outhdr_undo);
>      virtqueue_push(req->vq, &req->elem, req->in_len);
>      if (s->dataplane_started && !s->dataplane_disabled) {
>          virtio_blk_data_plane_notify(s->dataplane, req->vq);
> @@ -632,10 +634,12 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>          return -1;
>      }
>
> -    iov_discard_front(&out_iov, &out_num, sizeof(req->out));
> +    iov_discard_front_undoable(&out_iov, &out_num, sizeof(req->out),
> +                               &req->outhdr_undo);
>
>      if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
>          virtio_error(vdev, "virtio-blk request inhdr too short");
> +        iov_discard_undo(&req->outhdr_undo);
>          return -1;
>      }
>
> @@ -644,7 +648,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>      req->in = (void *)in_iov[in_num - 1].iov_base
>                + in_iov[in_num - 1].iov_len
>                - sizeof(struct virtio_blk_inhdr);
> -    iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr));
> +    iov_discard_back_undoable(in_iov, &in_num, sizeof(struct virtio_blk_inhdr),
> +                              &req->inhdr_undo);
>
>      type = virtio_ldl_p(vdev, &req->out.type);
>
> @@ -739,6 +744,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>
>          if (unlikely(iov_to_buf(out_iov, out_num, 0, &dwz_hdr,
>                                  sizeof(dwz_hdr)) != sizeof(dwz_hdr))) {
> +            iov_discard_undo(&req->inhdr_undo);
> +            iov_discard_undo(&req->outhdr_undo);
>              virtio_error(vdev, "virtio-blk discard/write_zeroes header"
>                           " too short");
>              return -1;
> --
> 2.26.2
>

[PATCH v2 3/3] virtio-crypto: don't modify elem->in/out_sg

[Stefan Hajnoczi]

A number of iov_discard_front/back() operations are made by
virtio-crypto. The elem->in/out_sg iovec arrays are modified by these
operations, resulting virtqueue_unmap_sg() calls on different addresses
than were originally mapped.

This is problematic because dirty memory may not be logged correctly,
MemoryRegion refcounts may be leaked, and the non-RAM bounce buffer can
be leaked.

Take a copy of the elem->in/out_sg arrays so that the originals are
preserved. The iov_discard_undo() API could be used instead (with better
performance) but requires careful auditing of the code, so do the simple
thing instead.

Reviewed-by: Li Qiang <liq3ea@gmail.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-crypto.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 6da12e315f..54f9bbb789 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -228,6 +228,8 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     size_t s;
 
     for (;;) {
+        g_autofree struct iovec *out_iov_copy = NULL;
+
         elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
         if (!elem) {
             break;
@@ -240,9 +242,12 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         }
 
         out_num = elem->out_num;
-        out_iov = elem->out_sg;
+        out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) * out_num);
+        out_iov = out_iov_copy;
+
         in_num = elem->in_num;
         in_iov = elem->in_sg;
+
         if (unlikely(iov_to_buf(out_iov, out_num, 0, &ctrl, sizeof(ctrl))
                     != sizeof(ctrl))) {
             virtio_error(vdev, "virtio-crypto request ctrl_hdr too short");
@@ -582,6 +587,8 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
     int queue_index = virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
     struct virtio_crypto_op_data_req req;
     int ret;
+    g_autofree struct iovec *in_iov_copy = NULL;
+    g_autofree struct iovec *out_iov_copy = NULL;
     struct iovec *in_iov;
     struct iovec *out_iov;
     unsigned in_num;
@@ -598,9 +605,13 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
     }
 
     out_num = elem->out_num;
-    out_iov = elem->out_sg;
+    out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) * out_num);
+    out_iov = out_iov_copy;
+
     in_num = elem->in_num;
-    in_iov = elem->in_sg;
+    in_iov_copy = g_memdup(elem->in_sg, sizeof(in_iov[0]) * in_num);
+    in_iov = in_iov_copy;
+
     if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
                 != sizeof(req))) {
         virtio_error(vdev, "virtio-crypto request outhdr too short");
-- 
2.26.2

Re: [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back()

[Stefan Hajnoczi]

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

On Thu, Sep 17, 2020 at 10:44:52AM +0100, Stefan Hajnoczi wrote:
> v2:
>  * Add missing undo in virtio-blk write zeroes error path [Li Qiang]
> 
> Both virtio-blk and virtio-crypto use destructive iov_discard_front/back()
> operations on elem->in/out_sg. virtqueue_push() calls dma_memory_unmap() on the
> modified iovec arrays. The memory addresses may not match those originally
> mapped with dma_memory_map().
> 
> This raises several issues:
> 1. MemoryRegion references can be leaked.
> 2. Dirty memory may not be tracked.
> 3. The non-RAM bounce buffer can be leaked.
> 
> This patch series solves the issue in two ways:
> 1. virtio-blk uses a new iov_discard_undo() API to restore iovec arrays.
> 2. virtio-crypto uses g_memdup() to avoid modifying the original iovec arrays.
> 
> The g_memdup() approach is slower than iov_discard_undo() but less
> complex/fragile. I am less familiar with the virtio-crypto code and it uses
> more complex sequences of iov_discard_front/back() calls than virtio-blk. If
> anyone feels like optimizing virtio-crypto, please go ahead.
> 
> The virtio-blk bug was found by Alexander Bulekov's fuzzing effort. I found the
> virtio-crypto bug through code inspection.
> 
> Stefan Hajnoczi (3):
>   util/iov: add iov_discard_undo()
>   virtio-blk: undo destructive iov_discard_*() operations
>   virtio-crypto: don't modify elem->in/out_sg
> 
>  include/hw/virtio/virtio-blk.h |   2 +
>  include/qemu/iov.h             |  23 +++++
>  hw/block/virtio-blk.c          |  11 ++-
>  hw/virtio/virtio-crypto.c      |  17 +++-
>  tests/test-iov.c               | 165 +++++++++++++++++++++++++++++++++
>  util/iov.c                     |  50 +++++++++-
>  6 files changed, 259 insertions(+), 9 deletions(-)
> 
> -- 
> 2.26.2
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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