diff mbox series

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

Message ID 20200812104918.107116-4-stefanha@redhat.com
State Superseded
Headers show
Series virtio: restore elem->in/out_sg after iov_discard_front/back() | expand

Commit Message

Stefan Hajnoczi Aug. 12, 2020, 10:49 a.m. UTC 
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(-)

Comments

Stefan Hajnoczi Sept. 16, 2020, 10:12 a.m. UTC | #1 
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.
diff mbox series

Patch

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