Re: [PATCH 6/9] iommu: direct page-table drivers implement iova_to_phys_length

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unmapped IOVAs return fake physical address and length
- [High] viommu_iova_to_phys_length returns non-power-of-two size, corrupting VFIO unmap
- [Medium] Integer truncation on 32-bit systems defeats unmap optimization

Pre-existing issues:
- [Critical] NULL pointer dereference when querying unmapped IOVAs on s390
--

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

iommu: direct page-table drivers implement iova_to_phys_length

Implement iova_to_phys_length for all IOMMU drivers with direct page tables.
Each returns the actual PTE mapping size. Also fixes an mtk_iommu_v1
pre-existing bug.

> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index 9664ef9840d2c..48a072074b56c 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
[ ... ]
> +	/*
> +	 * 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: Medium]
Will this assignment truncate on 32-bit systems?

On 32-bit architectures like PPC32 (which PAMU supports), size_t is 32 bits.
Assigning 1ULL << 36 to mapped_length will silently truncate to 0. While
callers like VFIO might check for 0 and safely fall back to PAGE_SIZE, doesn't
this entirely break the performance optimization intended for PAMU?

> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index f148f559ac564..8df98aceb5f72 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
[ ... ]
> @@ -986,8 +986,9 @@ static unsigned long *get_rto_from_iova(struct s390_domain *domain,
>  	}
>  }
>  
> -static phys_addr_t s390_iommu_iova_to_phys(struct iommu_domain *domain,
> -					   dma_addr_t iova)
> +static phys_addr_t s390_iommu_iova_to_phys_length(struct iommu_domain *domain,
> +						  dma_addr_t iova,
> +						  size_t *mapped_length)
>  {

[Severity: Critical]
This is a pre-existing issue, but does get_rto_from_iova() suffer from a NULL
pointer dereference when queried with unmapped IOVAs?

If the first-level table entry is invalid (which is normal for an unmapped
IOVA), get_rso_from_iova() returns NULL. When get_rto_from_iova() fails to
check this and immediately evaluates READ_ONCE(rso[rsx]), does it cause a
kernel panic?

Since the iova_to_phys_length API is exposed to external callers like VFIO,
could this be triggered by a guest or userspace?

> diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
> index c1a34445d244f..da14574a64308 100644
> --- a/drivers/iommu/sprd-iommu.c
> +++ b/drivers/iommu/sprd-iommu.c
[ ... ]
>  	pa = (pa << SPRD_IOMMU_PAGE_SHIFT) + ((iova - start) & (SPRD_IOMMU_PAGE_SIZE - 1));
>  	spin_unlock_irqrestore(&dom->pgtlock, flags);
>  
> +	if (pa && mapped_length)
> +		*mapped_length = SPRD_IOMMU_PAGE_SIZE;
> +

[Severity: High]
Does this code assign a fake mapped_length for unmapped IOVAs?

In sprd_iommu_iova_to_phys_length(), if the IOVA is unmapped (PTE is 0) but
not page-aligned, pa is incorrectly evaluated to a non-zero offset. By adding
this check, does the new API now formally claim a valid 4KB mapping exists?

Could this mislead the IOMMU core into treating unmapped addresses as valid,
potentially leading to memory corruption or undefined behavior in callers
like VFIO?

> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 587fc13197f12..80c9c06a13804 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
[ ... ]
>  	spin_lock_irqsave(&vdomain->mappings_lock, flags);
>  	node = interval_tree_iter_first(&vdomain->mappings, iova, iova);
>  	if (node) {
>  		mapping = container_of(node, struct viommu_mapping, iova);
>  		paddr = mapping->paddr + (iova - mapping->iova.start);
> +		if (mapped_length)
> +			*mapped_length = mapping->iova.last -
> +					 mapping->iova.start + 1;

[Severity: High]
Can returning a non-power-of-two size here break API consumers?

virtio-iommu computes mapped_length as the exact interval size, which might not
be a power-of-two (e.g., 3 pages). However, callers like vfio_iommu_type1 rely
on mapped_length being a valid power-of-two PTE size to calculate remaining
contiguous bytes via bitwise math:

    pgsize - (iova & (pgsize - 1))

Does providing a non-power-of-two size produce an invalid bitmask resulting in
garbage offset values? If this causes VFIO to overestimate the contiguous block
size and unmap adjacent, potentially unrelated memory, could it lead to memory
corruption?

Additionally, would an identity mapping spanning the 64-bit address space
overflow this calculation to 0?

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