Recently hit an issue on an attempt to mmap allocated DMA memory to the
user space. It isn't the case for any device, but only if dma_mmap_attrs
call will use the iommu_dma_mmap() function as a backend.
If the kernel address (cpu_addr) passed to the iommu_dma_mmap function
is the same as returned by allocator (dma_alloc_coherent) then memory
will be mapped correctly. But if the address passed is inside the
allocated region, then the mapping in the user space will still refer
to the start of the region and not to the middle of it.

The reason for it is the dma_common_find_pages() call, which returns the
very first page of the vm structure regardless of the cpu_addr passed.

The idea for the fix is to return not the first page in the vm
structure, but the page related to the requested cpu_addr.

Signed-off-by: Dima Stepanov <dstepanov.src@gmail.com>
---
 kernel/dma/remap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/dma/remap.c b/kernel/dma/remap.c
index 9e2afad1c615..238fbf2821a6 100644
--- a/kernel/dma/remap.c
+++ b/kernel/dma/remap.c
@@ -9,12 +9,15 @@
 struct page **dma_common_find_pages(void *cpu_addr)
 {
     struct vm_struct *area = find_vm_area(cpu_addr);
+    int i;

     if (!area || !(area->flags & VM_DMA_COHERENT))
         return NULL;
     WARN(area->flags != VM_DMA_COHERENT,
          "unexpected flags in area: %p\n", cpu_addr);
-    return area->pages;
+    i = (PAGE_ALIGN((uintptr_t)cpu_addr) - (uintptr_t)area->addr) >>
PAGE_SHIFT;
+
+    return &area->pages[i];
 }

 /*
-- 
2.43.0

On 2025-01-26 9:09 am, Dima Stepanov wrote:
> Recently hit an issue on an attempt to mmap allocated DMA memory to the
> user space. It isn't the case for any device, but only if dma_mmap_attrs
> call will use the iommu_dma_mmap() function as a backend.
> If the kernel address (cpu_addr) passed to the iommu_dma_mmap function
> is the same as returned by allocator (dma_alloc_coherent) then memory
> will be mapped correctly. But if the address passed is inside the
> allocated region, then the mapping in the user space will still refer
> to the start of the region and not to the middle of it.
> 
> The reason for it is the dma_common_find_pages() call, which returns the
> very first page of the vm structure regardless of the cpu_addr passed.
> 
> The idea for the fix is to return not the first page in the vm
> structure, but the page related to the requested cpu_addr.

No, that's a bug in the caller of dma_mmap_attrs(). As the kerneldoc 
says, cpu_addr/dma_addr/size must still represent the whole buffer as 
returned by the allocator - any offset for the mapping itself relative 
to the start of the buffer is expressed in vma->pgoff.

Thanks,
Robin.

> Signed-off-by: Dima Stepanov <dstepanov.src@gmail.com>
> ---
>   kernel/dma/remap.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/remap.c b/kernel/dma/remap.c
> index 9e2afad1c615..238fbf2821a6 100644
> --- a/kernel/dma/remap.c
> +++ b/kernel/dma/remap.c
> @@ -9,12 +9,15 @@
>   struct page **dma_common_find_pages(void *cpu_addr)
>   {
>       struct vm_struct *area = find_vm_area(cpu_addr);
> +    int i;
> 
>       if (!area || !(area->flags & VM_DMA_COHERENT))
>           return NULL;
>       WARN(area->flags != VM_DMA_COHERENT,
>            "unexpected flags in area: %p\n", cpu_addr);
> -    return area->pages;
> +    i = (PAGE_ALIGN((uintptr_t)cpu_addr) - (uintptr_t)area->addr) >>
> PAGE_SHIFT;
> +
> +    return &area->pages[i];
>   }
> 
>   /*

On Mon, Jan 27, 2025 at 12:50 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> No, that's a bug in the caller of dma_mmap_attrs(). As the kerneldoc
> says, cpu_addr/dma_addr/size must still represent the whole buffer as
> returned by the allocator - any offset for the mapping itself relative
> to the start of the buffer is expressed in vma->pgoff.
>
> Thanks,
> Robin.

I see, thanks for clarification Robin. I was confused, because depending
on the backend it will work or not work. But i believe that in this case it is
undefined behavior. That is unfortunate to me, since the idea behind was:
- The memory allocated once because of device/firmware
- Different users could request a part of this memory from the kernel and
mmap it

And i didn't want to expose this offset information to the user. Wanted to
rely on the kernel to mmap the proper part of the buffer.

Thanks,
Dima.

On 27/01/2025 5:47 pm, Dima Stepanov wrote:
> On Mon, Jan 27, 2025 at 12:50 PM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> No, that's a bug in the caller of dma_mmap_attrs(). As the kerneldoc
>> says, cpu_addr/dma_addr/size must still represent the whole buffer as
>> returned by the allocator - any offset for the mapping itself relative
>> to the start of the buffer is expressed in vma->pgoff.
>>
>> Thanks,
>> Robin.
> 
> I see, thanks for clarification Robin. I was confused, because depending
> on the backend it will work or not work. But i believe that in this case it is
> undefined behavior. That is unfortunate to me, since the idea behind was:
> - The memory allocated once because of device/firmware
> - Different users could request a part of this memory from the kernel and
> mmap it
> 
> And i didn't want to expose this offset information to the user. Wanted to
> rely on the kernel to mmap the proper part of the buffer.

In principle I don't see why you shouldn't be able to create multiple 
files representing different parts of one large buffer - seems like the 
ops for said files would just need to be a bit cleverer, and remain 
aware of the whole buffer as well as the range of their own part within 
it. Then they can adjust vma->vm_pgoff accordingly before calling 
dma_mmap_*() (and possibly perform their own stricter bounds checking as 
well.)

Thanks,
Robin.

On Tue, Jan 28, 2025 at 7:53 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 27/01/2025 5:47 pm, Dima Stepanov wrote:
> > On Mon, Jan 27, 2025 at 12:50 PM Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> No, that's a bug in the caller of dma_mmap_attrs(). As the kerneldoc
> >> says, cpu_addr/dma_addr/size must still represent the whole buffer as
> >> returned by the allocator - any offset for the mapping itself relative
> >> to the start of the buffer is expressed in vma->pgoff.
> >>
> >> Thanks,
> >> Robin.
> >
> > I see, thanks for clarification Robin. I was confused, because depending
> > on the backend it will work or not work. But i believe that in this case it is
> > undefined behavior. That is unfortunate to me, since the idea behind was:
> > - The memory allocated once because of device/firmware
> > - Different users could request a part of this memory from the kernel and
> > mmap it
> >
> > And i didn't want to expose this offset information to the user. Wanted to
> > rely on the kernel to mmap the proper part of the buffer.
>
> In principle I don't see why you shouldn't be able to create multiple
> files representing different parts of one large buffer - seems like the
> ops for said files would just need to be a bit cleverer, and remain
> aware of the whole buffer as well as the range of their own part within
> it. Then they can adjust vma->vm_pgoff accordingly before calling
> dma_mmap_*() (and possibly perform their own stricter bounds checking as
> well.)
>

This is a good point, i think you are right. Indeed in this case i could go with
the vm_pgoff approach internally without exposing any information to the user.
Thanks for figuring this out!

Thanks, Dima