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 <[email protected]>
> > Cc: Peter Maydell <[email protected]>
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > 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);
> > }
