Re: [PATCH v3 19/32] iommu/sprd: implement iova_to_phys_length

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unchecked dereference of lazily-allocated `dom->pgt_va` when called on an unattached domain.
- [High] Missing memory barrier before IOMMU TLB invalidate allows the hardware to fetch stale page table entries.
- [High] Physical addresses above 4GB are silently truncated before being mapped into the IOMMU page table.
--

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

iommu/sprd: implement iova_to_phys_length

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

> diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
> index c1a34445d244f..3c04ec040d96e 100644
> --- a/drivers/iommu/sprd-iommu.c
> +++ b/drivers/iommu/sprd-iommu.c

[ ... ]

> @@ -376,13 +377,19 @@ static phys_addr_t sprd_iommu_iova_to_phys(struct iommu_domain *domain,
>  	unsigned long end = domain->geometry.aperture_end;
>  
>  	if (WARN_ON(iova < start || iova > end))
> -		return 0;
> +		return PHYS_ADDR_MAX;
>  
>  	spin_lock_irqsave(&dom->pgtlock, flags);
>  	pa = *(dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT));

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a NULL
pointer dereference if called on an unattached domain?

Since dom->pgt_va is lazily allocated during sprd_iommu_attach_device(),
dom->pgt_va will be NULL if the IOMMU API queries the mapping on an unattached
domain (such as via a debug interface or an error recovery path).

> -	pa = (pa << SPRD_IOMMU_PAGE_SHIFT) + ((iova - start) & (SPRD_IOMMU_PAGE_SIZE - 1));
>  	spin_unlock_irqrestore(&dom->pgtlock, flags);
>  
> +	if (!pa)
> +		return PHYS_ADDR_MAX;
> +
> +	pa = (pa << SPRD_IOMMU_PAGE_SHIFT) + ((iova - start) & (SPRD_IOMMU_PAGE_SIZE - 1));
> +	if (mapped_length)
> +		*mapped_length = SPRD_IOMMU_PAGE_SIZE;
> +
>  	return pa;
>  }

I also noticed a few other pre-existing issues in this file:

[Severity: High]
This is a pre-existing issue, but is there a missing memory barrier before the
IOMMU TLB invalidate in sprd_iommu_sync_map()?

drivers/iommu/sprd-iommu.c:sprd_iommu_sync_map() {
    ...
	/* clear IOMMU TLB buffer after page table updated */
	sprd_iommu_write(dom->sdev, reg, 0xffffffff);
	return 0;
}

Because sprd_iommu_write() uses writel_relaxed(), it omits the write memory
barrier. Since the CPU updates the page table entries using standard memory
writes in sprd_iommu_map(), there is no memory barrier (wmb() or dma_wmb())
ordering these memory writes before the MMIO register write.

Could the CPU reorder the MMIO write before the page table writes are globally
visible, causing the IOMMU device to fetch stale PTEs?


[Severity: High]
This is a pre-existing issue, but does sprd_iommu_map() silently truncate
physical addresses above 4GB?

drivers/iommu/sprd-iommu.c:sprd_iommu_map() {
    ...
	u32 pabase = (u32)paddr;
    ...
}

The 64-bit phys_addr_t paddr is explicitly cast to a 32-bit integer before
computing the Page Frame Number (PFN), discarding the upper 32 bits.

If the target memory resides above the 4GB boundary, wouldn't the IOMMU be
programmed to map the device DMA to an incorrect lower physical memory address?

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