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

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

[Stefan Hajnoczi]

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 t=
he
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 t=
he
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          |   9 +-
 hw/virtio/virtio-crypto.c      |  17 +++-
 tests/test-iov.c               | 165 +++++++++++++++++++++++++++++++++
 util/iov.c                     |  50 +++++++++-
 6 files changed, 257 insertions(+), 9 deletions(-)

--=20
2.26.2

[PATCH 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.

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 45ef3043ee..efcf04b445 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 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          | 9 +++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index b1334c3904..0af654cace 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -68,6 +68,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 413783693c..2b7cc3e1c8 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);
 
-- 
2.26.2

[PATCH 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.

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 1/3] util/iov: add iov_discard_undo()

[Li Qiang]

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

Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:52写道：

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

Seems there are some errors. See below



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

The 'iov_discard_front_undoable' will change the 'iov_tmp' it will not
touch 'iov_orig'.
So the test will be always passed. The actually function will not be tested.

Also, maybe we could abstract a function to do these discard test?


> +    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 45ef3043ee..efcf04b445 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;
> +            }
> +
>

Why here we remember the 'cur'? 'cur' is the some of the 'iov'.
Maybe we remember the 'iov'. I think we need the undo structure like this:

struct IOVUndo {
    iov **modified_iov;
    iov *orig;
};

Then we can remeber the origin 'iov'.

Thanks,
Li Qiang



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

[-- Attachment #2: Type: text/html, Size: 15923 bytes --]

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

[Li Qiang]

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

Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:51写道：

> 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.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>

virtio-net also uses this method.

Reviewed-by: Li Qiang <liq3ea@gmail.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
>
>

[-- Attachment #2: Type: text/html, Size: 4124 bytes --]

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

[Stefan Hajnoczi]

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

On Sun, Aug 16, 2020 at 04:26:45PM +0800, Li Qiang wrote:
> Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:52写道：

Thanks for your review!

> > +    /* 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));
> >
> 
> The 'iov_discard_front_undoable' will change the 'iov_tmp' it will not
> touch 'iov_orig'.
> So the test will be always passed. The actually function will not be tested.

The test verifies that the iovec elements are restored to their previous
state by iov_discard_undo().

I think you are saying you'd like iov_discard_undo() to reset the
iov_tmp pointer? Currently that is not how the API works. The caller is
assumed to have the original pointer (e.g. virtio-blk has
req->elem.in/out_sg) and therefore it is not necessary to reset iov_tmp.

> Also, maybe we could abstract a function to do these discard test?

The structure of the test cases is similar but they vary in different
places. I'm not sure if this can be abstracted nicely.

> > diff --git a/util/iov.c b/util/iov.c
> > index 45ef3043ee..efcf04b445 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;
> > +            }
> > +
> >
> 
> Why here we remember the 'cur'? 'cur' is the some of the 'iov'.
> Maybe we remember the 'iov'. I think we need the undo structure like this:
> 
> struct IOVUndo {
>     iov **modified_iov;
>     iov *orig;
> };
> 
> Then we can remeber the origin 'iov'.

Yes, this could be done but it's not needed (yet?). VirtQueueElement has
the original struct iovec pointers so adding this would be redundant.

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

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

[Stefan Hajnoczi]

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

On Sun, Aug 16, 2020 at 04:32:06PM +0800, Li Qiang wrote:
> Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:51写道：
> 
> > 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.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >
> 
> virtio-net also uses this method.

virtio-net operates on a copy of the iovecs (g_memdup()) so no changes
are necessary.

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

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

[Li Qiang]

Stefan Hajnoczi <stefanha@redhat.com> 于2020年9月16日周三 下午6:09写道：
>
> On Sun, Aug 16, 2020 at 04:26:45PM +0800, Li Qiang wrote:
> > Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:52写道：
>
> Thanks for your review!
>
> > > +    /* 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));
> > >
> >
> > The 'iov_discard_front_undoable' will change the 'iov_tmp' it will not
> > touch 'iov_orig'.
> > So the test will be always passed. The actually function will not be tested.
>
> The test verifies that the iovec elements are restored to their previous
> state by iov_discard_undo().
>
> I think you are saying you'd like iov_discard_undo() to reset the
> iov_tmp pointer? Currently that is not how the API works. The caller is
> assumed to have the original pointer (e.g. virtio-blk has
> req->elem.in/out_sg) and therefore it is not necessary to reset iov_tmp.
>

Hi Stefan,

You're right, I just have the idea in my mind the the 'discard'
function change the *iov, but the caller have the origin pointer.
So

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

> > Also, maybe we could abstract a function to do these discard test?
>
> The structure of the test cases is similar but they vary in different
> places. I'm not sure if this can be abstracted nicely.
>
> > > diff --git a/util/iov.c b/util/iov.c
> > > index 45ef3043ee..efcf04b445 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;
> > > +            }
> > > +
> > >
> >
> > Why here we remember the 'cur'? 'cur' is the some of the 'iov'.
> > Maybe we remember the 'iov'. I think we need the undo structure like this:
> >
> > struct IOVUndo {
> >     iov **modified_iov;
> >     iov *orig;
> > };
> >
> > Then we can remeber the origin 'iov'.
>
> Yes, this could be done but it's not needed (yet?). VirtQueueElement has
> the original struct iovec pointers so adding this would be redundant.

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

[Li Qiang]

Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:51写道：
>
> 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          | 9 +++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index b1334c3904..0af654cace 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -68,6 +68,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 413783693c..2b7cc3e1c8 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);
>

It seems there is another error path need to do the undo operations.
case VIRTIO_BLK_T_WRITE_ZEROS & ~VIRTIO_BLK_T_OUT
handler has an error path.

Thanks,
Li Qiang

> --
> 2.26.2
>

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

[Stefan Hajnoczi]

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

On Wed, Sep 16, 2020 at 11:38:36PM +0800, Li Qiang wrote:
> Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:51写道：
> > @@ -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);
> >
> 
> It seems there is another error path need to do the undo operations.
> case VIRTIO_BLK_T_WRITE_ZEROS & ~VIRTIO_BLK_T_OUT
> handler has an error path.

Yes, thank you! I'll fix it in the next revision.

Stefan

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