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