Re: [PATCH] iommu/iova: validate iova_domain input to put_iova_domain

[Auger Eric]

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
> iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>