Re: [PATCH v3 12/16] accel/thames: Install memory mappings populated and dirty

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] accel/thames: double put and NULL deref on error in thames_gem_mmap()
- [Critical] accel/thames: COW bypass in thames_gem_mmap() on MAP_PRIVATE mappings
- [Low] accel/thames: MMU-only PTE helpers in thames_gem.c break !MMU COMPILE_TEST builds
--

commit c4d46a848bca800b97f80e3d5bd5a0d1cb370cf4
Author: Tomeu Vizoso <tomeu.vizoso@gmail.com>

accel/thames: Install memory mappings populated and dirty

This commit replaces the default GEM DMA and GEM shmem mmap helpers in the
thames accelerator driver with custom mmap callbacks that pre-populate page
table entries as writable and dirty up front.

> diff --git a/drivers/accel/thames/thames_gem.c b/drivers/accel/thames/thames_gem.c
> index 5a4dc497768f8..9f55169c6370b 100644
> --- a/drivers/accel/thames/thames_gem.c
> +++ b/drivers/accel/thames/thames_gem.c
> @@ -808,12 +808,82 @@ static void thames_gem_shmem_bo_free(struct drm_gem_object *obj)
>  	drm_gem_shmem_free(&bo->shmem);
>  }
>  
> +/*
> + * Installed dirty and writable: the A53s have no hardware dirty-bit management,
> + * so every first write would fault.
> + */
> +static int thames_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> +{
> +	struct drm_gem_dma_object *dma_obj = to_drm_gem_dma_obj(obj);
> +	pgprot_t prot;
> +	int ret;
> +
> +	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
> +	vm_flags_mod(vma, VM_DONTDUMP | VM_DONTEXPAND, VM_PFNMAP);
> +
> +	prot = vm_get_page_prot(vma->vm_flags);
> +	if (vma->vm_flags & VM_WRITE)
> +		prot = pte_pgprot(pte_mkdirty(pte_mkwrite_novma(pfn_pte(0, prot))));
> +	vma->vm_page_prot = prot;

[Severity: Critical]
What happens here if userspace calls mmap() with PROT_WRITE and MAP_PRIVATE
on a contiguous buffer object?

In that case, vma_is_cow_mapping(vma) is true (VM_WRITE is set while
VM_SHARED is clear), so vm_get_page_prot(vma->vm_flags) returns read-only
page protection to fault on writes for copy-on-write.

Because thames_gem_mmap() does not reject vma_is_cow_mapping(vma) (unlike
thames_gem_shmem_mmap() below) and applies pte_mkwrite_novma() whenever
VM_WRITE is set, dma_mmap_pages() -> remap_pfn_range() will install
hardware-writable PTEs pointing directly to dma_obj->vaddr in a private
mapping, allowing writes to bypass copy-on-write and mutate the shared DMA
buffer.

Should thames_gem_mmap() reject vma_is_cow_mapping(vma) with -EINVAL as well?

> +
> +	ret = dma_mmap_pages(drm_dev_dma_dev(obj->dev), vma, vma->vm_end - vma->vm_start,
> +			     virt_to_page(dma_obj->vaddr));
> +	if (ret)
> +		drm_gem_vm_close(vma);
> +
> +	return ret;
> +}

[Severity: Critical]
Does calling drm_gem_vm_close(vma) when dma_mmap_pages() fails cause a
double put or a NULL pointer dereference?

thames_gem_mmap() is wired up as the drm_gem_object_funcs.mmap callback in
thames_gem_funcs.

When called via drm_gem_mmap() -> drm_gem_mmap_obj(), drm_gem_mmap_obj()
takes one reference on obj for the VMA and drops that reference on error:

drivers/gpu/drm/drm_gem.c:drm_gem_mmap_obj() {
	drm_gem_object_get(obj);

	vma->vm_private_data = obj;
	vma->vm_ops = obj->funcs->vm_ops;

	if (obj->funcs->mmap) {
		ret = obj->funcs->mmap(obj, vma);
		if (ret)
			goto err_drm_gem_object_put;
	...
err_drm_gem_object_put:
	drm_gem_object_put(obj);
	return ret;
}

Since drm_gem_vm_close(vma) also calls drm_gem_object_put(vma->vm_private_data),
a failure in dma_mmap_pages() (for example, -ENOMEM in remap_pfn_range() or
-ENXIO on an out-of-range page offset) drops obj's reference twice,
underflowing the reference count held by the GEM handle and prematurely
freeing the buffer object.

Furthermore, when called on an exported DMA-BUF via
dma_buf_mmap_internal() -> drm_gem_dmabuf_mmap() -> drm_gem_prime_mmap(),
vma->vm_private_data is still NULL when obj->funcs->mmap(obj, vma) runs:

drivers/gpu/drm/drm_prime.c:drm_gem_prime_mmap() {
	if (obj->funcs && obj->funcs->mmap) {
		vma->vm_ops = obj->funcs->vm_ops;

		drm_gem_object_get(obj);
		ret = obj->funcs->mmap(obj, vma);
		if (ret) {
			drm_gem_object_put(obj);
			return ret;
		}
		vma->vm_private_data = obj;
		return 0;
	}
}

In that path, calling drm_gem_vm_close(vma) passes NULL to
drm_gem_object_put(), triggering a NULL pointer dereference.

> +
> +/*
> + * Every page installed now, dirty and writable: they are pinned for the
> + * object's life, and the helper would take two faults on each.
> + */
> +static int thames_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> +{
> +	struct thames_gem_object *bo = to_thames_bo(obj);
> +	unsigned long addr = vma->vm_start;
> +	struct scatterlist *sg;
> +	pgprot_t prot;
> +	unsigned int i;
> +	int ret;
> +
> +	if (vma_is_cow_mapping(vma))
> +		return -EINVAL;
> +
> +	vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
> +
> +	prot = vm_get_page_prot(vma->vm_flags);
> +	if (vma->vm_flags & VM_WRITE)
> +		prot = pte_pgprot(pte_mkdirty(pte_mkwrite_novma(pfn_pte(0, prot))));
> +	vma->vm_page_prot = prot;

[Severity: Low]
Could the use of pfn_pte(), pte_mkwrite_novma(), pte_mkdirty(), and
pte_pgprot() in both thames_gem_mmap() and thames_gem_shmem_mmap() break
COMPILE_TEST builds when CONFIG_MMU is disabled or clear protection bits on
architectures without a custom pte_pgprot()?

In drivers/accel/thames/Kconfig, DRM_ACCEL_THAMES depends on
TI_K3_DSP_REMOTEPROC (which allows COMPILE_TEST) without a depends on MMU
dependency. On !CONFIG_MMU targets, pfn_pte(), pte_mkwrite_novma(), and
pte_mkdirty() are not declared. Additionally, on architectures that rely on
the generic fallback in include/linux/pgtable.h:

#ifndef pte_pgprot
#define pte_pgprot(x) ((pgprot_t) {0})
#endif

pte_pgprot() zeroes out all protection bits in prot.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-thames-v3-0-3ab40aa17157@tomeuvizoso.net?part=12