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