Re: [PATCH v2 2/3] iommu/msm: use the IOMMU device for page table allocation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unchecked `list_first_entry` usage on potentially empty `ctx_list` causes out-of-bounds reads and fundamentally breaks support for multiple masters.
- [Critical] Adding an IOMMU device to a domain's list overwrites shared list pointers, corrupting lists if attached to multiple domains.
- [Critical] Hardware registers are accessed during probe before bus clocks are enabled.
- [High] I/O page tables are permanently leaked when an IOMMU domain is freed.
- [High] Attaching multiple devices to a domain unconditionally re-initializes locks and reallocates page tables, leaking memory and breaking domain consistency.
- [High] The `io_pgtable_ops` structure is leaked on error paths during device attachment.
- [High] IOMMU clocks are left persistently enabled if device attachment fails.
--

--- Patch [2]: [PATCH v2 2/3] iommu/msm: use the IOMMU device for page table allocation ---
commit e64cb01e4df1d4190662ca41772b64a8225c289f
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Subject: iommu/msm: use the IOMMU device for page table allocation

This patch updates the MSM IOMMU driver to allocate page tables using the
IOMMU device rather than the client device. This prevents issues on ARM32
where the client device's DMA ops might map tables through the very IOMMU
they describe. It locates the IOMMU instance before configuring the domain
and passes its device to io-pgtable.

> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index d6ef17cf6d108..30e2d061ecd1d 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c

[ ... ]

> @@ -336,7 +335,8 @@ static void msm_iommu_domain_free(struct iommu_domain *domain)
>  	spin_unlock_irqrestore(&msm_iommu_lock, flags);
>  }

[Severity: High]
This is a pre-existing issue, but does msm_iommu_domain_free() permanently
leak the dynamically allocated page tables?

msm_iommu_domain_free() frees the priv container via kfree(priv) but does not
call free_io_pgtable_ops(priv->iop) to release the io-pgtable resources. Will
this lead to a permanent memory leak when the domain is freed?

>  
> -static int msm_iommu_domain_config(struct msm_priv *priv)
> +static int msm_iommu_domain_config(struct msm_priv *priv,
> +				   struct msm_iommu_dev *iommu)
>  {

[ ... ]

> @@ -400,8 +400,15 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev,
>  	struct msm_priv *priv = to_msm_priv(domain);
>  	struct msm_iommu_ctx_dev *master;
>  
> -	priv->dev = dev;
> -	msm_iommu_domain_config(priv);
> +	spin_lock_irqsave(&msm_iommu_lock, flags);
> +	iommu = find_iommu_for_dev(dev);
> +	spin_unlock_irqrestore(&msm_iommu_lock, flags);
> +	if (!iommu)
> +		return -ENODEV;
> +
> +	ret = msm_iommu_domain_config(priv, iommu);

[Severity: High]
This is a pre-existing issue, but does unconditionally calling
msm_iommu_domain_config() on every device attach corrupt the shared domain 
state?

When multiple devices are attached to an active IOMMU domain, 
msm_iommu_attach_dev() executes msm_iommu_domain_config() for every attach.
This blindly executes spin_lock_init(&priv->pgtlock) and reassigns priv->iop
without freeing the previous allocation.

Could this permanently leak the first device's page tables and force the
driver to track a new structure that does not match the hardware state?

Additionally, there are a few pre-existing regressions in this file that might
warrant review:

[Severity: Critical]
This is a pre-existing issue, but does the list lookup in 
msm_iommu_attach_dev() safely handle an empty ctx_list?

drivers/iommu/msm_iommu.c:msm_iommu_attach_dev() {
    ...
	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node) {
		master = list_first_entry(&iommu->ctx_list,
					  struct msm_iommu_ctx_dev,
					  list);
		if (master->of_node == dev->of_node) {
    ...
}

If an IOMMU probes with no masters yet, could list_first_entry() generate a 
bogus pointer and cause an out-of-bounds read on master->of_node? 

Also, if an IOMMU has multiple masters, does this lookup inadvertently limit
matching to only the first appended master?

[Severity: High]
This is a pre-existing issue, but are the io-pgtable allocations permanently
leaked on the error path during device attachment?

drivers/iommu/msm_iommu.c:msm_iommu_attach_dev() {
    ...
				master->num =
					msm_iommu_alloc_ctx(iommu->context_map,
							    0, iommu->ncb);
				if (IS_ERR_VALUE(master->num)) {
					ret = -ENODEV;
					goto fail;
				}
    ...
fail:
	spin_unlock_irqrestore(&msm_iommu_lock, flags);

	return ret;
}

Since msm_iommu_domain_config() already succeeded in allocating priv->iop, 
does jumping to the fail label and returning without unwinding priv->iop 
permanently leak these resources?

[Severity: Critical]
This is a pre-existing issue, but does adding the IOMMU device to a domain's
list inadvertently corrupt the list if it is attached to multiple domains?

drivers/iommu/msm_iommu.c:msm_iommu_attach_dev() {
    ...
			__disable_clocks(iommu);
			list_add(&iommu->dom_node, &priv->list_attached);
		}
	}
    ...
}

Since dom_node is a single list_head in the shared msm_iommu_dev structure, 
would attaching a second domain to the master forcefully overwrite its
next and prev pointers, breaking the first domain's list?

[Severity: High]
This is a pre-existing issue, but are the IOMMU clocks left persistently
enabled if device attachment fails?

drivers/iommu/msm_iommu.c:msm_iommu_attach_dev() {
    ...
		if (master->of_node == dev->of_node) {
			ret = __enable_clocks(iommu);
			if (ret)
				goto fail;
            ...
				if (IS_ERR_VALUE(master->num)) {
					ret = -ENODEV;
					goto fail;
				}
    ...
fail:
	spin_unlock_irqrestore(&msm_iommu_lock, flags);

	return ret;
}

If an error occurs after __enable_clocks(), does jumping to the fail label
without calling __disable_clocks() leave the clocks unbalanced and drain
power?

[Severity: Critical]
This is a pre-existing issue, but are hardware registers accessed during probe
before the bus clocks are enabled?

drivers/iommu/msm_iommu.c:msm_iommu_probe() {
    ...
	msm_iommu_reset(iommu->base, iommu->ncb);
	SET_M(iommu->base, 0, 1);
	SET_PAR(iommu->base, 0, 0);
    ...
}

If the hardware is in a power-managed domain, would accessing unclocked 
registers synchronously trigger an external abort and panic the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-fix-qcom-smmu-v2-0-18e0daf2d836@oss.qualcomm.com?part=2