Re: [RFT 4/5] drm/msm: Convert iommu map/unmap to helpers

[sashiko-bot@kernel.org]

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 <robin.clark@oss.qualcomm.com>
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/20261006131000.81501-5-robin.clark@oss.qualcomm.com?part=1