Re: [PATCH v3 13/32] iommu/fsl_pamu: implement iova_to_phys_length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] iommufd and vfio misinterpret mapped_length as the remaining contiguous length, leading to out-of-bounds physical address batching.
- [Critical] Assigning 64GB (`1ULL << 36`) to `size_t *mapped_length` silently truncates to `0` on 32-bit kernels.
--

commit bd63c934fd433e9c7d21862a31a674fe50af2eae
Author: Guanghui Feng <guanghuifeng@linux.alibaba.com>

iommu/fsl_pamu: implement iova_to_phys_length

This commit implements the iova_to_phys_length callback for the FSL PAMU
IOMMU driver. It returns the actual PTE mapping size for a given IOVA.

> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index 9664ef9840d2c..60abd497dc639 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
[ ... ]
> @@ -169,12 +169,30 @@ static void attach_device(struct fsl_dma_domain *dma_domain, int liodn, struct d
>  	spin_unlock_irqrestore(&device_domain_lock, flags);
>  }
>  
> -static phys_addr_t fsl_pamu_iova_to_phys(struct iommu_domain *domain,
> -					 dma_addr_t iova)
> +static phys_addr_t fsl_pamu_iova_to_phys_length(struct iommu_domain *domain,
> +						dma_addr_t iova,
> +						size_t *mapped_length)
>  {
> +
>  	if (iova < domain->geometry.aperture_start ||
>  	    iova > domain->geometry.aperture_end)
> -		return 0;
> +		return PHYS_ADDR_MAX;
> +
> +	/*
> +	 * PAMU configures exactly one Primary PAACE entry per LIODN with the
> +	 * Multi-Window (MW) bit cleared and ATM = PAACE_ATM_WINDOW_XLATE,
> +	 * WBAL/TWBAL = 0. That is, every LIODN is backed by a single hardware
> +	 * mapping window of fixed size (1ULL << 36, i.e. 64GB, see
> +	 * pamu_config_ppaace()) performing an identity translation. The driver
> +	 * does not split this window into SPAACE sub-windows, so the entire
> +	 * aperture is one PAACE "PTE" entry. Return that single entry's size
> +	 * as mapped_length, matching the iova_to_phys_length contract that
> +	 * mapped_length reports the full size of the mapping entry which
> +	 * covers iova (not the remaining bytes from iova to its end).
> +	 */
> +	if (mapped_length)
> +		*mapped_length = 1ULL << 36;

[Severity: Critical]
Does this assignment silently truncate to 0 on 32-bit kernels?

Since mapped_length is a size_t pointer, assigning 1ULL << 36 (which is 64GB)
will truncate to 0 when size_t is 32-bit, potentially causing caller infinite
loops.

[Severity: Critical]
Could this implementation cause out-of-bounds physical address batching in
callers?

The comment mentions that mapped_length reports the full size of the mapping
entry rather than the remaining bytes from iova to its end.

However, callers appear to misinterpret mapped_length as the remaining
contiguous length. For example, in drivers/iommu/iommufd/pages.c,
batch_from_domain() fails to account for the IOVA offset within the PTE:

	npages = pgsize / PAGE_SIZE;

Similarly, in drivers/vfio/vfio_iommu_type1.c, vfio_unmap_unpin() directly
uses pgsize without adjusting for the offset:

	len = min_t(size_t, pgsize, dma->size - pos);

By returning the full PTE size without adjusting for the offset within the PTE,
can this lead to memory corruption when callers over-batch physical pages?

> +
>  	return iova;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603151804.1963871-1-guanghuifeng@linux.alibaba.com?part=13