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.
