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

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 <[email protected]>

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/[email protected]?part=1