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 <[email protected]>
>> ---
>> 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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
