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

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

[Michael S. Tsirkin]

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

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

[Michael Tokarev]

On 7/24/26 14:58, Michael S. Tsirkin wrote:
> 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.

What happens here with an attempt to migrate to an earlier
version of qemu, without this patch applied, but with non-
default values of vring.num?

Maybe we should send the values still, but always validate
them on load?

Also, what should we do here on older (stable) releases?
It raises the same question, how about migrating back to
a version without this change?

Thanks,

/mjt

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

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

[Michael S. Tsirkin]

On Tue, Jul 28, 2026 at 11:55:41AM +0300, Michael Tokarev wrote:
> On 7/24/26 14:58, Michael S. Tsirkin wrote:
> > 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.
> 
> What happens here with an attempt to migrate to an earlier
> version of qemu, without this patch applied, but with non-
> default values of vring.num?


It just works.

> Maybe we should send the values still, but always validate
> them on load?
>
> Also, what should we do here on older (stable) releases?
> It raises the same question, how about migrating back to
> a version without this change?
> 
> Thanks,
> 
> /mjt

We should never have sent them. Skipping the value works fine.

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