Hi Robin, Nate, On 14/07/2016 12:36, Robin Murphy wrote: > On 14/07/16 09:34, Joerg Roedel wrote: >> On Wed, Jul 13, 2016 at 02:49:32PM -0400, Nate Watterson wrote: >>> Passing a NULL or uninitialized iova_domain into put_iova_domain >>> will currently crash the kernel when the unconfigured iova_domain >>> data members are accessed. To prevent this from occurring, this patch >>> adds a check to make sure that the domain is non-NULL and that the >>> domain granule is non-zero. The granule can be used to check if the >>> domain was properly initialized because calling init_iova_domain >>> with a granule of zero would have already triggered a BUG statement >>> crashing the kernel. >> >> Have you seen real crashes happening because of this?
I also saw the crash happening with my PCIe passthrough series (not upstreamed) [PATCH v10 0/8] [PATCH v10 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 1/3: iommu changes https://lkml.org/lkml/2016/6/7/676 patch [PATCH v10 8/8] iommu/arm-smmu: get/put the msi cookie also uses iommu_put_dma_cookie and the uninitialised lock crash happens if the group gets destroyed before the iommu_dma_init_domain is called, which can also happen for me. > > It _can_ happen via the iommu-dma code if something goes wrong > initialising a group - the IOVA domain gets allocated at the same time > as the default IOMMU domain, but isn't initialised until later once the > device in question gets ity dma ops set up. If adding the device to the > group fails, everything gets torn down again and iommu_put_dma_cookie() > ends up trying to take an uninitialised lock . Cant' we allow the granule check also with UNMANAGED type? Thanks Eric > > However, I think the appropriate fix for that particular situation would > be more like this: > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index ea5a9ebf0f78..d00d22930a6b 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -65,10 +65,11 @@ void iommu_put_dma_cookie(struct iommu_domain *domain) > { > struct iova_domain *iovad = domain->iova_cookie; > > - if (!iovad) > + if (domain->type != IOMMU_DOMAIN_DMA || !iovad) > return; > > - put_iova_domain(iovad); > + if (iovad->granule) > + put_iova_domain(iovad); > kfree(iovad); > domain->iova_cookie = NULL; > } > > (It probably should have been that way from the start; mea culpa) > > Robin. > _______________________________________________ > iommu mailing list > io...@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu >
