On Mon, 14 Jul 2025 at 11:18, Stefano Garzarella <sgarz...@redhat.com> wrote:
>
> On Thu, Jul 10, 2025 at 12:40:43PM -0400, Michael S. Tsirkin wrote:
> >On Thu, Jul 10, 2025 at 04:46:34PM +0100, Peter Maydell wrote:
> >> Hi; Coverity complains about a potential filedescriptor leak in
> >> net/vhost-vdpa.c:net_init_vhost_vdpa(). This is CID 1490785.
> >>
> >> Specifically, in this function we do:
> >> queue_pairs = vhost_vdpa_get_max_queue_pairs(vdpa_device_fd, features,
> >> &has_cvq, errp);
> >> if (queue_pairs < 0) {
> >> [exit with failure]
> >> }
> >> ...
> >> ncs = g_malloc0(sizeof(*ncs) * queue_pairs);
> >> for (i = 0; i < queue_pairs; i++) {
> >> ...
> >> ncs[i] = net_vhost_vdpa_init(..., vdpa_device_fd, ...)
> >> ...
> >> }
> >> if (has_cvq) {
> >> ...
> >> nc = net_host_vdpa_init(..., vdpa_device_fd, ...)
> >> ...
> >> }
> >>
> >> So if queue_pairs is zero we will malloc(0) which seems dubious;
> >> and if queue_pairs is zero and has_cvq is false then the init
> >> function will exit success without ever calling net_vhost_vdpa_init()
> >> and it will leak the vdpa_device_fd.
> >>
> >> My guess is that queue_pairs == 0 should be an error, or possibly
> >> that (queue_pairs == 0 && !has_cvq) should be an error.
> >>
> >> Could somebody who knows more about this code tell me which, and
> >> perhaps produce a patch to make it handle that case?
> >
> >Historically queue_pairs == 0 was always same as 1, IIRC.
>
> Yep, also looking at vhost_vdpa_get_max_queue_pairs() it returns 1 if
> VIRTIO_NET_F_MQ is not negotiated, or what the device expose in the
> config space in the `max_virtqueue_pairs` field.
>
> In the spec we have:
> The device MUST set max_virtqueue_pairs to between 1 and 0x8000
> inclusive, if it offers VIRTIO_NET_F_MQ.
>
> So, IMHO we can just change the error check in this way:
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 58d738945d..8f39e5a983 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -1813,7 +1813,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const
> char *name,
>
> queue_pairs = vhost_vdpa_get_max_queue_pairs(vdpa_device_fd, features,
> &has_cvq, errp);
> - if (queue_pairs < 0) {
> + if (queue_pairs <= 0) {
> qemu_close(vdpa_device_fd);
> return queue_pairs;
> }
>
> I'll send a patch if no one complain.
Patch sent:
https://lore.kernel.org/qemu-devel/20250714101156.30024-1-sgarz...@redhat.com
Thanks,
Stefano
