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 <[email protected]> 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/[email protected]?part=10
