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

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

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.