[PATCH 1/2] iommu/dma: Don't put uninitialised IOVA domains

[PATCH 1/2] iommu/dma: Don't put uninitialised IOVA domains

[Robin Murphy]

Due to the limitations of having to wait until we see a device's DMA
restrictions before we know how we want an IOVA domain initialised,
there is a window for error if a DMA ops domain is allocated but later
freed without ever being used. In that case, init_iova_domain() was
never called, so calling put_iova_domain() from iommu_put_dma_cookie()
ends up trying to take an uninitialised lock and crashing.

Make things robust by skipping the call unless the IOVA domain actually
has been initialised, as we probably should have done from the start.

Fixes: 0db2e5d18f76 ("iommu: Implement common IOMMU ops for DMA mapping")
Cc: stable@vger.kernel.org
Reported-by: Nate Watterson <nwatters@codeaurora.org>
Reviewed-by: Nate Watterson <nwatters@codeaurora.org>
Tested-by: Nate Watterson <nwatters@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/dma-iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 08a1e2f3690f..7d991c81c4fa 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -68,7 +68,8 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
 	if (!iovad)
 		return;
 
-	put_iova_domain(iovad);
+	if (iovad->granule)
+		put_iova_domain(iovad);
 	kfree(iovad);
 	domain->iova_cookie = NULL;
 }
-- 
2.8.1.dirty

[PATCH 2/2] iommu/dma: Respect IOMMU aperture when allocating

[Robin Murphy]

Where a device driver has set a 64-bit DMA mask to indicate the absence
of addressing limitations, we still need to ensure that we don't
allocate IOVAs beyond the actual input size of the IOMMU. The reported
aperture is the most reliable way we have of inferring that input
address size, so use that to enforce a hard upper limit.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

This is the only other thing I currently have which could perhaps be
considered a fix; otherwise I'll pull it into the PCI/SMMU series.

 drivers/iommu/dma-iommu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 7d991c81c4fa..092d781f5f35 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -158,6 +158,8 @@ static struct iova *__alloc_iova(struct iova_domain *iovad, size_t size,
 	unsigned long shift = iova_shift(iovad);
 	unsigned long length = iova_align(iovad, size) >> shift;
 
+	if (domain->geometry.force_aperture)
+		dma_limit &= domain->geometry.aperture_end;
 	/*
 	 * Enforce size-alignment to be safe - there could perhaps be an
 	 * attribute to control this per-device, or at least per-domain...
-- 
2.8.1.dirty

Re: [PATCH 2/2] iommu/dma: Respect IOMMU aperture when allocating

[Joerg Roedel]

On Tue, Aug 09, 2016 at 04:23:18PM +0100, Robin Murphy wrote:
> Where a device driver has set a 64-bit DMA mask to indicate the absence
> of addressing limitations, we still need to ensure that we don't
> allocate IOVAs beyond the actual input size of the IOMMU. The reported
> aperture is the most reliable way we have of inferring that input
> address size, so use that to enforce a hard upper limit.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

Also missing 'Fixes:' line.

> ---
> 
> This is the only other thing I currently have which could perhaps be
> considered a fix; otherwise I'll pull it into the PCI/SMMU series.
> 
>  drivers/iommu/dma-iommu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 7d991c81c4fa..092d781f5f35 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -158,6 +158,8 @@ static struct iova *__alloc_iova(struct iova_domain *iovad, size_t size,
>  	unsigned long shift = iova_shift(iovad);
>  	unsigned long length = iova_align(iovad, size) >> shift;
>  
> +	if (domain->geometry.force_aperture)
> +		dma_limit &= domain->geometry.aperture_end;

This might work, but is misleading as limits and aperture_end are
neither a mask nor a bitfield. It is more descriptive to use min()
here:

	dma_limit = min(dma_limit, omain->geometry.aperture_end);

Re: [PATCH 2/2] iommu/dma: Respect IOMMU aperture when allocating

[Robin Murphy]

On 09/08/16 16:36, Joerg Roedel wrote:
> On Tue, Aug 09, 2016 at 04:23:18PM +0100, Robin Murphy wrote:
>> Where a device driver has set a 64-bit DMA mask to indicate the absence
>> of addressing limitations, we still need to ensure that we don't
>> allocate IOVAs beyond the actual input size of the IOMMU. The reported
>> aperture is the most reliable way we have of inferring that input
>> address size, so use that to enforce a hard upper limit.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> 
> Also missing 'Fixes:' line.
> 
>> ---
>>
>> This is the only other thing I currently have which could perhaps be
>> considered a fix; otherwise I'll pull it into the PCI/SMMU series.
>>
>>  drivers/iommu/dma-iommu.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 7d991c81c4fa..092d781f5f35 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -158,6 +158,8 @@ static struct iova *__alloc_iova(struct iova_domain *iovad, size_t size,
>>  	unsigned long shift = iova_shift(iovad);
>>  	unsigned long length = iova_align(iovad, size) >> shift;
>>  
>> +	if (domain->geometry.force_aperture)
>> +		dma_limit &= domain->geometry.aperture_end;
> 
> This might work, but is misleading as limits and aperture_end are
> neither a mask nor a bitfield. It is more descriptive to use min()
> here:
> 
> 	dma_limit = min(dma_limit, omain->geometry.aperture_end);
> 

Well, dma_limit is always a DMA mask, but I suppose you're right that an
IOMMU driver could in theory set a wacky non-power-of-two aperture if it
really wanted to. I'll respin...

Thanks,
Robin.

[PATCH v2] iommu/dma: Respect IOMMU aperture when allocating

[Robin Murphy]

Where a device driver has set a 64-bit DMA mask to indicate the absence
of addressing limitations, we still need to ensure that we don't
allocate IOVAs beyond the actual input size of the IOMMU. The reported
aperture is the most reliable way we have of inferring that input
address size, so use that to enforce a hard upper limit where available.

Fixes: 0db2e5d18f76 ("iommu: Implement common IOMMU ops for DMA mapping")
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

Now with bonus "not being completely broken" as well as the min()
change - seems the necessary prototype changes somehow wound up rebased
into a different patch :/

 drivers/iommu/dma-iommu.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 7d991c81c4fa..00c8a08d56e7 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -152,12 +152,15 @@ int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
 	}
 }
 
-static struct iova *__alloc_iova(struct iova_domain *iovad, size_t size,
+static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 		dma_addr_t dma_limit)
 {
+	struct iova_domain *iovad = domain->iova_cookie;
 	unsigned long shift = iova_shift(iovad);
 	unsigned long length = iova_align(iovad, size) >> shift;
 
+	if (domain->geometry.force_aperture)
+		dma_limit = min(dma_limit, domain->geometry.aperture_end);
 	/*
 	 * Enforce size-alignment to be safe - there could perhaps be an
 	 * attribute to control this per-device, or at least per-domain...
@@ -315,7 +318,7 @@ struct page **iommu_dma_alloc(struct device *dev, size_t size, gfp_t gfp,
 	if (!pages)
 		return NULL;
 
-	iova = __alloc_iova(iovad, size, dev->coherent_dma_mask);
+	iova = __alloc_iova(domain, size, dev->coherent_dma_mask);
 	if (!iova)
 		goto out_free_pages;
 
@@ -387,7 +390,7 @@ dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 	phys_addr_t phys = page_to_phys(page) + offset;
 	size_t iova_off = iova_offset(iovad, phys);
 	size_t len = iova_align(iovad, size + iova_off);
-	struct iova *iova = __alloc_iova(iovad, len, dma_get_mask(dev));
+	struct iova *iova = __alloc_iova(domain, len, dma_get_mask(dev));
 
 	if (!iova)
 		return DMA_ERROR_CODE;
@@ -539,7 +542,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 		prev = s;
 	}
 
-	iova = __alloc_iova(iovad, iova_len, dma_get_mask(dev));
+	iova = __alloc_iova(domain, iova_len, dma_get_mask(dev));
 	if (!iova)
 		goto out_restore_sg;
 
-- 
2.8.1.dirty

Re: [PATCH v2] iommu/dma: Respect IOMMU aperture when allocating

[Joerg Roedel]

On Tue, Aug 09, 2016 at 05:31:35PM +0100, Robin Murphy wrote:
> Where a device driver has set a 64-bit DMA mask to indicate the absence
> of addressing limitations, we still need to ensure that we don't
> allocate IOVAs beyond the actual input size of the IOMMU. The reported
> aperture is the most reliable way we have of inferring that input
> address size, so use that to enforce a hard upper limit where available.
> 
> Fixes: 0db2e5d18f76 ("iommu: Implement common IOMMU ops for DMA mapping")
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> Now with bonus "not being completely broken" as well as the min()
> change - seems the necessary prototype changes somehow wound up rebased
> into a different patch :/
> 
>  drivers/iommu/dma-iommu.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Applied, thanks.

Re: [PATCH 1/2] iommu/dma: Don't put uninitialised IOVA domains

[Joerg Roedel]

On Tue, Aug 09, 2016 at 04:23:17PM +0100, Robin Murphy wrote:
> Due to the limitations of having to wait until we see a device's DMA
> restrictions before we know how we want an IOVA domain initialised,
> there is a window for error if a DMA ops domain is allocated but later
> freed without ever being used. In that case, init_iova_domain() was
> never called, so calling put_iova_domain() from iommu_put_dma_cookie()
> ends up trying to take an uninitialised lock and crashing.
> 
> Make things robust by skipping the call unless the IOVA domain actually
> has been initialised, as we probably should have done from the start.
> 
> Fixes: 0db2e5d18f76 ("iommu: Implement common IOMMU ops for DMA mapping")
> Cc: stable@vger.kernel.org
> Reported-by: Nate Watterson <nwatters@codeaurora.org>
> Reviewed-by: Nate Watterson <nwatters@codeaurora.org>
> Tested-by: Nate Watterson <nwatters@codeaurora.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/dma-iommu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Wow, that was quick :)

Applied to iommu/fixes, thanks.