Re: [PATCH] hw/virtio: return NULL from qemu_get_virtqueue_element() on invalid state

[Laurent Vivier <lvivier@redhat.com>]

On 7/9/26 14:40, Michael S. Tsirkin wrote:
> On Thu, Jul 09, 2026 at 12:06:44PM +0200, Laurent Vivier wrote:
>> qemu_get_virtqueue_element() uses assert() to check that the in_num
>> and out_num fields deserialized from the migration stream do not
>> exceed VIRTQUEUE_MAX_SIZE. A crafted migration stream can set these
>> fields to invalid values, hitting the assertion and aborting the
>> destination QEMU process.
>>
>> Replace the assertions with a bounds check that returns NULL on
>> failure. Update all callers (virtio-serial-bus, virtio-blk,
>> virtio-scsi) to handle the NULL return and fail the migration
>> gracefully.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: 6bdc21c050a2 ("virtio: fix up max size checks")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3802
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   hw/block/virtio-blk.c       |  4 ++++
>>   hw/char/virtio-serial-bus.c |  3 +++
>>   hw/scsi/virtio-scsi.c       |  4 ++++
>>   hw/virtio/virtio.c          | 11 ++++-------
>>   4 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 6b92066aff4c..42c0f2553f18 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -1384,6 +1384,10 @@ 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/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
>> index c1973f0248fc..879df158608a 100644
>> --- a/hw/char/virtio-serial-bus.c
>> +++ b/hw/char/virtio-serial-bus.c
>> @@ -763,6 +763,9 @@ static int fetch_active_ports_list(QEMUFile *f,
>>   
>>               port->elem =
>>                   qemu_get_virtqueue_element(vdev, f, sizeof(VirtQueueElement));
>> +            if (!port->elem) {
>> +                return -EINVAL;
>> +            }
>>   
>>               /*
>>                *  Port was throttled on source machine.  Let's
>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>> index 6c7376801190..8dd0b88a30c4 100644
>> --- a/hw/scsi/virtio-scsi.c
>> +++ b/hw/scsi/virtio-scsi.c
>> @@ -274,6 +274,10 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
>>       assert(n < vs->conf.num_queues);
> 
> 
> I mean just above this we have this assert.

Sure, I can fix that too.

> 
> 
> If you are really want to be nicer about handling
> malformed input, pls do this systematically, not just
> at random places for no apparent reason.

I don't do this randomly for no apparent reason, see
https://gitlab.com/qemu-project/qemu/-/issues/3802

But I'm going to check if I can remove more assert() in the migration path.

Thanks,
Laurent

> 
> 
>>       req = qemu_get_virtqueue_element(vdev, f,
>>                                        sizeof(VirtIOSCSIReq) + vs->cdb_size);
>> +    if (!req) {
>> +        return NULL;
>> +    }
>> +
>>       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 f4d86a365530..7bccfdde33e7 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2167,13 +2167,10 @@ 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.
>> -     */
>> -    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)) {
>> +        return NULL;
>> +    }
>>   
>>       elem = virtqueue_alloc_element(sz, data.out_num, data.in_num);
>>       elem->index = data.index;
>> -- 
>> 2.54.0
>