Re: [PATCH v3 07/32] iommu/arm-smmu: implement iova_to_phys_length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] arm_smmu_iova_to_phys_hard() returns 0 instead of PHYS_ADDR_MAX on failure, causing memory corruption via unpinned physical page 0.
- [High] arm_smmu_iova_to_phys_length fails to populate mapped_length in the hardware translation path, causing potential DoS via log spam.
- [Medium] qcom_smmu_context_fault uses incorrect error checking (!phys_soft) for the new iova_to_phys_length API.
--

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

iommu/arm-smmu: implement iova_to_phys_length

Migrate ARM SMMU to implement iova_to_phys_length, calling
ops->iova_to_phys_length on the io-pgtable layer. Update qcom-debug
caller accordingly.

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> index 65e0ef6539fe7..4fd01341157f3 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> @@ -415,7 +415,7 @@ irqreturn_t qcom_smmu_context_fault(int irq, void *dev)
>  		return IRQ_HANDLED;
>  	}
>  
> -	phys_soft = ops->iova_to_phys(ops, cfi.iova);
> +	phys_soft = ops->iova_to_phys_length(ops, cfi.iova, NULL);

[Severity: Medium]
Will this result in skipped error reporting later in this function?

Since io-pgtable implementations of iova_to_phys_length now return
PHYS_ADDR_MAX on error instead of 0, the check further down in
qcom_smmu_context_fault:

	if (!phys_soft)
		dev_err(smmu->dev,
			"SOFTWARE TABLE WALK FAILED! Looks like %s accessed an unmapped address!\n",
			dev_name(smmu->dev));

will evaluate to false when the table walk fails. Can this cause
PHYS_ADDR_MAX to be erroneously treated and printed as a successfully
resolved physical address?

[ ... ]

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 0bd21d206eb3e..5c9ec7c937632 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -1366,7 +1366,7 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
>  			"iova to phys timed out on %pad. Falling back to software table walk.\n",
>  			&iova);
>  		arm_smmu_rpm_put(smmu);
> -		return ops->iova_to_phys(ops, iova);
> +		return ops->iova_to_phys_length(ops, iova, NULL);

[Severity: High]
Are we discarding the mapping size by explicitly passing NULL for
mapped_length?

If callers like vfio_unmap_unpin receive a pgsize of 0, will they trigger a
WARN_ON trace for every unmapped page, potentially spamming the kernel log
when unmapping large amounts of memory?

>  	}
>  
>  	phys = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_PAR);
> @@ -1384,20 +1384,21 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
>  	return addr;
>  }
>  
> -static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
> -					dma_addr_t iova)
> +static phys_addr_t arm_smmu_iova_to_phys_length(struct iommu_domain *domain,
> +					dma_addr_t iova, size_t *mapped_length)
>  {
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>  	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>  
> +
>  	if (!ops)
> -		return 0;
> +		return PHYS_ADDR_MAX;
>  
>  	if (smmu_domain->smmu->features & ARM_SMMU_FEAT_TRANS_OPS &&
>  			smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
>  		return arm_smmu_iova_to_phys_hard(domain, iova);

[Severity: High]
Could this delegation to hardware translation fail to populate mapped_length?

Since arm_smmu_iova_to_phys_hard does not take a mapped_length parameter, it
leaves the size unpopulated. Similar to the software fallback above, can
this cause callers to receive a pgsize of 0?

[Severity: Critical]
Is arm_smmu_iova_to_phys_hard still returning 0 on failure?

The updated API requires returning PHYS_ADDR_MAX upon translation failure,
but arm_smmu_iova_to_phys_hard returns 0 when arm_smmu_rpm_get fails, and
also when a translation fault occurs:

	phys = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_PAR);
	spin_unlock_irqrestore(&smmu_domain->cb_lock, flags);
	if (phys & ARM_SMMU_CB_PAR_F) {
		dev_err(dev, "translation fault!\n");
		dev_err(dev, "PAR = 0x%llx\n", phys);
		goto out;
	}

Because 0 is a valid physical address, will callers erroneously treat this
as a successfully mapped address, which could result in unpinning physical
page 0?

>  
> -	return ops->iova_to_phys(ops, iova);
> +	return ops->iova_to_phys_length(ops, iova, mapped_length);
>  }

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