[PATCH] virtio: make virtio_add_queue() queue_size an unsigned int

Stefan Hajnoczi posted 1 patch 1 month, 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260730153604.1462722-1-stefanha@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
include/hw/virtio/virtio.h | 2 +-
hw/virtio/virtio.c         | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
[PATCH] virtio: make virtio_add_queue() queue_size an unsigned int
Posted by Stefan Hajnoczi 1 month, 4 weeks ago
virtio_add_queue()'s queue_size argument is a signed int. Coverity is
unhappy when that type is used as an argument to g_new0():

  *** CID 1664271:         Error handling issues  (NEGATIVE_RETURNS)
  /builds/qemu-project/qemu/hw/virtio/virtio.c: 2595             in virtio_add_queue()
  2589         }
  2590
  2591         vdev->vq[i].vring.num = queue_size;
  2592         vdev->vq[i].vring.num_default = queue_size;
  2593         vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
  2594         vdev->vq[i].handle_output = handle_output;
  >>>     CID 1664271:         Error handling issues  (NEGATIVE_RETURNS)
  >>>     "__n" is passed to a parameter that cannot be negative.
  2595         vdev->vq[i].used_elems = g_new0(VirtQueueElement, queue_size);

The vdev->vq[i].vring.num and num_default fields are already declared as
unsigned int, so change the virtio_add_queue() argument's type for
consistency.

A note on consistency: the VIRTIO specification defines queue size as an
unsigned 16-bit value. QEMU's device models variously use uint16_t,
uint32_t, and other unsigned types for queue size qdev properties.
virtio_add_queue() limits queue size to the much smaller
VIRTQUEUE_MAX_SIZE (1024) constant, so the different widths don't really
matter.

I have checked that all callers of virtio_add_queue() pass an unsigned
queue size.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio.h | 2 +-
 hw/virtio/virtio.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index c99cb19d886..ff7f837fb92 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -302,7 +302,7 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
 
 typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
 
-VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
+VirtQueue *virtio_add_queue(VirtIODevice *vdev, unsigned int queue_size,
                             VirtIOHandleOutput handle_output);
 
 void virtio_del_queue(VirtIODevice *vdev, int n);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index daa5607338c..e1210f024c6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2564,7 +2564,7 @@ void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector)
     }
 }
 
-VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
+VirtQueue *virtio_add_queue(VirtIODevice *vdev, unsigned int queue_size,
                             VirtIOHandleOutput handle_output)
 {
     int i;
-- 
2.55.0
Re: [PATCH] virtio: make virtio_add_queue() queue_size an unsigned int
Posted by Peter Maydell 1 month, 4 weeks ago
On Thu, 30 Jul 2026 at 16:36, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> virtio_add_queue()'s queue_size argument is a signed int. Coverity is
> unhappy when that type is used as an argument to g_new0():
>
>   *** CID 1664271:         Error handling issues  (NEGATIVE_RETURNS)
>   /builds/qemu-project/qemu/hw/virtio/virtio.c: 2595             in virtio_add_queue()
>   2589         }
>   2590
>   2591         vdev->vq[i].vring.num = queue_size;
>   2592         vdev->vq[i].vring.num_default = queue_size;
>   2593         vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
>   2594         vdev->vq[i].handle_output = handle_output;
>   >>>     CID 1664271:         Error handling issues  (NEGATIVE_RETURNS)
>   >>>     "__n" is passed to a parameter that cannot be negative.
>   2595         vdev->vq[i].used_elems = g_new0(VirtQueueElement, queue_size);

Though note that the reason Coverity thinks this value can be negative
is not because the function might be passed a negative value, but
because it incorrectly thinks that
        int override = object_property_get_int(OBJECT(qbus->parent),
                                               VIRTIO_QUEUE_SIZE_OVERRIDE,
                                               &error_abort);

can return a negative number. (It can't because of the error_abort
and because the property is a UINT16 one.)

It might be a good idea to enforce the VIRTQUEUE_MAX_SIZE bounds
check on the queue size we get from the override property. Since
I think that can be set by the end-user (though probably it won't
be in practice) ideally we would not abort() on a bad property
value, but that's awkward to do at this point...

thanks
-- PMM
Re: [PATCH] virtio: make virtio_add_queue() queue_size an unsigned int
Posted by Stefan Hajnoczi 1 month, 4 weeks ago
On Thu, Jul 30, 2026 at 12:02 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 30 Jul 2026 at 16:36, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > virtio_add_queue()'s queue_size argument is a signed int. Coverity is
> > unhappy when that type is used as an argument to g_new0():
> >
> >   *** CID 1664271:         Error handling issues  (NEGATIVE_RETURNS)
> >   /builds/qemu-project/qemu/hw/virtio/virtio.c: 2595             in virtio_add_queue()
> >   2589         }
> >   2590
> >   2591         vdev->vq[i].vring.num = queue_size;
> >   2592         vdev->vq[i].vring.num_default = queue_size;
> >   2593         vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
> >   2594         vdev->vq[i].handle_output = handle_output;
> >   >>>     CID 1664271:         Error handling issues  (NEGATIVE_RETURNS)
> >   >>>     "__n" is passed to a parameter that cannot be negative.
> >   2595         vdev->vq[i].used_elems = g_new0(VirtQueueElement, queue_size);
>
> Though note that the reason Coverity thinks this value can be negative
> is not because the function might be passed a negative value, but
> because it incorrectly thinks that
>         int override = object_property_get_int(OBJECT(qbus->parent),
>                                                VIRTIO_QUEUE_SIZE_OVERRIDE,
>                                                &error_abort);
>
> can return a negative number. (It can't because of the error_abort
> and because the property is a UINT16 one.)
>
> It might be a good idea to enforce the VIRTQUEUE_MAX_SIZE bounds
> check on the queue size we get from the override property. Since
> I think that can be set by the end-user (though probably it won't
> be in practice) ideally we would not abort() on a bad property
> value, but that's awkward to do at this point...

I'll send a v2 that addresses VIRTIO_QUEUE_SIZE_OVERRIDE.

Stefan