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

[Robin Murphy <robin.murphy@arm.com>]

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