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