[PATCH] arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops

[PATCH] arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops

[Marek Szyprowski]

Patch 22b3c181c6c324a46f71aae806d8ddbe61d25761 ("arm: dma-mapping: limit
IOMMU mapping size") added a check for IO address space size. However
this patch broke IOMMU initialization for typical platforms initialized
from device tree, which get the default IO address space size of 4GiB.
This value doesn't fit into size_t and fails a check introduced by that
commit resulting in failed dma-mapping/iommu initialization. This patch
fixes this issue by adding proper support for full 4GiB address space
size.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/include/asm/dma-iommu.h | 2 +-
 arch/arm/mm/dma-mapping.c        | 9 +++------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
index 8e3fcb924db6..2ef282f96651 100644
--- a/arch/arm/include/asm/dma-iommu.h
+++ b/arch/arm/include/asm/dma-iommu.h
@@ -25,7 +25,7 @@ struct dma_iommu_mapping {
 };
 
 struct dma_iommu_mapping *
-arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size);
+arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size);
 
 void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
 
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 09c5fe3d30c2..b43b762eebc1 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1878,7 +1878,7 @@ struct dma_map_ops iommu_coherent_ops = {
  * arm_iommu_attach_device function.
  */
 struct dma_iommu_mapping *
-arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
+arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size)
 {
 	unsigned int bits = size >> PAGE_SHIFT;
 	unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);
@@ -2057,11 +2057,8 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!iommu)
 		return false;
 
-	/*
-	 * currently arm_iommu_create_mapping() takes a max of size_t
-	 * for size param. So check this limit for now.
-	 */
-	if (size > SIZE_MAX)
+	/* Only 32-bit DMA address space is supported for now */
+	if (size > DMA_BIT_MASK(32) + 1)
 		return false;
 
 	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
-- 
1.9.2

[PATCH] arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops

[Robin Murphy]

Hi Marek,

On 28/04/15 10:08, Marek Szyprowski wrote:
> Patch 22b3c181c6c324a46f71aae806d8ddbe61d25761 ("arm: dma-mapping: limit
> IOMMU mapping size") added a check for IO address space size. However
> this patch broke IOMMU initialization for typical platforms initialized
> from device tree, which get the default IO address space size of 4GiB.
> This value doesn't fit into size_t and fails a check introduced by that
> commit resulting in failed dma-mapping/iommu initialization. This patch
> fixes this issue by adding proper support for full 4GiB address space
> size.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Much nicer than my hack of just passing in size-1, thanks! I'd offer a 
tested-by for the default 32-bit case, however...

> ---
>   arch/arm/include/asm/dma-iommu.h | 2 +-
>   arch/arm/mm/dma-mapping.c        | 9 +++------
>   2 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
> index 8e3fcb924db6..2ef282f96651 100644
> --- a/arch/arm/include/asm/dma-iommu.h
> +++ b/arch/arm/include/asm/dma-iommu.h
> @@ -25,7 +25,7 @@ struct dma_iommu_mapping {
>   };
>
>   struct dma_iommu_mapping *
> -arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size);
> +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size);
>
>   void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 09c5fe3d30c2..b43b762eebc1 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1878,7 +1878,7 @@ struct dma_map_ops iommu_coherent_ops = {
>    * arm_iommu_attach_device function.
>    */
>   struct dma_iommu_mapping *
> -arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
> +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size)
>   {
>   	unsigned int bits = size >> PAGE_SHIFT;

...doesn't this u64->int conversion now have the potential to truncate 
(if the device has a large enough DMA mask specified) and end up 
generating a weirdly small mapping rather than failing outright? 
Admittedly you'd have to have a 44-bit or larger DMA mask, and I'm not 
sure what the practical likelihood of seeing that even on LPAE systems 
is, but still I'm a little uneasy about it.

Robin.

>   	unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);
> @@ -2057,11 +2057,8 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
>   	if (!iommu)
>   		return false;
>
> -	/*
> -	 * currently arm_iommu_create_mapping() takes a max of size_t
> -	 * for size param. So check this limit for now.
> -	 */
> -	if (size > SIZE_MAX)
> +	/* Only 32-bit DMA address space is supported for now */
> +	if (size > DMA_BIT_MASK(32) + 1)
>   		return false;
>
>   	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
>

[PATCH] arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops

[Robin Murphy]

On 28/04/15 15:52, Robin Murphy wrote:
> Hi Marek,
>
> On 28/04/15 10:08, Marek Szyprowski wrote:
>> Patch 22b3c181c6c324a46f71aae806d8ddbe61d25761 ("arm: dma-mapping: limit
>> IOMMU mapping size") added a check for IO address space size. However
>> this patch broke IOMMU initialization for typical platforms initialized
>> from device tree, which get the default IO address space size of 4GiB.
>> This value doesn't fit into size_t and fails a check introduced by that
>> commit resulting in failed dma-mapping/iommu initialization. This patch
>> fixes this issue by adding proper support for full 4GiB address space
>> size.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> Much nicer than my hack of just passing in size-1, thanks! I'd offer a
> tested-by for the default 32-bit case, however...
>
>> ---
>>    arch/arm/include/asm/dma-iommu.h | 2 +-
>>    arch/arm/mm/dma-mapping.c        | 9 +++------
>>    2 files changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
>> index 8e3fcb924db6..2ef282f96651 100644
>> --- a/arch/arm/include/asm/dma-iommu.h
>> +++ b/arch/arm/include/asm/dma-iommu.h
>> @@ -25,7 +25,7 @@ struct dma_iommu_mapping {
>>    };
>>
>>    struct dma_iommu_mapping *
>> -arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size);
>> +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size);
>>
>>    void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index 09c5fe3d30c2..b43b762eebc1 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -1878,7 +1878,7 @@ struct dma_map_ops iommu_coherent_ops = {
>>     * arm_iommu_attach_device function.
>>     */
>>    struct dma_iommu_mapping *
>> -arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
>> +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size)
>>    {
>>    	unsigned int bits = size >> PAGE_SHIFT;
>
> ...doesn't this u64->int conversion now have the potential to truncate
> (if the device has a large enough DMA mask specified) and end up
> generating a weirdly small mapping rather than failing outright?
> Admittedly you'd have to have a 44-bit or larger DMA mask, and I'm not
> sure what the practical likelihood of seeing that even on LPAE systems
> is, but still I'm a little uneasy about it.

...by which I mean we should move the current size check into 
arm_iommu_create_mapping itself, rather than relying on the caller, 
because looking at dependent bits of code in isolation can be confusing ;)

Robin.

>>    	unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);
>> @@ -2057,11 +2057,8 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>    	if (!iommu)
>>    		return false;
>>
>> -	/*
>> -	 * currently arm_iommu_create_mapping() takes a max of size_t
>> -	 * for size param. So check this limit for now.
>> -	 */
>> -	if (size > SIZE_MAX)
>> +	/* Only 32-bit DMA address space is supported for now */
>> +	if (size > DMA_BIT_MASK(32) + 1)
>>    		return false;
>>
>>    	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
>>
>
> _______________________________________________
> iommu mailing list
> iommu at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

[PATCH] arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops

[Marek Szyprowski]

Hello,

On 2015-04-28 17:10, Robin Murphy wrote:
> On 28/04/15 15:52, Robin Murphy wrote:
>> Hi Marek,
>>
>> On 28/04/15 10:08, Marek Szyprowski wrote:
>>> Patch 22b3c181c6c324a46f71aae806d8ddbe61d25761 ("arm: dma-mapping: 
>>> limit
>>> IOMMU mapping size") added a check for IO address space size. However
>>> this patch broke IOMMU initialization for typical platforms initialized
>>> from device tree, which get the default IO address space size of 4GiB.
>>> This value doesn't fit into size_t and fails a check introduced by that
>>> commit resulting in failed dma-mapping/iommu initialization. This patch
>>> fixes this issue by adding proper support for full 4GiB address space
>>> size.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> Much nicer than my hack of just passing in size-1, thanks! I'd offer a
>> tested-by for the default 32-bit case, however...
>>
>>> ---
>>>    arch/arm/include/asm/dma-iommu.h | 2 +-
>>>    arch/arm/mm/dma-mapping.c        | 9 +++------
>>>    2 files changed, 4 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/dma-iommu.h 
>>> b/arch/arm/include/asm/dma-iommu.h
>>> index 8e3fcb924db6..2ef282f96651 100644
>>> --- a/arch/arm/include/asm/dma-iommu.h
>>> +++ b/arch/arm/include/asm/dma-iommu.h
>>> @@ -25,7 +25,7 @@ struct dma_iommu_mapping {
>>>    };
>>>
>>>    struct dma_iommu_mapping *
>>> -arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, 
>>> size_t size);
>>> +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 
>>> size);
>>>
>>>    void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
>>>
>>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>>> index 09c5fe3d30c2..b43b762eebc1 100644
>>> --- a/arch/arm/mm/dma-mapping.c
>>> +++ b/arch/arm/mm/dma-mapping.c
>>> @@ -1878,7 +1878,7 @@ struct dma_map_ops iommu_coherent_ops = {
>>>     * arm_iommu_attach_device function.
>>>     */
>>>    struct dma_iommu_mapping *
>>> -arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, 
>>> size_t size)
>>> +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 
>>> size)
>>>    {
>>>        unsigned int bits = size >> PAGE_SHIFT;
>>
>> ...doesn't this u64->int conversion now have the potential to truncate
>> (if the device has a large enough DMA mask specified) and end up
>> generating a weirdly small mapping rather than failing outright?
>> Admittedly you'd have to have a 44-bit or larger DMA mask, and I'm not
>> sure what the practical likelihood of seeing that even on LPAE systems
>> is, but still I'm a little uneasy about it.
>
> ...by which I mean we should move the current size check into 
> arm_iommu_create_mapping itself, rather than relying on the caller, 
> because looking at dependent bits of code in isolation can be 
> confusing ;)

Okay, no problem. I will move this check to arm_iommu_create_mapping() 
and the
problem of potential truncating will be solved. This fix is really 
needed for
v4.1-rcX. For next releases this will be probably rewritten for common 
code with
arm64 anyway.

>
> Robin.
>
>>>        unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);
>>> @@ -2057,11 +2057,8 @@ static bool arm_setup_iommu_dma_ops(struct 
>>> device *dev, u64 dma_base, u64 size,
>>>        if (!iommu)
>>>            return false;
>>>
>>> -    /*
>>> -     * currently arm_iommu_create_mapping() takes a max of size_t
>>> -     * for size param. So check this limit for now.
>>> -     */
>>> -    if (size > SIZE_MAX)
>>> +    /* Only 32-bit DMA address space is supported for now */
>>> +    if (size > DMA_BIT_MASK(32) + 1)
>>>            return false;
>>>
>>>        mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
>>>
>>
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH] arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops

[Marek Szyprowski]

Patch 22b3c181c6c324a46f71aae806d8ddbe61d25761 ("arm: dma-mapping: limit
IOMMU mapping size") added a check for IO address space size. However
this patch broke IOMMU initialization for typical platforms initialized
from device tree, which get the default IO address space size of 4GiB.
This value doesn't fit into size_t and fails a check introduced by that
commit resulting in failed dma-mapping/iommu initialization. This patch
fixes this issue by adding proper support for full 4GiB address space
size.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Will Deacon <will.deacon@arm.com>
---
Changelog:
v3:
- fixed typo and changed error code to ERANGE

v2:
- moved check from arm_setup_iommu_dma_ops() to arm_iommu_create_mapping()

v1: http://www.spinics.net/lists/arm-kernel/msg414483.html
---
 arch/arm/include/asm/dma-iommu.h |  2 +-
 arch/arm/mm/dma-mapping.c        | 13 +++++--------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
index 8e3fcb924db6..2ef282f96651 100644
--- a/arch/arm/include/asm/dma-iommu.h
+++ b/arch/arm/include/asm/dma-iommu.h
@@ -25,7 +25,7 @@ struct dma_iommu_mapping {
 };
 
 struct dma_iommu_mapping *
-arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size);
+arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size);
 
 void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
 
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 09c5fe3d30c2..7e7583ddd607 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1878,7 +1878,7 @@ struct dma_map_ops iommu_coherent_ops = {
  * arm_iommu_attach_device function.
  */
 struct dma_iommu_mapping *
-arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
+arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size)
 {
 	unsigned int bits = size >> PAGE_SHIFT;
 	unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);
@@ -1886,6 +1886,10 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
 	int extensions = 1;
 	int err = -ENOMEM;
 
+	/* currently only 32-bit DMA address space is supported */
+	if (size > DMA_BIT_MASK(32) + 1)
+		return ERR_PTR(-ERANGE);
+
 	if (!bitmap_size)
 		return ERR_PTR(-EINVAL);
 
@@ -2057,13 +2061,6 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!iommu)
 		return false;
 
-	/*
-	 * currently arm_iommu_create_mapping() takes a max of size_t
-	 * for size param. So check this limit for now.
-	 */
-	if (size > SIZE_MAX)
-		return false;
-
 	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
 	if (IS_ERR(mapping)) {
 		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
-- 
1.9.2