[PATCH] virtio: stop migrating num_default, validate vring.num on load

Michael S. Tsirkin posted 1 patch 1 day, 21 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/3e6a7c403f93acc37af6a6332fdc65049ae218fb.1784894327.git.mst@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
hw/virtio/virtio.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
[PATCH] virtio: stop migrating num_default, validate vring.num on load
Posted by Michael S. Tsirkin 1 day, 21 hours ago
num_default tracks the allocation size of used_elems, set by
virtio_add_queue(). Migrating it via the ringsize subsection is
wrong: a migration stream (malicious or simply from a different
configuration) can inflate num_default so that
virtio_queue_set_num() accepts oversized values, leading to OOB
access on the used_elems array.

It is not even migrated consistently: a configuration with a
smaller num_default could thinkably migrate and work but in the
common case of num == num_default the value is not actually sent.

Stop migrating num_default: make virtio_ringsize_needed() return
false so the subsection is never sent, and use VMSTATE_UNUSED to
consume the field from old streams without applying it. The
destination keeps its local num_default from virtio_add_queue(),
which matches the actual allocation.

Also validate vring.num against num_default when loading the core
virtio state, rejecting streams that supply a queue size larger
than the locally allocated maximum.

Fixes: 46c5d0823d ("virtio: ring sizes vs. reset")
Fixes: 50e5ae4dc3 ("migration/virtio: Remove simple .get/.put use")
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 94ddbfd09f..437c001a04 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2817,14 +2817,6 @@ static bool virtio_packed_virtqueue_needed(void *opaque)
 
 static bool virtio_ringsize_needed(void *opaque)
 {
-    VirtIODevice *vdev = opaque;
-    int i;
-
-    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-        if (vdev->vq[i].vring.num != vdev->vq[i].vring.num_default) {
-            return true;
-        }
-    }
     return false;
 }
 
@@ -2913,7 +2905,7 @@ static const VMStateDescription vmstate_ringsize = {
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (const VMStateField[]) {
-        VMSTATE_UINT32(vring.num_default, struct VirtQueue),
+        VMSTATE_UNUSED(sizeof(uint32_t)),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -3593,6 +3585,12 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
 
     for (i = 0; i < num; i++) {
         vdev->vq[i].vring.num = qemu_get_be32(f);
+        if (vdev->vq[i].vring.num > vdev->vq[i].vring.num_default) {
+            error_report("VQ %d vring.num %u exceeds allocated max %u",
+                         i, vdev->vq[i].vring.num,
+                         vdev->vq[i].vring.num_default);
+            return -1;
+        }
         if (k->has_variable_vring_alignment) {
             vdev->vq[i].vring.align = qemu_get_be32(f);
         }
-- 
MST