Re: [PATCH v3 22/32] 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:
- [High] viommu_iova_to_phys_length incorrectly returns the total mapping span size instead of the remaining mapped length.
- [High] Calculation of mapped_length wraps around to 0 when a mapping covers the entire address space.

Pre-existing issues:
- [High] On 32-bit systems with 64-bit DMA addresses, the interval tree node silently truncates 64-bit IOVAs to 32 bits, leading to mapping collisions and incorrect physical address translations.
--

commit 4bfdcec317099653f8ccbaeac00ce9bc697a92cf
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
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -912,10 +912,11 @@ static size_t viommu_unmap_pages(struct iommu_domain *domain, unsigned long iova
[ ... ]
> -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;
> @@ -926,6 +927,9 @@ static phys_addr_t viommu_iova_to_phys(struct iommu_domain *domain,
>  	if (node) {
>  		mapping = container_of(node, struct viommu_mapping, iova);
>  		paddr = mapping->paddr + (iova - mapping->iova.start);

[Severity: High]
This isn't a bug introduced by this patch, but on 32-bit systems with 64-bit
DMA addresses, is it possible that interval_tree_node truncates the 64-bit
IOVAs to 32 bits?

Because mapping->iova.start is an unsigned long, mixing the 64-bit iova
input with the truncated 32-bit mapping->iova.start can result in wild
physical address offsets when calculating paddr in
viommu_iova_to_phys_length.

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

[Severity: High]
Can this logic in viommu_iova_to_phys_length overestimate the remaining
valid contiguous length?

By subtracting mapping->iova.start instead of the passed-in iova offset,
this evaluates to the total size of the mapping block. If a caller requests
translation at an offset within the block, returning the full block size can
cause callers to assume subsequent out-of-bounds physical frames are valid
and mapped.

[Severity: High]
Could this calculation in viommu_iova_to_phys_length also wrap around to 0
when a mapping covers the entire address space?

If the identity mapping has start = 0 and last = ULONG_MAX, the result of
ULONG_MAX - 0 + 1 overflows to 0. Returning a mapped_length of 0 can
trigger kernel warnings or infinite loops in iteration callers.

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

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