Re: [PATCH v2 22/30] iommu/virtio: implement iova_to_phys_length

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] viommu_iova_to_phys_length returns the total arbitrary size of the interval tree mapping instead of a valid power-of-two PTE page size, leading to bitwise math errors in callers and out-of-bounds physical memory access.
- [High] The mapped_length calculation overflows to 0 for mappings covering the maximum address space.

Pre-existing issues:
- [Critical] Virtio-IOMMU uses standard `interval_tree` which silently truncates 64-bit `dma_addr_t` IOVAs to 32-bit `unsigned long` on 32-bit systems with LPAE.
--

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

iommu/virtio: implement iova_to_phys_length

Implement iova_to_phys_length for virtio IOMMU driver,
returning the actual PTE mapping size.

> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 587fc13197f12..c90d02cbbfd0d 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
[ ... ]
> -static phys_addr_t viommu_iova_to_phys(struct iommu_domain *domain,
> -				       dma_addr_t iova)
> +static phys_addr_t viommu_iova_to_phys_length(struct iommu_domain *domain,
> +					      dma_addr_t iova,
> +					      size_t *mapped_length)
>  {
> -	u64 paddr = 0;
> +	u64 paddr = PHYS_ADDR_MAX;
>  	unsigned long flags;
>  	struct viommu_mapping *mapping;
>  	struct interval_tree_node *node;
>  	struct viommu_domain *vdomain = to_viommu_domain(domain);
>  
> +	if (mapped_length)
> +		*mapped_length = 0;
> +
>  	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);

[Severity: Critical]
This is a pre-existing issue, but does this calculation in
viommu_iova_to_phys_length() suffer from truncation on 32-bit systems
with LPAE?

The standard interval tree node uses unsigned long for its boundaries.
When assigning a 64-bit dma_addr_t to mapping->iova.start on these
architectures, it is silently truncated to 32 bits.

Mixing the 64-bit iova with the truncated 32-bit start in this subtraction
could produce incorrect physical address offsets and corrupt memory.

> +		if (mapped_length)
> +			*mapped_length = mapping->iova.last -
> +					 mapping->iova.start + 1;

[Severity: Critical]
Does this return an arbitrary block size instead of a valid power-of-two PTE
page size?

The IOMMU API expects mapped_length to output the PTE page size, which is a
power of two. Since virtio-iommu tracks mappings as coalesced interval tree
nodes, this calculation returns the full block size which is not guaranteed
to be a power of two.

Callers like batch_from_domain() in drivers/iommu/iommufd/pages.c rely on
bitwise arithmetic that assumes a power-of-two size:

	npages = (pgsize - (iova & (pgsize - 1)) + page_offset) / PAGE_SIZE;

Passing a non-power-of-two size here calculates an incorrect, larger
remaining length, causing out-of-bounds physical memory accesses.

[Severity: High]
Can this mapped_length calculation overflow and evaluate to zero for mappings
covering the maximum address space?

If an identity mapping spans the entire available address space,
mapping->iova.last will be ULONG_MAX and mapping->iova.start will be 0.
Adding 1 to this difference causes an integer overflow resulting in 0.

While callers defensively handle a 0 return by falling back to PAGE_SIZE, it
triggers WARN_ON(!pgsize) in downstream callers like vfio_iommu_type1.c and
iommufd, which can panic the kernel if panic_on_warn is set.

>  	}
>  	spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
>

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