Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] msm: missing TLB flush in unmap sequence causes GPU UAF - [Low] msm: build failure due to undefined __do_map and __do_unmap --
commit a6f7885817ad9924dcb4d6924c8061a9096367bc Author: Rob Clark <[email protected]> Subject: drm/msm: Convert iommu map/unmap to helpers This patch converts the msm iommu map and unmap paths to use new helpers. It replaces calls to iommu_map_sgtable() and iommu_unmap() with __do_map() and __do_unmap(), utilizing __map_iommu() and __unmap_iommu() callbacks. > diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c > index da6782fca6bd2..03acc851c5e6d 100644 > --- a/drivers/gpu/drm/msm/msm_iommu.c > +++ b/drivers/gpu/drm/msm/msm_iommu.c [ ... ] > @@ -684,12 +684,27 @@ static void msm_iommu_detach(struct msm_mmu *mmu) > iommu_detach_device(iommu->domain, mmu->dev); > } > > +static size_t > +__unmap_iommu(void *arg, u64 iova, size_t pgsize, size_t pgcount) > +{ > + struct iommu_domain *domain = arg; > + return domain->ops->unmap_pages(domain, iova, pgsize, pgcount, NULL); > +} [Severity: Critical] By passing NULL for iotlb_gather, does this bypass the IOMMU driver's TLB gathering and flushing mechanism? When iommu_unmap() was previously used, it provided an iotlb_gather structure and followed up with a synchronous TLB flush. Since this code is invoked from msm_iommu_unmap(), leaving the TLB unflushed might retain stale translations in hardware. Can this lead to a use-after-free regression if the physical pages are later reallocated and accessed by the GPU through the stale TLB entries? > + > +static int > +__map_iommu(void *arg, phys_addr_t paddr, u64 iova, size_t pgsize, [ ... ] > @@ -697,23 +712,18 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t > iova, > if (iova & BIT_ULL(48)) > iova |= GENMASK_ULL(63, 49); > > - ret = iommu_map_sgtable(iommu->domain, iova, sgt, prot); > - if (ret < 0) > - return ret; > - > - return (ret == len) ? 0 : -EINVAL; > + return __do_map(domain->pgsize_bitmap, iova, sgt, off, len, prot, > + domain, __map_iommu, __unmap_iommu); > } [Severity: Low] Is __do_map() defined anywhere in the tree? It appears this replaces iommu_map_sgtable() with a call to __do_map(), but there is no declaration or definition of __do_map() available. Does this cause an implicit declaration compile error when building with DRM_MSM enabled? > > static int msm_iommu_unmap(struct msm_mmu *mmu, uint64_t iova, size_t len) > { > - struct msm_iommu *iommu = to_msm_iommu(mmu); > + struct iommu_domain *domain = to_msm_iommu(mmu)->domain; > > if (iova & BIT_ULL(48)) > iova |= GENMASK_ULL(63, 49); > > - iommu_unmap(iommu->domain, iova, len); > - > - return 0; > + return __do_unmap(domain->pgsize_bitmap, iova, len, domain, > __unmap_iommu); > } [Severity: Low] Similar to the mapping path, is __do_unmap() actually defined? It looks like the change from iommu_unmap() to __do_unmap() will fail to compile due to a missing definition. Additionally, if __do_unmap() simply invokes __unmap_iommu() and returns, is the required TLB flush completely missing from this unmap sequence? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
