Re: [PATCH v2] drm/etnaviv: honor read-only userptr flag in GPU MMU mapping

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] MMUv1 hardware lacks read-only page protection, allowing unprivileged users to overwrite arbitrary read-only CPU pages (e.g., `/etc/passwd`) via the GPU.
- [Critical] The MMUv1 linear window shortcut for single-entry BOs bypasses page-table isolation, allowing unprivileged users to arbitrarily read and write physical memory.
- [High] Read-only userptr BOs are mapped using DMA_BIDIRECTIONAL, causing memory corruption when SWIOTLB bounce buffers are unmapped.
- [High] Calling drm_gem_object_put() on a partially initialized GEM object triggers a kernel panic via list_del() on an uninitialized list node.
--

--- Patch [1]: [PATCH v2] drm/etnaviv: honor read-only userptr flag in GPU MMU mapping ---
commit 3b31d2a8b362305b34038576c89dbe65957eea85
Author: Christopher Lusk <clusk@northecho.dev>

drm/etnaviv: honor read-only userptr flag in GPU MMU mapping

The userptr interface records the requested access mode in
etnaviv_obj->userptr.ro, but etnaviv_iommu_map_gem() ignores it and maps
every buffer with ETNAVIV_PROT_READ | ETNAVIV_PROT_WRITE. This patch
builds the protection mask from userptr.ro instead, enforcing read-only
mappings on MMUv2 and confining the GPU to mapped pages on MMUv1.

Link: https://lore.kernel.org/all/20260508180518.1417371-1-n7l8m4@u.northwestern.edu/

> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
[ ... ]
> @@ -735,9 +735,20 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
>  	uintptr_t ptr, u32 size, u32 flags, u32 *handle)
>  {
>  	struct etnaviv_gem_object *etnaviv_obj;
> +	u32 bo_flags = ETNA_BO_CACHED;
>  	int ret;
>  
> -	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_CACHED,
> +	/*
> +	 * Keep read-only userptr BOs out of the MMUv1 linear window, which
> +	 * would expose far more than the pinned pages to the GPU.  MMUv1
> +	 * PTEs have no writeable bit, so this confines the GPU rather than
> +	 * making the BO read-only; MMUv2 enforces read-only per PTE in
> +	 * etnaviv_iommu_map_gem().
> +	 */
> +	if (!(flags & ETNA_USERPTR_WRITE))
> +		bo_flags |= ETNA_BO_FORCE_MMU;

[Severity: Critical]
This is a pre-existing issue, but does this explicitly leave read-only
userptr BOs writable by the GPU on MMUv1 hardware?

Since MMUv1 lacks read-only page protection and ignores the protection
argument, an unprivileged user could pin a read-only file via the userptr
ioctl and submit a GPU command stream to write to the mapped buffer.

Does this allow arbitrary modification of read-only files or the zero
page?

[Severity: Critical]
This is a pre-existing issue, but does the MMUv1 linear window shortcut
bypass page-table isolation for single-entry BOs?

While this patch forces ETNA_BO_FORCE_MMU for read-only userptrs, writable
userptrs or standard shmem BOs lacking this flag are given an IOVA directly
offset into a 2GB linear window in etnaviv_iommu_map_gem():

	if (context->global->version == ETNAVIV_IOMMU_V1 &&
	    sgt->nents == 1 && !(etnaviv_obj->flags & ETNA_BO_FORCE_MMU)) {
		u32 iova;

		iova = sg_dma_address(sgt->sgl) - memory_base;

Without bounds checking, could a malicious command stream access this offset
to read or write arbitrary physical pages in the contiguous memory area?

[ ... ]
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
[ ... ]
> @@ -269,10 +269,18 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>  {
>  	struct sg_table *sgt = etnaviv_obj->sgt;
>  	struct drm_mm_node *node;
> +	int prot = ETNAVIV_PROT_READ;
>  	int ret;
>  
>  	lockdep_assert_held(&etnaviv_obj->lock);
>  
> +	/*
> +	 * Read-only userptr BOs drop ETNAVIV_PROT_WRITE.  MMUv2 honors this
> +	 * via MMUv2_PTE_WRITEABLE; MMUv1 ignores prot entirely.
> +	 */
> +	if (!etnaviv_obj->userptr.mm || !etnaviv_obj->userptr.ro)
> +		prot |= ETNAVIV_PROT_WRITE;
> +

[Severity: Critical]
This is a pre-existing issue, but does dropping ETNAVIV_PROT_WRITE only
protect MMUv2?

In etnaviv_iommu_map_gem(), since MMUv1 ignores the protection flag entirely,
the read-only userptr remains writable by the GPU. Could this overwrite
arbitrary read-only CPU pages?

[ ... ]
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
[ ... ]
>  if (etnaviv_obj->flags & ETNA_BO_CACHE_MASK)
>  	dma_map_sgtable(dev->dev, sgt, DMA_BIDIRECTIONAL, 0);

[Severity: High]
This is a pre-existing issue, but does etnaviv_gem_scatter_map() hardcode
DMA_BIDIRECTIONAL for all cached BOs, including read-only userptrs?

If the system uses SWIOTLB bounce buffering, dma_unmap_sgtable() with
DMA_BIDIRECTIONAL in etnaviv_gem_scatterlist_unmap() will copy the
modified bounce buffer contents back over the original CPU pages.

For a userptr pinned without FOLL_WRITE, could this forcefully overwrite
read-only file mappings or the zero page?

[ ... ]
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
[ ... ]
>  fail:
>  	drm_gem_object_put(&etnaviv_obj->base);

[Severity: High]
This is a pre-existing issue, but can calling drm_gem_object_put() on a
partially initialized GEM object trigger a kernel panic?

In etnaviv_gem_prime_import_sg_table(), if object initialization fails,
the error path drops the refcount to 0 and calls etnaviv_gem_free_object():

	mutex_lock(&priv->gem_lock);
	list_del(&etnaviv_obj->gem_node);
	mutex_unlock(&priv->gem_lock);

Since etnaviv_gem_obj_add() was not yet called, the gem_node is
uninitialized. Will list_del() dereference NULL pointers and crash here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724222124.537101-1-clusk@northecho.dev?part=1