Re: [PATCH] iommu/dma: Don't touch invalid iova_domain members

[Auger Eric]

Hi,

On 05/05/2017 12:04, Robin Murphy wrote:
> When __iommu_dma_map() and iommu_dma_free_iova() are called from
> iommu_dma_get_msi_page(), various iova_*() helpers are still invoked in
> the process, whcih is unwise since they access a different member of the
> union (the iova_domain) from that which was last written, and there's no
> guarantee that sensible values will result anyway.
> 
> CLean up the code paths that are valid for an MSI cookie to ensure we
> only do iova_domain-specific things when we're actually dealing with one.
> 
> Reported-by: Nate Watterson 
> Tested-by: Shanker Donthineni 
> Tested-by: Bharat Bhushan 
> Signed-off-by: Robin Murphy 
Tested-by: Eric Auger 

Thanks

Eric

> ---
> 
> I've taken the liberty of upgrading the prose testing confirmations
> into tested-by tags, hope that's OK.
> 
> Joerg; I'm happy to resend this after -rc1 with a fixes tag if you'd
> rather - I'm just throwing it out now for the sake of catching up with
> things.
> 
> Robin.
> 
>  drivers/iommu/dma-iommu.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 8348f366ddd1..62618e77bedc 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct iommu_dma_cookie 
> *cookie,
>   dma_addr_t iova, size_t size)
>  {
>   struct iova_domain *iovad = >iovad;
> - unsigned long shift = iova_shift(iovad);
>  
>   /* The MSI case is only ever cleaning up its most recent allocation */
>   if (cookie->type == IOMMU_DMA_MSI_COOKIE)
>   cookie->msi_iova -= size;
>   else
> - free_iova_fast(iovad, iova >> shift, size >> shift);
> + free_iova_fast(iovad, iova_pfn(iovad, iova),
> + size >> iova_shift(iovad));
>  }
>  
>  static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t 
> dma_addr,
> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device *dev, 
> phys_addr_t phys,
>  {
>   struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>   struct iommu_dma_cookie *cookie = domain->iova_cookie;
> - struct iova_domain *iovad = >iovad;
> - size_t iova_off = iova_offset(iovad, phys);
> + size_t iova_off = 0;
>   dma_addr_t iova;
>  
> - size = iova_align(iovad, size + iova_off);
> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
> + iova_off = iova_offset(>iovad, phys);
> + size = iova_align(>iovad, size + iova_off);
> + }
> +
>   iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
>   if (!iova)
>   return DMA_ERROR_CODE;
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] iommu/dma: Don't touch invalid iova_domain members

[Robin Murphy]

When __iommu_dma_map() and iommu_dma_free_iova() are called from
iommu_dma_get_msi_page(), various iova_*() helpers are still invoked in
the process, whcih is unwise since they access a different member of the
union (the iova_domain) from that which was last written, and there's no
guarantee that sensible values will result anyway.

CLean up the code paths that are valid for an MSI cookie to ensure we
only do iova_domain-specific things when we're actually dealing with one.

Reported-by: Nate Watterson 
Tested-by: Shanker Donthineni 
Tested-by: Bharat Bhushan 
Signed-off-by: Robin Murphy 
---

I've taken the liberty of upgrading the prose testing confirmations
into tested-by tags, hope that's OK.

Joerg; I'm happy to resend this after -rc1 with a fixes tag if you'd
rather - I'm just throwing it out now for the sake of catching up with
things.

Robin.

 drivers/iommu/dma-iommu.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 8348f366ddd1..62618e77bedc 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct iommu_dma_cookie 
*cookie,
dma_addr_t iova, size_t size)
 {
struct iova_domain *iovad = >iovad;
-   unsigned long shift = iova_shift(iovad);
 
/* The MSI case is only ever cleaning up its most recent allocation */
if (cookie->type == IOMMU_DMA_MSI_COOKIE)
cookie->msi_iova -= size;
else
-   free_iova_fast(iovad, iova >> shift, size >> shift);
+   free_iova_fast(iovad, iova_pfn(iovad, iova),
+   size >> iova_shift(iovad));
 }
 
 static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr,
@@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device *dev, 
phys_addr_t phys,
 {
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
struct iommu_dma_cookie *cookie = domain->iova_cookie;
-   struct iova_domain *iovad = >iovad;
-   size_t iova_off = iova_offset(iovad, phys);
+   size_t iova_off = 0;
dma_addr_t iova;
 
-   size = iova_align(iovad, size + iova_off);
+   if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
+   iova_off = iova_offset(>iovad, phys);
+   size = iova_align(>iovad, size + iova_off);
+   }
+
iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
if (!iova)
return DMA_ERROR_CODE;
-- 
2.11.0.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu