When mergeable buffer is enabled, we try to set the num_buffers after
the virtqueue elem has been unmapped. This will lead several issues,
E.g a use after free when the descriptor has an address which belongs
to the non direct access region. In this case we use bounce buffer
that is allocated during address_space_map() and freed during
address_space_unmap().

Fixing this by storing the elems temporarily in an array and delay the
unmap after we set the the num_buffers.

This addresses CVE-2021-3748.

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Fixes: fbe78f4f55c6 ("virtio-net support")
Cc: qemu-stable@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 16d20cdee5..f205331dcf 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1746,10 +1746,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
     VirtIONet *n = qemu_get_nic_opaque(nc);
     VirtIONetQueue *q = virtio_net_get_subqueue(nc);
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
+    VirtQueueElement *elems[VIRTQUEUE_MAX_SIZE];
+    size_t lens[VIRTQUEUE_MAX_SIZE];
     struct iovec mhdr_sg[VIRTQUEUE_MAX_SIZE];
     struct virtio_net_hdr_mrg_rxbuf mhdr;
     unsigned mhdr_cnt = 0;
-    size_t offset, i, guest_offset;
+    size_t offset, i, guest_offset, j;
+    ssize_t err;
 
     if (!virtio_net_can_receive(nc)) {
         return -1;
@@ -1780,6 +1783,12 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
 
         total = 0;
 
+        if (i == VIRTQUEUE_MAX_SIZE) {
+            virtio_error(vdev, "virtio-net unexpected long buffer chain");
+            err = size;
+            goto err;
+        }
+
         elem = virtqueue_pop(q->rx_vq, sizeof(VirtQueueElement));
         if (!elem) {
             if (i) {
@@ -1791,7 +1800,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
                              n->guest_hdr_len, n->host_hdr_len,
                              vdev->guest_features);
             }
-            return -1;
+            err = -1;
+            goto err;
         }
 
         if (elem->in_num < 1) {
@@ -1799,7 +1809,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
                          "virtio-net receive queue contains no in buffers");
             virtqueue_detach_element(q->rx_vq, elem, 0);
             g_free(elem);
-            return -1;
+            err = -1;
+            goto err;
         }
 
         sg = elem->in_sg;
@@ -1836,12 +1847,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
         if (!n->mergeable_rx_bufs && offset < size) {
             virtqueue_unpop(q->rx_vq, elem, total);
             g_free(elem);
-            return size;
+            err = size;
+            goto err;
         }
 
-        /* signal other side */
-        virtqueue_fill(q->rx_vq, elem, total, i++);
-        g_free(elem);
+        elems[i] = elem;
+        lens[i] = total;
+        i++;
     }
 
     if (mhdr_cnt) {
@@ -1851,10 +1863,23 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
                      &mhdr.num_buffers, sizeof mhdr.num_buffers);
     }
 
+    for (j = 0; j < i; j++) {
+        /* signal other side */
+        virtqueue_fill(q->rx_vq, elems[j], lens[j], j);
+        g_free(elems[j]);
+    }
+
     virtqueue_flush(q->rx_vq, i);
     virtio_notify(vdev, q->rx_vq);
 
     return size;
+
+err:
+    for (j = 0; j < i; j++) {
+        g_free(elems[j]);
+    }
+
+    return err;
 }
 
 static ssize_t virtio_net_do_receive(NetClientState *nc, const uint8_t *buf,
-- 
2.25.1

在 2021/9/2 下午1:44, Jason Wang 写道:
> When mergeable buffer is enabled, we try to set the num_buffers after
> the virtqueue elem has been unmapped. This will lead several issues,
> E.g a use after free when the descriptor has an address which belongs
> to the non direct access region. In this case we use bounce buffer
> that is allocated during address_space_map() and freed during
> address_space_unmap().
>
> Fixing this by storing the elems temporarily in an array and delay the
> unmap after we set the the num_buffers.
>
> This addresses CVE-2021-3748.
>
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Fixes: fbe78f4f55c6 ("virtio-net support")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>


Applied.

Thanks


> ---
>   hw/net/virtio-net.c | 39 ++++++++++++++++++++++++++++++++-------
>   1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 16d20cdee5..f205331dcf 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1746,10 +1746,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
>       VirtIONet *n = qemu_get_nic_opaque(nc);
>       VirtIONetQueue *q = virtio_net_get_subqueue(nc);
>       VirtIODevice *vdev = VIRTIO_DEVICE(n);
> +    VirtQueueElement *elems[VIRTQUEUE_MAX_SIZE];
> +    size_t lens[VIRTQUEUE_MAX_SIZE];
>       struct iovec mhdr_sg[VIRTQUEUE_MAX_SIZE];
>       struct virtio_net_hdr_mrg_rxbuf mhdr;
>       unsigned mhdr_cnt = 0;
> -    size_t offset, i, guest_offset;
> +    size_t offset, i, guest_offset, j;
> +    ssize_t err;
>   
>       if (!virtio_net_can_receive(nc)) {
>           return -1;
> @@ -1780,6 +1783,12 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
>   
>           total = 0;
>   
> +        if (i == VIRTQUEUE_MAX_SIZE) {
> +            virtio_error(vdev, "virtio-net unexpected long buffer chain");
> +            err = size;
> +            goto err;
> +        }
> +
>           elem = virtqueue_pop(q->rx_vq, sizeof(VirtQueueElement));
>           if (!elem) {
>               if (i) {
> @@ -1791,7 +1800,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
>                                n->guest_hdr_len, n->host_hdr_len,
>                                vdev->guest_features);
>               }
> -            return -1;
> +            err = -1;
> +            goto err;
>           }
>   
>           if (elem->in_num < 1) {
> @@ -1799,7 +1809,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
>                            "virtio-net receive queue contains no in buffers");
>               virtqueue_detach_element(q->rx_vq, elem, 0);
>               g_free(elem);
> -            return -1;
> +            err = -1;
> +            goto err;
>           }
>   
>           sg = elem->in_sg;
> @@ -1836,12 +1847,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
>           if (!n->mergeable_rx_bufs && offset < size) {
>               virtqueue_unpop(q->rx_vq, elem, total);
>               g_free(elem);
> -            return size;
> +            err = size;
> +            goto err;
>           }
>   
> -        /* signal other side */
> -        virtqueue_fill(q->rx_vq, elem, total, i++);
> -        g_free(elem);
> +        elems[i] = elem;
> +        lens[i] = total;
> +        i++;
>       }
>   
>       if (mhdr_cnt) {
> @@ -1851,10 +1863,23 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
>                        &mhdr.num_buffers, sizeof mhdr.num_buffers);
>       }
>   
> +    for (j = 0; j < i; j++) {
> +        /* signal other side */
> +        virtqueue_fill(q->rx_vq, elems[j], lens[j], j);
> +        g_free(elems[j]);
> +    }
> +
>       virtqueue_flush(q->rx_vq, i);
>       virtio_notify(vdev, q->rx_vq);
>   
>       return size;
> +
> +err:
> +    for (j = 0; j < i; j++) {
> +        g_free(elems[j]);
> +    }
> +
> +    return err;
>   }
>   
>   static ssize_t virtio_net_do_receive(NetClientState *nc, const uint8_t *buf,