Re: [PATCH 5/9] iommu: apple-dart/ipmmu/mtk_iommu implement iova_to_phys_length

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unsafe dereference of `iop` and `bank` pointers without NULL checks when querying an unattached domain.
--

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

iommu: apple-dart/ipmmu/mtk_iommu implement iova_to_phys_length

Migrate remaining io-pgtable user drivers to implement
iova_to_phys_length, passing through mapped_length from io-pgtable.

> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c

[ ... ]

> @@ -699,14 +699,18 @@ static void ipmmu_iotlb_sync(struct iommu_domain *io_domain,
> -static phys_addr_t ipmmu_iova_to_phys(struct iommu_domain *io_domain,
> -				      dma_addr_t iova)
> +static phys_addr_t ipmmu_iova_to_phys_length(struct iommu_domain *io_domain,
> +				      dma_addr_t iova, size_t *mapped_length)
>  {
>  	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
>  
> +	if (mapped_length)
> +		*mapped_length = 0;
> +
>  	/* TODO: Is locking needed ? */
>  
> -	return domain->iop->iova_to_phys(domain->iop, iova);
> +	return domain->iop->iova_to_phys_length(domain->iop, iova,
> +						mapped_length);

[Severity: High]
This is a pre-existing issue, but could this unconditional dereference of
domain->iop cause a panic for unattached domains?

The IOMMUFD framework allows userspace to allocate a hardware page table
which creates an IOMMU domain without immediately attaching it. If a
translation is triggered on this unattached domain, domain->iop will be NULL.

>  }

[ ... ]

> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c

[ ... ]

> @@ -858,13 +858,17 @@ static int mtk_iommu_sync_map(struct iommu_domain *domain, unsigned long iova,
> -static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
> -					  dma_addr_t iova)
> +static phys_addr_t mtk_iommu_iova_to_phys_length(struct iommu_domain *domain,
> +					  dma_addr_t iova, size_t *mapped_length)
>  {
>  	struct mtk_iommu_domain *dom = to_mtk_domain(domain);
>  	phys_addr_t pa;
>  
> -	pa = dom->iop->iova_to_phys(dom->iop, iova);
> +	if (mapped_length)
> +		*mapped_length = 0;
> +
> +	pa = dom->iop->iova_to_phys_length(dom->iop, iova, mapped_length);
> +
>  	if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) &&
>  	    dom->bank->parent_data->enable_4GB &&

[Severity: High]
This is also a pre-existing issue, but does this function safely handle querying
unattached domains without dereferencing NULL dom->iop and dom->bank pointers?

Similar to the ipmmu-vmsa driver, domain initialization is deferred until
attach_dev. If an unattached domain is queried, both pointers would be NULL,
causing a kernel panic. The apple_dart driver prevents this by explicitly
checking the operations pointer first before querying it.

>  	    pa >= MTK_IOMMU_4GB_MODE_REMAP_BASE)

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