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

[Li Qiang <liq3ea@gmail.com>]

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