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 <nicol...@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: