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

[nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

On 2016-07-14 07:21, Auger Eric wrote:
> 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?

In my case, it was calling iommu_request_dm_for_dev() which triggered 
the
"iommu_[get/put]_dma_cookie() without iommu_dma_init_domain()" issue 
that
has Robin documented below.

> 
> 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)

I originally put together a similar patch, but then thought that people 
would
complain it didn't fix the root of the problem. Yet another instance 
where
thinking was best avoided I guess.

>> 
>> Robin.
>> _______________________________________________
>> iommu mailing list
>> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>> 

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.