Re: [PATCH v2 08/13] mm: introduce vma_get_page_prot() and use it

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Modifying `vma->vm_page_prot` after `vm_mmap()` without `vma_start_write(vma)` causes data races with per-VMA lock readers and allows cache aliasing via concurrent page faults.
- [High] Multiple DRM drivers call `drm_gem_vm_close(vma)` on their internal `mmap` error paths, causing a double-put and Use-After-Free (UAF).
- [High] `i915_gem_mmap_ioctl()` leaks a user-space memory mapping (VMA) if operations fail after `vm_mmap()` succeeds.
- [High] `exynos_drm_gem_mmap()` leaks a GEM object reference when delegating mapping of an imported dma-buf.
--

--- Patch [8]: [PATCH v2 08/13] mm: introduce vma_get_page_prot() and use it ---
commit 02617eb8c6f24873696c9debadc114509af28be5
Author: Lorenzo Stoakes <ljs@kernel.org>

mm: introduce vma_get_page_prot() and use it

This commit introduces a new helper vma_get_page_prot() to simplify
retrieving the page protection from a VMA. It replaces numerous direct
calls to vm_get_page_prot() across various DRM and core mm files.

> diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/drm_gem_dma_helper.c
> index 1c00a71ab3c9d..7d9612075d310 100644
> --- a/drivers/gpu/drm/drm_gem_dma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_dma_helper.c
> @@ -540,7 +540,7 @@ int drm_gem_dma_mmap(struct drm_gem_dma_object *dma_obj, struct vm_area_struct *
>  	vm_flags_mod(vma, VM_DONTDUMP | VM_DONTEXPAND, VM_PFNMAP);
>  
>  	if (dma_obj->map_noncoherent) {
> -		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +		vma->vm_page_prot = vma_get_page_prot(vma);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential reference
count underflow here.

In drm_gem_dma_mmap(), if the dma_mmap_pages() or dma_mmap_wc() mapping
fails, we call drm_gem_vm_close(vma) which drops the GEM object reference.
However, the caller drm_gem_mmap_obj() will also hit its own error path
and call drm_gem_object_put(obj) again on the same object. Could this
lead to a double-put and use-after-free? I see a similar pattern in
__tegra_gem_mmap() in the tegra driver as well.

>  
>  		ret = dma_mmap_pages(drm_dev_dma_dev(dma_obj->base.dev),
>  				     vma, vma->vm_end - vma->vm_start,

> [ ... ]

> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 9a6270f3dca6b..0208c92595725 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -377,13 +377,13 @@ static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct
>  
>  	/* non-cachable as default. */
>  	if (exynos_gem->flags & EXYNOS_BO_CACHABLE)
> -		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +		vma->vm_page_prot = vma_get_page_prot(vma);

[Severity: High]
This is a pre-existing issue, but does this function leak a GEM object
reference when mapping an imported dma-buf?

When exynos_drm_gem_mmap() delegates an imported dma-buf to dma_buf_mmap()
early in the function, the dma_buf_mmap() routine replaces
vma->vm_private_data and vma->vm_ops with the exporter's state. When the
VMA is later unmapped, the exporter's close handler is invoked instead of
drm_gem_vm_close(). Doesn't this mean the reference originally taken by
drm_gem_mmap_obj() right before calling this callback is never dropped?

>  	else if (exynos_gem->flags & EXYNOS_BO_WC)
>  		vma->vm_page_prot =
> -			pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> +			pgprot_writecombine(vma_get_page_prot(vma));

> [ ... ]

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 0644f85c6c8e5..9ca90c1bb5b42 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -112,7 +112,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,

[Severity: High]
This isn't a bug introduced by this patch, but could this code leak a VMA?

If vm_mmap() succeeds earlier in i915_gem_mmap_ioctl(), but we then hit
the mmap_write_lock_killable() failure path:

i915_gem_mmap_ioctl()
    addr = vm_mmap(...);
    ...
    if (mmap_write_lock_killable(mm)) {
        addr = -EINTR;
        goto err;
    }
    
Does this overwrite the successfully mapped address and jump to the error
handling without unmapping the VMA, permanently leaking it?

>  		vma = find_vma(mm, addr);
>  		if (vma && __vma_matches(vma, obj->base.filp, addr, args->size))
>  			vma->vm_page_prot =
> -				pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> +				pgprot_writecombine(vma_get_page_prot(vma));

[Severity: High]
This is also a pre-existing issue, but is it safe to modify the VMA
page protection properties here without holding the per-VMA write lock?

We hold the mmap_write_lock, but since vma_start_write() is not called
first, per-VMA lock readers are not drained. Could a concurrent page
fault handle install Write-Back PTEs while we change the VMA to
Write-Combine, leading to cache aliasing?

>  		else
>  			addr = -ENOMEM;
>  		mmap_write_unlock(mm);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-b4-vma-flags-mm-v2-0-0fa2357d5431@kernel.org?part=8