[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <333f4b4f52168db68250a447ef5db3f3@codeaurora.org>
Date: Thu, 14 Jul 2016 13:55:32 -0400
From: nwatters@...eaurora.org
To: Auger Eric <eric.auger@...hat.com>
Cc: Robin Murphy <robin.murphy@....com>,
Joerg Roedel <joro@...tes.org>,
iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu/iova: validate iova_domain input to put_iova_domain
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@...ts.linux-foundation.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.
Powered by blists - more mailing lists