hw/block/virtio-blk.c | 3 +++ hw/scsi/virtio-scsi.c | 5 +++++ hw/virtio/virtio.c | 19 +++++++++++++------ include/hw/virtio/virtio.h | 4 ++++ 4 files changed, 25 insertions(+), 6 deletions(-)
qemu_get_virtqueue_element() asserts on in_num/out_num read straight
from the migration stream, then feeds them to virtqueue_alloc_element()
as allocation sizes. A malformed stream from the source crashes the
destination; with NDEBUG the asserts vanish and the heap allocation is
driven by attacker-controlled values - the reason the in-tree TODO
forbade NDEBUG builds. The hole is reachable on every inbound migration
of a virtio-blk or virtio-scsi guest.
Reject in_num/out_num that overflow their arrays, and reject
in_num + out_num > VIRTQUEUE_MAX_SIZE (a single chain cannot exceed
one ring). The OR short-circuits so the sum is only evaluated once
both counts are individually bounded.
virtio_blk_load_device() returns -EINVAL through its existing int
return. virtio_scsi_load_request() has no error channel in its
SCSIBusInfo hook signature and mirrors the existing error_report() +
exit(1) used a few lines below for parse failures.
Document the new NULL return on the prototype in virtio.h.
Cc: qemu-stable@nongnu.org
Fixes: 8059feee0041 ("virtio: introduce virtio_map")
Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
hw/block/virtio-blk.c | 3 +++
hw/scsi/virtio-scsi.c | 5 +++++
hw/virtio/virtio.c | 19 +++++++++++++------
include/hw/virtio/virtio.h | 4 ++++
4 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9cb9f1fb2b..97482c7981 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1378,6 +1378,9 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
}
req = qemu_get_virtqueue_element(vdev, f, sizeof(VirtIOBlockReq));
+ if (!req) {
+ return -EINVAL;
+ }
virtio_blk_init_request(s, virtio_get_queue(vdev, vq_idx), req);
WITH_QEMU_LOCK_GUARD(&s->rq_lock) {
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 6c73768011..bb2412e89c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -274,6 +274,11 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
assert(n < vs->conf.num_queues);
req = qemu_get_virtqueue_element(vdev, f,
sizeof(VirtIOSCSIReq) + vs->cdb_size);
+ if (!req) {
+ error_report("virtio-scsi: failed to load request from migration "
+ "stream (queue=%u)", n);
+ exit(1);
+ }
virtio_scsi_init_req(s, vs->cmd_vqs[n], req);
if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 63e2faee99..24df1d0300 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2167,13 +2167,20 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
qemu_get_buffer(f, (uint8_t *)&data, sizeof(VirtQueueElementOld));
- /* TODO: teach all callers that this can fail, and return failure instead
- * of asserting here.
- * This is just one thing (there are probably more) that must be
- * fixed before we can allow NDEBUG compilation.
+ /*
+ * Bound the untrusted counts before they reach
+ * virtqueue_alloc_element() as allocation sizes. A single chain
+ * cannot exceed the ring size, so the sum is bounded too.
*/
- assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
- assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
+ if (data.in_num > ARRAY_SIZE(data.in_addr) ||
+ data.out_num > ARRAY_SIZE(data.out_addr) ||
+ data.in_num + data.out_num > VIRTQUEUE_MAX_SIZE) {
+ error_report("virtio: malformed VirtQueueElement on migration load: "
+ "in_num=%u out_num=%u (max=%u)",
+ data.in_num, data.out_num,
+ (unsigned)VIRTQUEUE_MAX_SIZE);
+ return NULL;
+ }
elem = virtqueue_alloc_element(sz, data.out_num, data.in_num);
elem->index = data.index;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 6344bd7b68..3034257505 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -281,6 +281,10 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
void virtqueue_map(VirtIODevice *vdev, VirtQueueElement *elem);
void *virtqueue_pop(VirtQueue *vq, size_t sz);
unsigned int virtqueue_drop_all(VirtQueue *vq);
+/**
+ * Returns NULL if the migration stream encodes an out-of-range
+ * descriptor count; callers must check before dereferencing.
+ */
void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz);
void qemu_put_virtqueue_element(VirtIODevice *vdev, QEMUFile *f,
VirtQueueElement *elem);
--
2.50.1 (Apple Git-155)On Thu, Jun 04, 2026 at 04:56:24PM +0800, Bin Guo wrote:
> qemu_get_virtqueue_element() asserts on in_num/out_num read straight
> from the migration stream, then feeds them to virtqueue_alloc_element()
> as allocation sizes. A malformed stream from the source crashes the
> destination; with NDEBUG the asserts vanish and the heap allocation is
> driven by attacker-controlled values - the reason the in-tree TODO
> forbade NDEBUG builds. The hole is reachable on every inbound migration
> of a virtio-blk or virtio-scsi guest.
>
> Reject in_num/out_num that overflow their arrays, and reject
> in_num + out_num > VIRTQUEUE_MAX_SIZE (a single chain cannot exceed
> one ring). The OR short-circuits so the sum is only evaluated once
> both counts are individually bounded.
>
> virtio_blk_load_device() returns -EINVAL through its existing int
> return. virtio_scsi_load_request() has no error channel in its
> SCSIBusInfo hook signature and mirrors the existing error_report() +
> exit(1) used a few lines below for parse failures.
>
> Document the new NULL return on the prototype in virtio.h.
>
> Cc: qemu-stable@nongnu.org
> Fixes: 8059feee0041 ("virtio: introduce virtio_map")
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
> hw/block/virtio-blk.c | 3 +++
> hw/scsi/virtio-scsi.c | 5 +++++
> hw/virtio/virtio.c | 19 +++++++++++++------
> include/hw/virtio/virtio.h | 4 ++++
> 4 files changed, 25 insertions(+), 6 deletions(-)
- The purpose of this patch is unclear. Are you working on a larger
scale effort to enable NDEBUG builds in QEMU? There are still lots of
asserts that validate migration state, so I'm not sure this patch
makes sense in isolation. Or is there some other reason you want to
make this change? Addressing the TODO comment just for the sake of
getting rid of a TODO is probably not worth it without a bigger
strategy.
- virtio-serial-bus.c uses qemu_get_virtqueue_element() without checking
for a NULL return value. While the code is written to handle
port->elem == NULL, the existing semantics are not intended for
silently ignored an invalid migration stream.
- Is virtio-blk's s->rq freed when virtio_blk_load_device() fails? The
code depends on virtio_blk_reset() being called before
virtio_blk_device_unrealize() - otherwise s->rq is leaked. If reset is
always called, then a comment in virtio_blk_load_device() should make
it that clear that s->rq is not leaked if an error is hit after
virtqueue elements have already been successfully loaded.
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 9cb9f1fb2b..97482c7981 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1378,6 +1378,9 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
> }
>
> req = qemu_get_virtqueue_element(vdev, f, sizeof(VirtIOBlockReq));
> + if (!req) {
> + return -EINVAL;
> + }
> virtio_blk_init_request(s, virtio_get_queue(vdev, vq_idx), req);
>
> WITH_QEMU_LOCK_GUARD(&s->rq_lock) {
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 6c73768011..bb2412e89c 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -274,6 +274,11 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
> assert(n < vs->conf.num_queues);
> req = qemu_get_virtqueue_element(vdev, f,
> sizeof(VirtIOSCSIReq) + vs->cdb_size);
> + if (!req) {
> + error_report("virtio-scsi: failed to load request from migration "
> + "stream (queue=%u)", n);
> + exit(1);
> + }
> virtio_scsi_init_req(s, vs->cmd_vqs[n], req);
>
> if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 63e2faee99..24df1d0300 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2167,13 +2167,20 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
>
> qemu_get_buffer(f, (uint8_t *)&data, sizeof(VirtQueueElementOld));
>
> - /* TODO: teach all callers that this can fail, and return failure instead
> - * of asserting here.
> - * This is just one thing (there are probably more) that must be
> - * fixed before we can allow NDEBUG compilation.
> + /*
> + * Bound the untrusted counts before they reach
> + * virtqueue_alloc_element() as allocation sizes. A single chain
> + * cannot exceed the ring size, so the sum is bounded too.
> */
> - assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
> - assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
> + if (data.in_num > ARRAY_SIZE(data.in_addr) ||
> + data.out_num > ARRAY_SIZE(data.out_addr) ||
> + data.in_num + data.out_num > VIRTQUEUE_MAX_SIZE) {
> + error_report("virtio: malformed VirtQueueElement on migration load: "
> + "in_num=%u out_num=%u (max=%u)",
> + data.in_num, data.out_num,
> + (unsigned)VIRTQUEUE_MAX_SIZE);
> + return NULL;
> + }
>
> elem = virtqueue_alloc_element(sz, data.out_num, data.in_num);
> elem->index = data.index;
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 6344bd7b68..3034257505 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -281,6 +281,10 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
> void virtqueue_map(VirtIODevice *vdev, VirtQueueElement *elem);
> void *virtqueue_pop(VirtQueue *vq, size_t sz);
> unsigned int virtqueue_drop_all(VirtQueue *vq);
> +/**
> + * Returns NULL if the migration stream encodes an out-of-range
> + * descriptor count; callers must check before dereferencing.
> + */
> void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz);
> void qemu_put_virtqueue_element(VirtIODevice *vdev, QEMUFile *f,
> VirtQueueElement *elem);
> --
> 2.50.1 (Apple Git-155)
>
© 2016 - 2026 Red Hat, Inc.