Re: [PATCH v2 14/30] iommu/msm: 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] Incorrect mapping length hardcoded to `SZ_4K` for 64KB and 1MB mappings causes unmap failures and resource leaks.

Pre-existing issues:
- [High] Negative error code from `__enable_clocks()` is returned as a valid physical address, bypassing IOMMU API failure checks.
- [High] Unsafe `list_first_entry()` on a potentially empty list causes invalid pointer dereference.
--

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

iommu/msm: implement iova_to_phys_length

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

> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index 0ad5ff431d5b6..af4020ed99876 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -523,15 +523,19 @@ static size_t msm_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
>  	return ret;
>  }
>  
> -static phys_addr_t msm_iommu_iova_to_phys(struct iommu_domain *domain,
> -					  dma_addr_t va)
> +static phys_addr_t msm_iommu_iova_to_phys_length(struct iommu_domain *domain,
> +						 dma_addr_t va,
> +						 size_t *mapped_length)
>  {

[Severity: High]
This is a pre-existing issue, but does list_first_entry() return a valid
pointer if the list_attached list is empty?

If a domain has no devices attached, such as directly after domain
allocation, the list will be empty. Because list_first_entry() does not
check for emptiness, it seems this could result in a bogus pointer being
dereferenced during the subsequent list_empty() check on the ctx_list:

msm_iommu.c:msm_iommu_iova_to_phys_length() {
        ...
        priv = to_msm_priv(domain);
        iommu = list_first_entry(&priv->list_attached,
                                 struct msm_iommu_dev, dom_node);

        if (list_empty(&iommu->ctx_list))
                goto fail;
        ...
}

[Severity: High]
This is also a pre-existing issue, but does the failure path for
__enable_clocks() unintentionally return a negative error code as a valid
physical address?

When __enable_clocks() fails, it returns a negative integer like -EINVAL.
This is assigned to ret, which is of type phys_addr_t. Since the code then
jumps to the fail label and returns ret, the unsigned cast of the negative
error code won't match the PHYS_ADDR_MAX failure sentinel:

msm_iommu.c:msm_iommu_iova_to_phys_length() {
        ...
        ret = __enable_clocks(iommu);
        if (ret)
                goto fail;
        ...
fail:
        spin_unlock_irqrestore(&msm_iommu_lock, flags);
        return ret;
}

Could this cause the IOMMU API to interpret the failure as a valid mapping?

>  	struct msm_priv *priv;
>  	struct msm_iommu_dev *iommu;
>  	struct msm_iommu_ctx_dev *master;

[ ... ]

> @@ -558,13 +562,22 @@ static phys_addr_t msm_iommu_iova_to_phys(struct iommu_domain *domain,
>  	par = GET_PAR(iommu->base, master->num);
>  
>  	/* We are dealing with a supersection */
> -	if (GET_NOFAULT_SS(iommu->base, master->num))
> +	if (GET_NOFAULT_SS(iommu->base, master->num)) {
>  		ret = (par & 0xFF000000) | (va & 0x00FFFFFF);
> -	else	/* Upper 20 bits from PAR, lower 12 from VA */
> +		if (mapped_length)
> +			*mapped_length = SZ_16M;
> +	} else {
> +		/* Upper 20 bits from PAR, lower 12 from VA */
>  		ret = (par & 0xFFFFF000) | (va & 0x00000FFF);
> +		if (mapped_length)
> +			*mapped_length = SZ_4K;
> +	}

[Severity: High]
Does setting mapped_length to SZ_4K for all non-supersection mappings cause
issues with 64KB and 1MB mappings?

The hardware uses the ARM_V7S short descriptor format where the PAR register
doesn't explicitly differentiate between 1MB section, 64KB large page, and
4KB page mappings. If mapped_length is hardcoded to SZ_4K here, unmap
requests for larger regions might fail.

The underlying arm_v7s_unmap() pagetable code rejects partial unmaps of
larger sections. Could this lead to leaked memory and IOVA space?

[ ... ]

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