Re: [PATCH v3 10/10] iommu/tegra-smmu: Use a DMA API default domain per swgroup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iommu/tegra-smmu: tegra_smmu_attach_dev() leaks old paging domain resources
- [High] iommu/tegra-smmu: NULL dereference in tegra_smmu_identity_attach() on probe failure
--

commit 82c4953fbede4f90881980155ea701c19e530777
Author: Mikko Perttunen <mperttunen@nvidia.com>

iommu/tegra-smmu: Use a DMA API default domain per swgroup

This commit enables IOMMU_DOMAIN_DMA default domains on a per-swgroup
basis via a use_dma_api flag, initially enabled for TegraDRM and Host1x
devices, and sets .release_domain to tegra_smmu_identity_domain.

> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 6779528a9e7cc..73c6151db8f8c 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -994,16 +994,20 @@ static int tegra_smmu_of_xlate(struct device *dev,
>  
>  static int tegra_smmu_def_domain_type(struct device *dev)
>  {
> -	/*
> -	 * FIXME: For now we want to run all translation in IDENTITY mode, due
> -	 * to some device quirks. Better would be to just quirk the troubled
> -	 * devices.
> -	 */
> -	return IOMMU_DOMAIN_IDENTITY;
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +	struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> +	const struct tegra_smmu_swgroup *swgroup;
> +
> +	swgroup = tegra_smmu_find_swgroup(smmu, fwspec->ids[0]);
> +	if (WARN_ON(!swgroup) || !swgroup->use_dma_api)
> +		return IOMMU_DOMAIN_IDENTITY;
> +
> +	return IOMMU_DOMAIN_DMA;
>  }

[Severity: High]
Does transitioning between two paging domains leak the old domain's as->id
and as->pd_dma?

When tegra_smmu_def_domain_type() returns IOMMU_DOMAIN_DMA,
group->default_domain is a paging domain (struct tegra_smmu_as) rather than
tegra_smmu_identity_domain.

When switching between two paging domains (for example, attaching or
detaching an explicit unmanaged domain or a blocking domain over the
IOMMU_DOMAIN_DMA default domain, or changing the default domain type via
sysfs), the core invokes tegra_smmu_attach_dev(domain, dev, old). However,
tegra_smmu_attach_dev() ignores old and only prepares and enables the new
domain:

drivers/iommu/tegra-smmu.c:tegra_smmu_attach_dev() {
	...
	for (index = 0; index < fwspec->num_ids; index++) {
		err = tegra_smmu_as_prepare(smmu, as);
		if (err)
			goto disable;

		tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
	}
	...
}

Because tegra_smmu_as_unprepare() is only called from
tegra_smmu_identity_attach(), the replaced paging domain's as->use_count is
never decremented. When that domain is later freed via
tegra_smmu_domain_free(), it triggers WARN_ON_ONCE(as->use_count) and
permanently leaks its hardware ASID in smmu->asids and its page directory
DMA mapping as->pd_dma.

>  
>  static const struct iommu_ops tegra_smmu_ops = {
>  	.identity_domain = &tegra_smmu_identity_domain,
> +	.release_domain = &tegra_smmu_identity_domain,

[Severity: High]
Can setting .release_domain to tegra_smmu_identity_domain cause a NULL
pointer dereference in tegra_smmu_identity_attach() if attaching the
default domain fails during probe?

During __iommu_probe_device() -> iommu_setup_default_domain(), a new
IOMMU_DOMAIN_DMA domain is allocated with as->smmu = NULL and
as->use_count = 0, and attached via:

  __iommu_group_set_domain_internal(group, dom, IOMMU_SET_DOMAIN_MUST_SUCCEED)

If tegra_smmu_attach_dev() fails inside tegra_smmu_as_prepare() (for
example, if tegra_smmu_alloc_asid() returns -ENOSPC when all 4 hardware
ASIDs on Tegra30/Tegra114 are in use, or dma_map_single() fails), as->smmu
remains NULL. Because IOMMU_SET_DOMAIN_MUST_SUCCEED is passed,
__iommu_group_set_domain_internal() still assigns group->domain = dom
before returning the error.

On the error unwind path in __iommu_probe_device(), iommu_deinit_device()
attaches ops->release_domain while passing group->domain as old, which
calls tegra_smmu_identity_attach():

drivers/iommu/tegra-smmu.c:tegra_smmu_identity_attach() {
	...
	if (old == identity_domain || !old)
		return 0;

	as = to_smmu_as(old);
	smmu = as->smmu;
	for (index = 0; index < fwspec->num_ids; index++) {
		tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
		tegra_smmu_as_unprepare(smmu, as);
	}
	return 0;
}

Here, smmu is NULL, which is then dereferenced in tegra_smmu_disable() and
tegra_smmu_as_unprepare(). Similarly, if a second device joining an already
active shared group fails in __iommu_probe_device() before attaching to
group->domain, iommu_deinit_device() will call tegra_smmu_identity_attach()
and unprepare the shared group->domain even though the failing device never
prepared it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-b4-arm32-iommu-dma-cookie-v3-0-3901d74b1ff2@nvidia.com?part=10