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

[Nicolin Chen]

On Mon, Sep 12, 2022 at 02:38:52PM +0200, Cornelia Huck wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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.

The caller does check rc all the time. So I made a smaller fix
(the first option). Attaching the git-diff for the second one.

Alex, please let me know which one you prefer. Thanks!

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 51b2e05c76..74431411ab 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -2109,6 +2109,7 @@ static int vfio_connect_container(VFIOGroup *group, 
AddressSpace *as,
     case VFIO_TYPE1_IOMMU:
     {
         struct vfio_iommu_type1_info *info;
+        uint64_t iova_pgsizes;
 
         /*
          * FIXME: This assumes that a Type1 IOMMU can map any 64-bit
@@ -2119,20 +2120,22 @@ static int vfio_connect_container(VFIOGroup *group, 
AddressSpace *as,
          */
         ret = vfio_get_iommu_info(container, &info);
 
-        if (ret || !(info->flags & VFIO_IOMMU_INFO_PGSIZES)) {
+        if (info && (info->flags & VFIO_IOMMU_INFO_PGSIZES)) {
+            iova_pgsizes = info->iova_pgsizes;
+        } else {
             /* Assume 4k IOVA page size */
-            info->iova_pgsizes = 4096;
+            iova_pgsizes = 4096;
         }
-        vfio_host_win_add(container, 0, (hwaddr)-1, info->iova_pgsizes);
-        container->pgsizes = info->iova_pgsizes;
+        vfio_host_win_add(container, 0, (hwaddr)-1, iova_pgsizes);
+        container->pgsizes = iova_pgsizes;
 
         /* The default in the kernel ("dma_entry_limit") is 65535. */
         container->dma_max_mappings = 65535;
-        if (!ret) {
+        if (info) {
             vfio_get_info_dma_avail(info, &container->dma_max_mappings);
             vfio_get_iommu_info_migration(container, info);
+            g_free(info);
         }
-        g_free(info);
         break;
     }
     case VFIO_SPAPR_TCE_v2_IOMMU: