Re: [PATCH v3 14/32] 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:

Pre-existing issues:
- [High] When `__enable_clocks()` fails, its negative error code is incorrectly returned cast to `phys_addr_t` instead of `PHYS_ADDR_MAX`.
- [High] Unsafe `list_first_entry()` on a potentially empty list leads to out-of-bounds memory accesses.
- [High] Concurrent traversal and modification of `priv->list_attached` without adequate common locking.
--

commit 48fb553fe9efe8fd4fc6286128070ec03e449c35
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..1038e8141223f 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -523,15 +523,16 @@ 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)
>  {
>  	struct msm_priv *priv;
>  	struct msm_iommu_dev *iommu;
>  	struct msm_iommu_ctx_dev *master;
>  	unsigned int par;
>  	unsigned long flags;
> -	phys_addr_t ret = 0;
> +	phys_addr_t ret = PHYS_ADDR_MAX;

[Severity: High]
This isn't a bug introduced by this patch, but if
msm_iommu_iova_to_phys_length() is called on an IOMMU domain before any
devices are attached, priv->list_attached would be empty. In that case,
does list_first_entry() calculate an invalid pointer?

If so, would dereferencing this pointer in the subsequent
list_empty(&iommu->ctx_list) and __enable_clocks(iommu) calls lead to
out-of-bounds memory accesses?

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but when __enable_clocks() fails, it returns a negative integer
error code. Since ret is initialized to PHYS_ADDR_MAX but gets overwritten
shortly after:

msm_iommu_iova_to_phys_length() {
        ...
        ret = __enable_clocks(iommu);
        if (ret)
                goto fail;
        ...

Could this negative error code be incorrectly cast to phys_addr_t and
returned on the failure path instead of PHYS_ADDR_MAX?

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of
concurrent modification and traversal of priv->list_attached?

It looks like msm_iommu_attach_dev() modifies the list using list_add()
while holding only the msm_iommu_lock spinlock. However,
msm_iommu_sync_map() traverses this same list locklessly by calling
__flush_iotlb_range() without taking any locks, and msm_iommu_unmap()
traverses it holding only priv->pgtlock. Can this lack of synchronization
lead to list corruption?

>  	spin_lock_irqsave(&msm_iommu_lock, flags);
>  

[ ... ]

> @@ -558,13 +559,19 @@ 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;
> +	}
>  
>  	if (GET_FAULT(iommu->base, master->num))
> -		ret = 0;
> +		ret = PHYS_ADDR_MAX;
>  
>  	__disable_clocks(iommu);
>  fail:

[ ... ]

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