Re: [PATCH v3 27/32] drm/panthor: switch to iova_to_phys_length

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The `iommufd` page batching incorrectly uses `pgsize` returned by `iommu_iova_to_phys_length` as the contiguous length from an unaligned IOVA, leading to physical page boundary overshoots and unpinning of incorrect pages.

Pre-existing issues:
- [High] The fallback loop in `panthor_vm_unmap_pages` incorrectly queries `iova + unmapped_sz` instead of `iova + offset + unmapped_sz`, leading to incorrect address checks and silent page table leaks when `offset > 0`.
--

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

drm/panthor: switch to iova_to_phys_length

Migrate panthor_mmu to use ops->iova_to_phys_length(ops, iova, NULL)
instead of the deprecated ops->iova_to_phys.

> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 75d98dad7b1dd..3b635fc1f6518 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -903,7 +903,7 @@ static void panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)
>  			 * are out-of-sync. This is not supposed to happen, hence the
>  			 * above WARN_ON().
>  			 */
> -			while (!ops->iova_to_phys(ops, iova + unmapped_sz) &&
> +			while (ops->iova_to_phys_length(ops, iova + unmapped_sz, NULL) == PHYS_ADDR_MAX &&
>  			       unmapped_sz < pgsize * pgcount)
>  				unmapped_sz += SZ_4K;

[Severity: High]
This is a pre-existing issue, but does this fallback loop in
panthor_vm_unmap_pages() incorrectly query the page table when offset > 0?

By passing iova + unmapped_sz instead of iova + offset + unmapped_sz, it seems
this completely omits the block's offset. Since earlier blocks might have been
successfully unmapped, won't iova_to_phys_length() spuriously return
PHYS_ADDR_MAX?

This could cause the fallback loop to improperly advance unmapped_sz to its
maximum (pgsize * pgcount), blindly skipping the failing block without
unmapping any remaining mapped pages inside it, permanently leaking page table
entries and defeating the recovery logic.

I also noticed an issue in drivers/iommu/iommufd/pages.c that appears to
have the same root cause.

In batch_from_domain():

>		phys = iommu_iova_to_phys_length(domain, iova - page_offset,
>						  &pgsize);
>		if (WARN_ON(phys == PHYS_ADDR_MAX))
>			break;
>		if (WARN_ON(!pgsize || pgsize < PAGE_SIZE))
>			pgsize = PAGE_SIZE;
>
>		/*
>		 * pgsize is the contiguous length from the page-aligned
>		 * iova, so npages is simply pgsize / PAGE_SIZE.
>		 */
>		npages = pgsize / PAGE_SIZE;
>		npages = min_t(unsigned long, npages,
>			       last_index - start_index + 1);

[Severity: Critical]
Does this calculation blindly use the full block size (pgsize) without
subtracting the offset within the block?

Most IOMMU page table implementations return the full block size of the
underlying page table entry, regardless of whether iova is unaligned.
If queried at an offset of 4KB into a 2MB block, iommu_iova_to_phys_length()
returns phys corresponding to the 4KB offset and pgsize = 2M.

Since iommufd allows unmapping or tracking sub-regions, start_index can
correspond to an IOVA in the middle of a huge page. The code then computes
npages = 2M / 4K = 512 and iterates from i = 0 to 511, adding
PHYS_PFN(phys) + i.

Since there are only 511 pages remaining in the block, won't this loop
overshoot the physical block boundary by 1 page, incorrectly appending the
first page of the adjacent physical block?

When these collected PFNs are later passed to batch_unpin(), the kernel will
call unpin_user_page() on a page that is not part of the mapping, decrementing
its refcount. Could this create a use-after-free vulnerability?

The identical logic flaw exists in raw_pages_from_domain():

>		phys = iommu_iova_to_phys_length(domain, iova - page_offset,
>						  &pgsize);
>		if (WARN_ON(phys == PHYS_ADDR_MAX))
>			break;
>		if (WARN_ON(!pgsize || pgsize < PAGE_SIZE))
>			pgsize = PAGE_SIZE;
>
>		npages = pgsize / PAGE_SIZE;
>		npages = min_t(unsigned long, npages,
>			       last_index - start_index + 1);

[Severity: Critical]
Does this code also overestimate the remaining contiguous pages when reading
from an unaligned IOVA?

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