* [PATCH] hw/virtio: return NULL from qemu_get_virtqueue_element() on invalid state
@ 2026-07-09 10:06 Laurent Vivier
2026-07-09 12:40 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2026-07-09 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Hanna Reitz, Amit Shah, Laurent Vivier,
Michael S. Tsirkin, Stefan Hajnoczi, Fam Zheng, Kevin Wolf,
Marc-André Lureau, Paolo Bonzini, qemu-stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/virtio: return NULL from qemu_get_virtqueue_element() on invalid state
2026-07-09 10:06 [PATCH] hw/virtio: return NULL from qemu_get_virtqueue_element() on invalid state Laurent Vivier
@ 2026-07-09 12:40 ` Michael S. Tsirkin
2026-07-09 13:24 ` Laurent Vivier
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2026-07-09 12:40 UTC (permalink / raw)
To: Laurent Vivier
Cc: qemu-devel, qemu-block, Hanna Reitz, Amit Shah, Stefan Hajnoczi,
Fam Zheng, Kevin Wolf, Marc-André Lureau, Paolo Bonzini,
qemu-stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/virtio: return NULL from qemu_get_virtqueue_element() on invalid state
2026-07-09 12:40 ` Michael S. Tsirkin
@ 2026-07-09 13:24 ` Laurent Vivier
2026-07-09 14:15 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2026-07-09 13:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, qemu-block, Hanna Reitz, Amit Shah, Stefan Hajnoczi,
Fam Zheng, Kevin Wolf, Marc-André Lureau, Paolo Bonzini,
qemu-stable
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/virtio: return NULL from qemu_get_virtqueue_element() on invalid state
2026-07-09 13:24 ` Laurent Vivier
@ 2026-07-09 14:15 ` Michael S. Tsirkin
2026-07-09 15:01 ` Laurent Vivier
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2026-07-09 14:15 UTC (permalink / raw)
To: Laurent Vivier
Cc: qemu-devel, qemu-block, Hanna Reitz, Amit Shah, Stefan Hajnoczi,
Fam Zheng, Kevin Wolf, Marc-André Lureau, Paolo Bonzini,
qemu-stable
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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/virtio: return NULL from qemu_get_virtqueue_element() on invalid state
2026-07-09 14:15 ` Michael S. Tsirkin
@ 2026-07-09 15:01 ` Laurent Vivier
2026-07-09 15:14 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2026-07-09 15:01 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, qemu-block, Hanna Reitz, Amit Shah, Stefan Hajnoczi,
Fam Zheng, Kevin Wolf, Marc-André Lureau, Paolo Bonzini,
qemu-stable
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
>>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/virtio: return NULL from qemu_get_virtqueue_element() on invalid state
2026-07-09 15:01 ` Laurent Vivier
@ 2026-07-09 15:14 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2026-07-09 15:14 UTC (permalink / raw)
To: Laurent Vivier
Cc: qemu-devel, qemu-block, Hanna Reitz, Amit Shah, Stefan Hajnoczi,
Fam Zheng, Kevin Wolf, Marc-André Lureau, Paolo Bonzini,
qemu-stable
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
> > > >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-07-09 15:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-09 10:06 [PATCH] hw/virtio: return NULL from qemu_get_virtqueue_element() on invalid state Laurent Vivier
2026-07-09 12:40 ` Michael S. Tsirkin
2026-07-09 13:24 ` Laurent Vivier
2026-07-09 14:15 ` Michael S. Tsirkin
2026-07-09 15:01 ` Laurent Vivier
2026-07-09 15:14 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.