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: [email protected] Fixes: 8059feee0041 ("virtio: introduce virtio_map") Signed-off-by: Bin Guo <[email protected]> --- 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)
