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

[PATCH] 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.

Reported-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

I'm not sure this warrants a cc stable, as with the code currently in
mainline it's only at all likely if other things have already failed
elsewhere in a manner they should not be expected to.

 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 ea5a9ebf0f78..97a23082e18a 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

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

[Auger Eric]

Hi,
On 27/07/2016 17:46, 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.
> 
> Reported-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> I'm not sure this warrants a cc stable, as with the code currently in
> mainline it's only at all likely if other things have already failed
> elsewhere in a manner they should not be expected to.
> 
>  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 ea5a9ebf0f78..97a23082e18a 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;
>  }
> 
Reviewed-by: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Tested-by: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Thanks

Eric

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

[nwatters-sgV2jX0FEOL9JmXXK+q4OQ]

On 2016-07-27 12:00, Auger Eric wrote:
> Hi,
> On 27/07/2016 17:46, 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.
>> 
>> Reported-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>> 
>> I'm not sure this warrants a cc stable, as with the code currently in
>> mainline it's only at all likely if other things have already failed
>> elsewhere in a manner they should not be expected to.
>> 
>>  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 ea5a9ebf0f78..97a23082e18a 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;
>>  }
>> 
> Reviewed-by: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Tested-by: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Thanks
> 
> Eric
> 
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Reviewed-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Tested-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

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

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

[Joerg Roedel]

On Wed, Jul 27, 2016 at 04:46:06PM +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.
>

Missing 'Fixes:' and probably 'Cc: stable' lines?

> Reported-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> I'm not sure this warrants a cc stable, as with the code currently in
> mainline it's only at all likely if other things have already failed
> elsewhere in a manner they should not be expected to.

Yes, I think this qualifies for stable. Please re-send with the Acks and
Reviewed-by lines too. I'll queue this in my fixes branch and send it
upstream asap.


	Joerg

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

[Robin Murphy]

On 09/08/16 16:01, Joerg Roedel wrote:
> On Wed, Jul 27, 2016 at 04:46:06PM +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.
>>
> 
> Missing 'Fixes:' and probably 'Cc: stable' lines?
> 
>> Reported-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> I'm not sure this warrants a cc stable, as with the code currently in
>> mainline it's only at all likely if other things have already failed
>> elsewhere in a manner they should not be expected to.
> 
> Yes, I think this qualifies for stable. Please re-send with the Acks and
> Reviewed-by lines too. I'll queue this in my fixes branch and send it
> upstream asap.

Will do, thanks!

Robin.

> 
> 
> 	Joerg
>