[PATCH] drm/gem-dma: bound the mmap against the object size

[PATCH] drm/gem-dma: bound the mmap against the object size

[Baul Lee]

drm_gem_dma_mmap() passes the caller's request, not the object, as the
size of the buffer being mapped:

	ret = dma_mmap_wc(drm_dev_dma_dev(dma_obj->base.dev), vma,
			  dma_obj->vaddr, dma_obj->dma_addr,
			  vma->vm_end - vma->vm_start);

dma_direct_mmap() derives its page count from that size, so count and
user_count are the same number and the bounds test degenerates into
"vm_pgoff must be zero":

	unsigned long user_count = vma_pages(vma);
	unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
	...
	if (vma->vm_pgoff >= count || user_count > count - vma->vm_pgoff)
		return -ENXIO;

remap_pfn_range() then installs writable PTEs for every frame the caller
asked for, starting at the object and running past its end.

On the DRM node that is unreachable: drm_gem_mmap_obj() rejects a VMA
larger than the object before the object's mmap function runs.  The fbdev
emulation does not go through it.  drm_fbdev_dma_fb_mmap() calls
drm_gem_prime_mmap(), which invokes obj->funcs->mmap() directly, so
/dev/fb0 accepts a length that /dev/dri/card0 refuses for the same object.

The framebuffer is contiguous CMA inside system DRAM, so the excess
mapping covers kernel-owned RAM, readable and writable, at an offset the
caller picks.  On arm64 a 1 GiB mapping of an 8294400-byte framebuffer
object was accepted, and writes through it landed in the private memory
of another unprivileged process.  Nothing is logged: every access comes
from userspace through a PTE the kernel installed.  open() and mmap() on
/dev/fb0 are enough, so any member of group video reaches it.

Pass the object size to the DMA layer, which makes the dma_direct_mmap()
test meaningful, and check the VMA length the way drm_gem_mmap_obj() does
so both nodes reject the same request.

Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA helpers")
Cc: stable@vger.kernel.org
Signed-off-by: Baul Lee <baul.lee@xbow.com>
---
 drivers/gpu/drm/drm_gem_dma_helper.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/drm_gem_dma_helper.c
--- a/drivers/gpu/drm/drm_gem_dma_helper.c
+++ b/drivers/gpu/drm/drm_gem_dma_helper.c
@@ -531,6 +531,9 @@ int drm_gem_dma_mmap(struct drm_gem_dma_object *dma_obj, struct vm_area_struct *
 	struct drm_gem_object *obj = &dma_obj->base;
 	int ret;

+	if (obj->size < vma->vm_end - vma->vm_start)
+		return -EINVAL;
+
 	/*
 	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
 	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
@@ -543,12 +546,12 @@ int drm_gem_dma_mmap(struct drm_gem_dma_object *dma_obj, struct vm_area_struct *
 		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);

 		ret = dma_mmap_pages(drm_dev_dma_dev(dma_obj->base.dev),
-				     vma, vma->vm_end - vma->vm_start,
+				     vma, obj->size,
 				     virt_to_page(dma_obj->vaddr));
 	} else {
 		ret = dma_mmap_wc(drm_dev_dma_dev(dma_obj->base.dev), vma,
 				  dma_obj->vaddr, dma_obj->dma_addr,
-				  vma->vm_end - vma->vm_start);
+				  obj->size);
 	}
 	if (ret)
 		drm_gem_vm_close(vma);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] drm/gem-dma: bound the mmap against the object size

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Double put of the GEM object when dma_mmap_pages() or dma_mmap_wc() fails in drm_gem_dma_mmap(), leading to a potential use-after-free.
--

--- Patch [1]: [PATCH] drm/gem-dma: bound the mmap against the object size ---
commit ec72c1516cca4ec0e08262f650b80d864b2ece0c
Author: Baul Lee <baul.lee@xbow.com>

drm/gem-dma: bound the mmap against the object size

This commit updates drm_gem_dma_mmap() to pass the object size to the DMA
layer instead of the VMA length. It also adds an early bounds check to
reject VMA lengths exceeding the object size.

> diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/drm_gem_dma_helper.c
> index 1c00a71ab3c9d..4211b404ded22 100644
> --- a/drivers/gpu/drm/drm_gem_dma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_dma_helper.c
[ ... ]
> @@ -543,12 +546,12 @@ int drm_gem_dma_mmap(struct drm_gem_dma_object *dma_obj, struct vm_area_struct *
>  		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>  
>  		ret = dma_mmap_pages(drm_dev_dma_dev(dma_obj->base.dev),
> -				     vma, vma->vm_end - vma->vm_start,
> +				     vma, obj->size,
>  				     virt_to_page(dma_obj->vaddr));
>  	} else {
>  		ret = dma_mmap_wc(drm_dev_dma_dev(dma_obj->base.dev), vma,
>  				  dma_obj->vaddr, dma_obj->dma_addr,
> -				  vma->vm_end - vma->vm_start);
> +				  obj->size);
>  	}
>  	if (ret)
>  		drm_gem_vm_close(vma);

[Severity: Critical]
This is a pre-existing issue, but is it possible this error path causes a
double-put of the GEM object?

When a user calls mmap on a DRM device node under memory pressure,
drm_gem_mmap_obj() in drivers/gpu/drm/drm_gem.c sets vma->vm_private_data
to the object and then calls the object's mmap function:

drivers/gpu/drm/drm_gem.c:drm_gem_mmap_obj() {
    ...
    vma->vm_private_data = obj;
    ...
    ret = obj->funcs->mmap(obj, vma);
    ...
}

If the underlying dma_mmap_pages() or dma_mmap_wc() fails (for example,
returning -ENOMEM), drm_gem_dma_mmap() calls drm_gem_vm_close(vma), which
drops the first reference.

The error is then returned to drm_gem_mmap_obj(), which takes the error
path and drops a second reference:

drivers/gpu/drm/drm_gem.c:drm_gem_mmap_obj() {
    ...
err_drm_gem_object_put:
    drm_gem_object_put(obj);
    return ret;
}

Could this double-put lead to a use-after-free?

>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805080147.12564-1-baul.lee@xbow.com?part=1