Re: [PATCH] vfio/common: Do not g_free in vfio_get_iommu_info

[Cornelia Huck <cohuck@redhat.com>]

On Fri, Sep 09 2022, Nicolin Chen <nicolinc@nvidia.com> wrote:

> Its caller vfio_connect_container() assigns a default value
> to info->iova_pgsizes, even if vfio_get_iommu_info() fails.
> This would result in a "Segmentation fault" error, when the
> VFIO_IOMMU_GET_INFO ioctl errors out.
>
> Since the caller has g_free already, drop the g_free in its
> rollback routine and add a line of comments to highlight it.

There's basically two ways to fix this:

- return *info in any case, even on error
- free *info on error, and make sure that the caller doesn't try to
  access *info if the function returned !0

The problem with the first option is that the caller will access invalid
information if it neglects to check the return code, and that might lead
to not-that-obvious errors; in the second case, a broken caller would at
least fail quickly with a segfault. The current code is easier to fix
with the first option.

I think I'd prefer the second option; but obviously maintainer's choice.

>
> Fixes: 87ea529c50 ("vfio: Get migration capability flags for container")
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  hw/vfio/common.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index ace9562a9b..51b2e05c76 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1940,6 +1940,7 @@ static int vfio_init_container(VFIOContainer *container, int group_fd,
>      return 0;
>  }
>  
> +/* The caller is responsible for g_free(*info) */
>  static int vfio_get_iommu_info(VFIOContainer *container,
>                                 struct vfio_iommu_type1_info **info)
>  {
> @@ -1951,8 +1952,6 @@ again:
>      (*info)->argsz = argsz;
>  
>      if (ioctl(container->fd, VFIO_IOMMU_GET_INFO, *info)) {
> -        g_free(*info);
> -        *info = NULL;
>          return -errno;
>      }
>