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

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 14, 2025 at 11:18:51AM +0200, Stefano Garzarella 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.
> 
> > 
> > > 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


vhost kind of includes all vhost things. Not an issue if it's
in both places, I think.