[PATCH] [dma-coherent] Fix integer overflow in the reserved-memory dma allocation

[PATCH] [dma-coherent] Fix integer overflow in the reserved-memory dma allocation

[Kevin Grandemange]

pageno is an int and the PAGE_SHIFT shift is done on an int,
overflowing if the memory is bigger than 2G

This can be reproduced using for example a reserved-memory of 4G

reserved-memory {
		    #address-cells = <2>;
		    #size-cells = <2>;
		    ranges;

		    reserved_dma: buffer@0 {
		        compatible = "shared-dma-pool";
		        no-map;
		        reg = <0x5 0x00000000 0x4 0x0>;
        };
};

Signed-off-by: Kevin Grandemange <kevin.grandemange@allegrodvt.com>
---
 kernel/dma/coherent.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
index 551b0eb7028a..c20c6c0635b7 100644
--- a/kernel/dma/coherent.c
+++ b/kernel/dma/coherent.c
@@ -130,6 +130,7 @@ static void *__dma_alloc_from_coherent(struct device *dev,
 	int order = get_order(size);
 	unsigned long flags;
 	int pageno;
+	ssize_t mem_offset;
 	void *ret;
 
 	spin_lock_irqsave(&mem->spinlock, flags);
@@ -144,8 +145,9 @@ static void *__dma_alloc_from_coherent(struct device *dev,
 	/*
 	 * Memory was found in the coherent area.
 	 */
-	*dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT);
-	ret = mem->virt_base + (pageno << PAGE_SHIFT);
+	mem_offset = (ssize_t)pageno << PAGE_SHIFT;
+	*dma_handle = dma_get_device_base(dev, mem) + mem_offset;
+	ret = mem->virt_base + mem_offset;
 	spin_unlock_irqrestore(&mem->spinlock, flags);
 	memset(ret, 0, size);
 	return ret;
-- 
2.20.1

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

Re: [PATCH] [dma-coherent] Fix integer overflow in the reserved-memory dma allocation

[Robin Murphy]

On 09/03/2020 11:02 am, Kevin Grandemange wrote:
> pageno is an int and the PAGE_SHIFT shift is done on an int,
> overflowing if the memory is bigger than 2G
> 
> This can be reproduced using for example a reserved-memory of 4G

Nit: the example shows 16GB, not 4.

> reserved-memory {
> 		    #address-cells = <2>;
> 		    #size-cells = <2>;
> 		    ranges;
> 
> 		    reserved_dma: buffer@0 {
> 		        compatible = "shared-dma-pool";
> 		        no-map;
> 		        reg = <0x5 0x00000000 0x4 0x0>;
>          };
> };
> 
> Signed-off-by: Kevin Grandemange <kevin.grandemange@allegrodvt.com>
> ---
>   kernel/dma/coherent.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
> index 551b0eb7028a..c20c6c0635b7 100644
> --- a/kernel/dma/coherent.c
> +++ b/kernel/dma/coherent.c
> @@ -130,6 +130,7 @@ static void *__dma_alloc_from_coherent(struct device *dev,
>   	int order = get_order(size);
>   	unsigned long flags;
>   	int pageno;
> +	ssize_t mem_offset;
>   	void *ret;
>   
>   	spin_lock_irqsave(&mem->spinlock, flags);
> @@ -144,8 +145,9 @@ static void *__dma_alloc_from_coherent(struct device *dev,
>   	/*
>   	 * Memory was found in the coherent area.
>   	 */
> -	*dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT);
> -	ret = mem->virt_base + (pageno << PAGE_SHIFT);
> +	mem_offset = (ssize_t)pageno << PAGE_SHIFT;

Isn't this still capable of overflowing for 32-bit architectures where 
ssize_t is most likely to be long and LONG_MAX == INT_MAX (before we 
even get to PAE and friends)? Logically, dma_addr_t would be the most 
appropriate type here.

FWIW since you have to have an explicit cast either way, I don't see 
much point in introducing the local variable vs. just adding the cast 
into the existing expression.

Thanks,
Robin.

> +	*dma_handle = dma_get_device_base(dev, mem) + mem_offset;
> +	ret = mem->virt_base + mem_offset;
>   	spin_unlock_irqrestore(&mem->spinlock, flags);
>   	memset(ret, 0, size);
>   	return ret;
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] [dma-coherent] Fix integer overflow in the reserved-memory dma allocation

[Kevin Grandemange]

On 3/9/20 2:56 PM, Robin Murphy wrote:

> On 09/03/2020 11:02 am, Kevin Grandemange wrote:
>> pageno is an int and the PAGE_SHIFT shift is done on an int,
>> overflowing if the memory is bigger than 2G
>>
>> This can be reproduced using for example a reserved-memory of 4G
>
> Nit: the example shows 16GB, not 4.
>
Will fix. My usecase is an external memory that can span 16GB / 4GB depending on the configuration.
>> reserved-memory {
>>             #address-cells = <2>;
>>             #size-cells = <2>;
>>             ranges;
>>
>>             reserved_dma: buffer@0 {
>>                 compatible = "shared-dma-pool";
>>                 no-map;
>>                 reg = <0x5 0x00000000 0x4 0x0>;
>>          };
>> };
>>
>> Signed-off-by: Kevin Grandemange <kevin.grandemange@allegrodvt.com>
>> ---
>>   kernel/dma/coherent.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
>> index 551b0eb7028a..c20c6c0635b7 100644
>> --- a/kernel/dma/coherent.c
>> +++ b/kernel/dma/coherent.c
>> @@ -130,6 +130,7 @@ static void *__dma_alloc_from_coherent(struct device *dev,
>>       int order = get_order(size);
>>       unsigned long flags;
>>       int pageno;
>> +    ssize_t mem_offset;
>>       void *ret;
>>         spin_lock_irqsave(&mem->spinlock, flags);
>> @@ -144,8 +145,9 @@ static void *__dma_alloc_from_coherent(struct device *dev,
>>       /*
>>        * Memory was found in the coherent area.
>>        */
>> -    *dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT);
>> -    ret = mem->virt_base + (pageno << PAGE_SHIFT);
>> +    mem_offset = (ssize_t)pageno << PAGE_SHIFT;
>
> Isn't this still capable of overflowing for 32-bit architectures where ssize_t is most likely to be long and LONG_MAX == INT_MAX (before we even get to PAE and friends)? Logically, dma_addr_t would be the most appropriate type here.

You are right, I was only thinking in my usecase (arm64) and wrongly assuming ssize_t had the same properties as size_t.

I will change that to dma_addr_t.

>
> FWIW since you have to have an explicit cast either way, I don't see much point in introducing the local variable vs. just adding the cast into the existing expression.
The local variable was just for checkpatch.pl line length check, I agree with you, it doesn't add much.
>
> Thanks,
> Robin.
>
>> +    *dma_handle = dma_get_device_base(dev, mem) + mem_offset;
>> +    ret = mem->virt_base + mem_offset;
>>       spin_unlock_irqrestore(&mem->spinlock, flags);
>>       memset(ret, 0, size);
>>       return ret;
>>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2] [dma-coherent] Fix integer overflow in the reserved-memory dma allocation

[Kevin Grandemange]

pageno is an int and the PAGE_SHIFT shift is done on an int,
overflowing if the memory is bigger than 2G

This can be reproduced using for example a reserved-memory of 4G

reserved-memory {
		    #address-cells = <2>;
		    #size-cells = <2>;
		    ranges;

		    reserved_dma: buffer@0 {
		        compatible = "shared-dma-pool";
		        no-map;
		        reg = <0x5 0x00000000 0x1 0x0>;
        };
};

Signed-off-by: Kevin Grandemange <kevin.grandemange@allegrodvt.com>
---
Changes v1 -> v2:
  - removed mem_offset tmp variable
  - use dma_addr_t instead of ssize_t
  - Fix reserved-memory size in the dts example

 kernel/dma/coherent.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
index 551b0eb7028a..b8e4fa441493 100644
--- a/kernel/dma/coherent.c
+++ b/kernel/dma/coherent.c
@@ -144,8 +144,8 @@ static void *__dma_alloc_from_coherent(struct device *dev,
 	/*
 	 * Memory was found in the coherent area.
 	 */
-	*dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT);
-	ret = mem->virt_base + (pageno << PAGE_SHIFT);
+	*dma_handle = dma_get_device_base(dev, mem) + ((dma_addr_t)pageno << PAGE_SHIFT);
+	ret = mem->virt_base + ((dma_addr_t)pageno << PAGE_SHIFT);
 	spin_unlock_irqrestore(&mem->spinlock, flags);
 	memset(ret, 0, size);
 	return ret;
-- 
2.20.1

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

[PATCH v3] [dma-coherent] Fix integer overflow in the reserved-memory dma allocation

[Kevin Grandemange]

pageno is an int and the PAGE_SHIFT shift is done on an int,
overflowing if the memory is bigger than 2G

This can be reproduced using for example a reserved-memory of 4G

reserved-memory {
		    #address-cells = <2>;
		    #size-cells = <2>;
		    ranges;

		    reserved_dma: buffer@0 {
		        compatible = "shared-dma-pool";
		        no-map;
		        reg = <0x5 0x00000000 0x1 0x0>;
        };
};

Signed-off-by: Kevin Grandemange <kevin.grandemange@allegrodvt.com>
---

Changes v1 -> v2:
  - removed mem_offset tmp variable
  - use dma_addr_t instead of ssize_t
  - Fix reserved-memory size in the dts example

Changes v2 -> v3:
  - Fix several other site where PAGE_SHIFT shifts are done on ints.

 kernel/dma/coherent.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
index 551b0eb7028a..d322cb786e7e 100644
--- a/kernel/dma/coherent.c
+++ b/kernel/dma/coherent.c
@@ -134,7 +134,7 @@ static void *__dma_alloc_from_coherent(struct device *dev,
 
 	spin_lock_irqsave(&mem->spinlock, flags);
 
-	if (unlikely(size > (mem->size << PAGE_SHIFT)))
+	if (unlikely(size > ((dma_addr_t)mem->size << PAGE_SHIFT)))
 		goto err;
 
 	pageno = bitmap_find_free_region(mem->bitmap, mem->size, order);
@@ -144,8 +144,8 @@ static void *__dma_alloc_from_coherent(struct device *dev,
 	/*
 	 * Memory was found in the coherent area.
 	 */
-	*dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT);
-	ret = mem->virt_base + (pageno << PAGE_SHIFT);
+	*dma_handle = dma_get_device_base(dev, mem) + ((dma_addr_t)pageno << PAGE_SHIFT);
+	ret = mem->virt_base + ((dma_addr_t)pageno << PAGE_SHIFT);
 	spin_unlock_irqrestore(&mem->spinlock, flags);
 	memset(ret, 0, size);
 	return ret;
@@ -194,7 +194,7 @@ static int __dma_release_from_coherent(struct dma_coherent_mem *mem,
 				       int order, void *vaddr)
 {
 	if (mem && vaddr >= mem->virt_base && vaddr <
-		   (mem->virt_base + (mem->size << PAGE_SHIFT))) {
+		   (mem->virt_base + ((dma_addr_t)mem->size << PAGE_SHIFT))) {
 		int page = (vaddr - mem->virt_base) >> PAGE_SHIFT;
 		unsigned long flags;
 
@@ -238,7 +238,7 @@ static int __dma_mmap_from_coherent(struct dma_coherent_mem *mem,
 		struct vm_area_struct *vma, void *vaddr, size_t size, int *ret)
 {
 	if (mem && vaddr >= mem->virt_base && vaddr + size <=
-		   (mem->virt_base + (mem->size << PAGE_SHIFT))) {
+		   (mem->virt_base + ((dma_addr_t)mem->size << PAGE_SHIFT))) {
 		unsigned long off = vma->vm_pgoff;
 		int start = (vaddr - mem->virt_base) >> PAGE_SHIFT;
 		int user_count = vma_pages(vma);
@@ -248,7 +248,7 @@ static int __dma_mmap_from_coherent(struct dma_coherent_mem *mem,
 		if (off < count && user_count <= count - off) {
 			unsigned long pfn = mem->pfn_base + start + off;
 			*ret = remap_pfn_range(vma, vma->vm_start, pfn,
-					       user_count << PAGE_SHIFT,
+					       (unsigned long)user_count << PAGE_SHIFT,
 					       vma->vm_page_prot);
 		}
 		return 1;
-- 
2.20.1

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

Re: [PATCH v3] [dma-coherent] Fix integer overflow in the reserved-memory dma allocation

[Christoph Hellwig]

Thanks,

applied to the dma-mapping tree for 5.7 with minor codingstyle fixes.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu