Re: vhost-vdpa potential fd leak (coverity issue)

[Stefano Garzarella]

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.


Q: should this file be listed in the "vhost" subcategory of 
MAINTAINERS?
At the moment it only gets caught by "Network device backends".

Maybe yes, but it's really virtio-net specific.
@Michael WDYT?

Thanks,
Stefano