[PATCH] dma-remap: fix dma_common_find_pages() page lookup for offsets

[PATCH] dma-remap: fix dma_common_find_pages() page lookup for offsets

[Andrei-Edward Popa]

dma_common_find_pages() previously assumed that the CPU virtual address
always pointed to the start of a DMA-coherent allocation. This fails when
memory is allocated via dma_alloc_attrs() without DMA_ATTR_FORCE_CONTIGUOUS
and then subdivided into smaller blocks using a gen_pool, relevant only
when an IOMMU is enabled.

In such cases, userspace may request a mapping via dma_mmap_attrs()
for a CPU address that is offset inside the original allocation. The
previous code could return the wrong struct page pointer.

Example scenario:

  - Allocate a large DMA buffer with dma_alloc_attrs() (non-contiguous)
  - Create a gen_pool using this buffer
  - Take sub-allocations from the gen_pool
  - Map the sub-allocations to userspace via dma_mmap_attrs()
  - dma_common_find_pages() must return the correct struct page for the offset

This patch computes the page index relative to the vm_struct backing
the DMA allocation:

    page_index = (vaddr - area->addr) >> PAGE_SHIFT

Bounds checks ensure the CPU address is within the vm_struct and
page_index < area->nr_pages.

This ensures correct behavior for sub-allocated regions from gen_pool,
without affecting allocations starting at the base address or allocations
with DMA_ATTR_FORCE_CONTIGUOUS.

Signed-off-by: Andrei-Edward Popa <andrei.popa105@yahoo.com>
---
 kernel/dma/remap.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/kernel/dma/remap.c b/kernel/dma/remap.c
index b7c1c0c92d0c..d27477e32ed8 100644
--- a/kernel/dma/remap.c
+++ b/kernel/dma/remap.c
@@ -9,12 +9,25 @@
 struct page **dma_common_find_pages(void *cpu_addr)
 {
 	struct vm_struct *area = find_vm_area(cpu_addr);
+	unsigned long vaddr, area_vaddr;
+	size_t page_index;
 
 	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;
+
+	vaddr = (unsigned long)cpu_addr;
+	area_vaddr = (unsigned long)area->addr;
+	if (unlikely(vaddr < area_vaddr ||
+		     vaddr >= area_vaddr + area->size))
+		return NULL;
+
+	page_index = (vaddr - area_vaddr) >> PAGE_SHIFT;
+	if (unlikely(page_index >= area->nr_pages))
+		return NULL;
+
+	return &area->pages[page_index];
 }
 
 /*
-- 
2.52.0

Re: [PATCH] dma-remap: fix dma_common_find_pages() page lookup for offsets

[Christoph Hellwig]

On Fri, Dec 12, 2025 at 10:09:14PM +0200, Andrei-Edward Popa wrote:
> dma_common_find_pages() previously assumed that the CPU virtual address
> always pointed to the start of a DMA-coherent allocation. This fails when
> memory is allocated via dma_alloc_attrs() without DMA_ATTR_FORCE_CONTIGUOUS
> and then subdivided into smaller blocks using a gen_pool, relevant only
> when an IOMMU is enabled.
> 
> In such cases, userspace may request a mapping via dma_mmap_attrs()
> for a CPU address that is offset inside the original allocation. The
> previous code could return the wrong struct page pointer.

No, you can't mmap part of a dma coherent allocation.  What caller is
trying to do this?  It needs to be fixed instead.

Re: [PATCH] dma-remap: fix dma_common_find_pages() page lookup for offsets

[Marek Szyprowski]

On 15.12.2025 06:57, Christoph Hellwig wrote:
> On Fri, Dec 12, 2025 at 10:09:14PM +0200, Andrei-Edward Popa wrote:
>> dma_common_find_pages() previously assumed that the CPU virtual address
>> always pointed to the start of a DMA-coherent allocation. This fails when
>> memory is allocated via dma_alloc_attrs() without DMA_ATTR_FORCE_CONTIGUOUS
>> and then subdivided into smaller blocks using a gen_pool, relevant only
>> when an IOMMU is enabled.
>>
>> In such cases, userspace may request a mapping via dma_mmap_attrs()
>> for a CPU address that is offset inside the original allocation. The
>> previous code could return the wrong struct page pointer.
> No, you can't mmap part of a dma coherent allocation.  What caller is
> trying to do this?  It needs to be fixed instead.

I wonder if this was ever explicitly stated.

dma_mmap_coherent() was initially added for mmapeing a 
dma_alloc_coherent()-allocated buffer for fbdev and alsa, and at least 
the first one allowed to mmap the buffer partially or starting at 
non-zero offset. I doubt that this feature was useful for anything, but 
I'm quite sure this was at least allowed and there were some comments in 
the code about that.

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

Re: [PATCH] dma-remap: fix dma_common_find_pages() page lookup for offsets

[Robin Murphy]

On 2025-12-15 8:13 am, Marek Szyprowski wrote:
> On 15.12.2025 06:57, Christoph Hellwig wrote:
>> On Fri, Dec 12, 2025 at 10:09:14PM +0200, Andrei-Edward Popa wrote:
>>> dma_common_find_pages() previously assumed that the CPU virtual address
>>> always pointed to the start of a DMA-coherent allocation. This fails when
>>> memory is allocated via dma_alloc_attrs() without DMA_ATTR_FORCE_CONTIGUOUS
>>> and then subdivided into smaller blocks using a gen_pool, relevant only
>>> when an IOMMU is enabled.
>>>
>>> In such cases, userspace may request a mapping via dma_mmap_attrs()
>>> for a CPU address that is offset inside the original allocation. The
>>> previous code could return the wrong struct page pointer.
>> No, you can't mmap part of a dma coherent allocation.  What caller is
>> trying to do this?  It needs to be fixed instead.
> 
> I wonder if this was ever explicitly stated.

Seems it never made it into the text documentation, but it is stated in 
the kerneldoc.

> dma_mmap_coherent() was initially added for mmapeing a
> dma_alloc_coherent()-allocated buffer for fbdev and alsa, and at least
> the first one allowed to mmap the buffer partially or starting at
> non-zero offset. I doubt that this feature was useful for anything, but
> I'm quite sure this was at least allowed and there were some comments in
> the code about that.

Mapping part of a buffer in general is fine, provided the fd advertised 
to userspace represents the entire buffer - then the offset/size of the 
VMA determine how much of it actually gets mapped. What you can't do is 
advertise parts of a buffer *as separate fds* and then expect to pass 
mmap calls on those straight through, without adjusting them to be 
relative to the whole buffer.

We don't provide such a generic helper for mmap'ing from a generic 
dma_pool, largely because the main point of those is for drivers that 
want to allocate lots of buffers smaller than PAGE_SIZE, such that 
trying to mmap them to userspace would most likely be a giant security 
hole anyway.

Thanks,
Robin.

Re: [PATCH] dma-remap: fix dma_common_find_pages() page lookup for offsets

[Andrei-Edward Popa]

Hi Christoph, Marek, Robin,

Thanks for the detailed explanations, that clarified things a lot.

I now understand that dma_mmap_attrs() expects cpu_addr to be the exact
pointer returned by dma_alloc_attrs(), and that subdividing an allocation
(e.g. via gen_pool) requires the driver to keep track of offsets and
adjust the mmap parameters accordingly before calling into the DMA API.

My patch was effectively trying to make dma_common_find_pages() accept
offset cpu_addr values implicitly, which would change the DMA API
semantics and mask driver bugs rather than fixing them in the driver
itself.

Given this, I agree that the correct fix is in the driver using the API,
not in dma-remap, and I’ll drop this patch.

Thanks again for taking the time to explain the intended usage and
constraints of the DMA mmap helpers.

Best regards,
Andrei

Re: [PATCH] dma-remap: fix dma_common_find_pages() page lookup for offsets

[Robin Murphy]

On 2025-12-12 8:09 pm, Andrei-Edward Popa wrote:
> dma_common_find_pages() previously assumed that the CPU virtual address
> always pointed to the start of a DMA-coherent allocation. This fails when
> memory is allocated via dma_alloc_attrs() without DMA_ATTR_FORCE_CONTIGUOUS
> and then subdivided into smaller blocks using a gen_pool, relevant only
> when an IOMMU is enabled.
> 
> In such cases, userspace may request a mapping via dma_mmap_attrs()
> for a CPU address that is offset inside the original allocation. The
> previous code could return the wrong struct page pointer.
> 
> Example scenario:
> 
>    - Allocate a large DMA buffer with dma_alloc_attrs() (non-contiguous)
>    - Create a gen_pool using this buffer
>    - Take sub-allocations from the gen_pool
>    - Map the sub-allocations to userspace via dma_mmap_attrs()

That's where the bug is. If a driver wants to subdivide an allocation 
then it needs to keep track of what it's done, and handle mmap properly 
to offset the VMA relative to the original allocation before passing the 
request through. See the kerneldoc for dma_mmap_attrs():

  * @cpu_addr: kernel CPU-view address returned from dma_alloc_attrs
  * @dma_addr: device-view address returned from dma_alloc_attrs
  * @size: size of memory originally requested in dma_alloc_attrs

"address returned from dma_alloc_attrs" does not mean "any arbitrary 
address within the bounds of the original address/size", it means 
exactly what it says, i.e. the original address.

Thanks
Robin.

>    - dma_common_find_pages() must return the correct struct page for the offset
> 
> This patch computes the page index relative to the vm_struct backing
> the DMA allocation:
> 
>      page_index = (vaddr - area->addr) >> PAGE_SHIFT
> 
> Bounds checks ensure the CPU address is within the vm_struct and
> page_index < area->nr_pages.
> 
> This ensures correct behavior for sub-allocated regions from gen_pool,
> without affecting allocations starting at the base address or allocations
> with DMA_ATTR_FORCE_CONTIGUOUS.
> 
> Signed-off-by: Andrei-Edward Popa <andrei.popa105@yahoo.com>
> ---
>   kernel/dma/remap.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/remap.c b/kernel/dma/remap.c
> index b7c1c0c92d0c..d27477e32ed8 100644
> --- a/kernel/dma/remap.c
> +++ b/kernel/dma/remap.c
> @@ -9,12 +9,25 @@
>   struct page **dma_common_find_pages(void *cpu_addr)
>   {
>   	struct vm_struct *area = find_vm_area(cpu_addr);
> +	unsigned long vaddr, area_vaddr;
> +	size_t page_index;
>   
>   	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;
> +
> +	vaddr = (unsigned long)cpu_addr;
> +	area_vaddr = (unsigned long)area->addr;
> +	if (unlikely(vaddr < area_vaddr ||
> +		     vaddr >= area_vaddr + area->size))
> +		return NULL;
> +
> +	page_index = (vaddr - area_vaddr) >> PAGE_SHIFT;
> +	if (unlikely(page_index >= area->nr_pages))
> +		return NULL;
> +
> +	return &area->pages[page_index];
>   }
>   
>   /*