lkml.org 
[lkml]   [2016]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] iommu/iova: validate iova_domain input to put_iova_domain
From
Date

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

\
 
 \ /
  Last update: 2016-07-14 13:41    [W:0.039 / U:0.732 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site