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

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

[Laurent Vivier]

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

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

[Michael S. Tsirkin]

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.


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.


>      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

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

[Laurent Vivier]

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
> 

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

[Michael S. Tsirkin]

On Thu, Jul 09, 2026 at 03:24:31PM +0200, Laurent Vivier wrote:
> 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

I am not blaming you, sorry if it sounded like it.
But this is not coming from any customers (how can it)
the fact that someone decided to fuzz our migration
stream and triggered a specific assert *is* random and it does not
mean we will now go and try to focus on the specific things
that triggered in the specific test.


> 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
> > 

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

[Laurent Vivier]

On 7/9/26 16:15, Michael S. Tsirkin wrote:
> On Thu, Jul 09, 2026 at 03:24:31PM +0200, Laurent Vivier wrote:
>> 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
> 
> I am not blaming you, sorry if it sounded like it.
> But this is not coming from any customers (how can it)
> the fact that someone decided to fuzz our migration
> stream and triggered a specific assert *is* random and it does not
> mean we will now go and try to focus on the specific things
> that triggered in the specific test.

I agree, but I fix them to avoid to have hundreds of duplicates in the future. And I do as 
simple as possible to avoid to spend too much time on them.

Thanks,
Laurent

> 
> 
>> 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
>>>
> 

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

[Michael S. Tsirkin]

On Thu, Jul 09, 2026 at 05:01:04PM +0200, Laurent Vivier wrote:
> On 7/9/26 16:15, Michael S. Tsirkin wrote:
> > On Thu, Jul 09, 2026 at 03:24:31PM +0200, Laurent Vivier wrote:
> > > 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
> > 
> > I am not blaming you, sorry if it sounded like it.
> > But this is not coming from any customers (how can it)
> > the fact that someone decided to fuzz our migration
> > stream and triggered a specific assert *is* random and it does not
> > mean we will now go and try to focus on the specific things
> > that triggered in the specific test.
> 
> I agree, but I fix them to avoid to have hundreds of duplicates in the
> future.

whoever reports this in the future  will get exactly the same
response. either fix them all in some reasonably
defined subsystem or we are not bothering.

> And I do as simple as possible to avoid to spend too much time on
> them.
> 
> Thanks,
> Laurent
> 
> > 
> > 
> > > 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
> > > > 
> >