Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] iommu: missing IDENTITY default domain override for qcom_iommu on ARM32 - [Critical] iommu/rockchip: use-after-free on device teardown with DMA default domain --
commit b1b0900ebc55a49be25130b8187ffecfc2191b42 Author: Mikko Perttunen <[email protected]> iommu: Enable DMA default domains on (most) 32-bit ARM platforms This commit enables core-owned DMA default domains on 32-bit ARM platforms with CONFIG_ARM_DMA_USE_IOMMU by removing the IOMMU_DOMAIN_IDENTITY override in iommu_get_default_domain_type(). For several IOMMU drivers that are not ready to switch over, it adds a def_domain_type callback returning IOMMU_DOMAIN_IDENTITY. > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 5fbb76d44a1e9..e47f95fea44d0 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1884,18 +1884,6 @@ static int iommu_get_default_domain_type(struct > iommu_group *group, > > lockdep_assert_held(&group->mutex); > > - /* > - * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an > - * identity_domain and it will automatically become their default > - * domain. Later on ARM_DMA_USE_IOMMU will install its UNMANAGED domain. > - * Override the selection to IDENTITY. > - */ > - if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) { > - static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && > - IS_ENABLED(CONFIG_IOMMU_DMA))); > - driver_type = IOMMU_DOMAIN_IDENTITY; > - } > - [Severity: Critical] Should qcom_iommu also keep an IOMMU_DOMAIN_IDENTITY default domain on 32-bit ARM platforms? CONFIG_QCOM_IOMMU selects CONFIG_ARM_DMA_USE_IOMMU, and qcom_iommu_ops in drivers/iommu/arm/arm-smmu/qcom_iommu.c does not define a def_domain_type callback or release_domain: drivers/iommu/arm/arm-smmu/qcom_iommu.c: static const struct iommu_ops qcom_iommu_ops = { .identity_domain = &qcom_iommu_identity_domain, .capable = qcom_iommu_capable, .domain_alloc_paging = qcom_iommu_domain_alloc_paging, With this change, iommu_setup_default_domain() allocates and attaches a paging IOMMU_DOMAIN_DMA group->default_domain via qcom_iommu_attach_dev() -> qcom_iommu_init_domain(), which sets qcom_domain->iommu, programs ARM_SMMU_CB_TTBR0 and ARM_SMMU_CB_SCTLR, and sets ctx->domain = group->default_domain. When a client driver such as drm/msm in msm_iommu_new() (drivers/gpu/drm/msm/msm_iommu.c) or Venus in venus_firmware_init() (drivers/media/platform/qcom/venus/firmware.c) attaches its own unmanaged paging domain, arm_iommu_detach_device() and arm_iommu_release_mapping() become no-ops because iommu_is_dma_domain(mapping->domain) is now true, leaving dev->dma_ops set to &iommu_ops instead of restoring direct DMA ops. Attaching the client's unmanaged domain then overwrites the context bank registers and sets ctx->domain to the unmanaged domain. When that unmanaged domain is later detached and freed (for example via mdp5_kms_destroy() -> msm_iommu_detach() followed by drm_gpuvm_put() -> msm_gem_vm_free() -> msm_iommu_destroy() in drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c), __iommu_group_set_core_domain() attempts to re-attach group->default_domain through qcom_iommu_attach_dev() -> qcom_iommu_init_domain(): drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_init_domain() { mutex_lock(&qcom_domain->init_mutex); if (qcom_domain->iommu) goto out_unlock; ... } Because qcom_domain->iommu is already non-NULL from the initial boot attach, qcom_iommu_init_domain() returns early without reprogramming ARM_SMMU_CB_TTBR0 or ARM_SMMU_CB_SCTLR and without updating ctx->domain. This leaves ARM_SMMU_CB_TTBR0 pointing to the freed page tables with translation still enabled in ARM_SMMU_CB_SCTLR, and leaves ctx->domain pointing to the freed unmanaged domain where qcom_iommu_fault() will dereference it on a context fault: drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_fault() { ... if (report_iommu_fault(ctx->domain, ctx->dev, iova, 0)) { ... } Similarly, during iommu_deinit_device(), because qcom_iommu_ops does not set release_domain = &qcom_iommu_identity_domain, group->default_domain is freed while the hardware context bank remains enabled and ctx->domain remains a dangling pointer. > for_each_group_device(group, gdev) { > driver_type = iommu_get_def_domain_type(group, gdev->dev, > driver_type); > @@ -1912,11 +1900,11 @@ static int iommu_get_default_domain_type(struct > iommu_group *group, > } > > /* > - * If the common dma ops are not selected in kconfig then we cannot use > - * IOMMU_DOMAIN_DMA at all. Force IDENTITY if nothing else has been > - * selected. > + * Without an IOMMU DMA implementation, force IDENTITY if the driver > + * has not selected a domain type. > */ > - if (!IS_ENABLED(CONFIG_IOMMU_DMA)) { > + if (!IS_ENABLED(CONFIG_IOMMU_DMA) && > + !IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) { [Severity: Critical] Can switching rockchip-iommu to an IOMMU_DOMAIN_DMA default domain on 32-bit ARM cause a use-after-free during device teardown? CONFIG_ROCKCHIP_IOMMU selects CONFIG_ARM_DMA_USE_IOMMU, and rk_iommu_ops in drivers/iommu/rockchip-iommu.c defines neither def_domain_type nor release_domain = &rk_identity_domain: drivers/iommu/rockchip-iommu.c: static void rk_iommu_release_device(struct device *dev) { struct rk_iommudata *data = dev_iommu_priv_get(dev); device_link_del(data->link); } ... static const struct iommu_ops rk_iommu_ops = { .identity_domain = &rk_identity_domain, .domain_alloc_paging = rk_iommu_domain_alloc_paging, .probe_device = rk_iommu_probe_device, .release_device = rk_iommu_release_device, Previously on 32-bit ARM, group->default_domain was the static &rk_identity_domain, and during device teardown arch_teardown_dma_ops() -> arm_teardown_iommu_dma_ops() -> arm_iommu_detach_device() detached the legacy UNMANAGED domain (re-attaching &rk_identity_domain and removing iommu->node from rk_domain->iommus) before freeing it. With this patch, group->default_domain is a dynamically allocated IOMMU_DOMAIN_DMA paging domain attached via rk_iommu_attach_device(), which sets iommu->domain = domain and adds iommu->node to rk_domain->iommus. When a client device is removed or fails probe, arm_teardown_iommu_dma_ops() returns early for DMA default domains, and iommu_deinit_device() sees ops->release_domain == NULL, calls rk_iommu_release_device() without detaching the domain, and then calls iommu_domain_free(group->default_domain) -> rk_iommu_domain_free(): drivers/iommu/rockchip-iommu.c:rk_iommu_domain_free() { struct rk_iommu_domain *rk_domain = to_rk_domain(domain); int i; WARN_ON(!list_empty(&rk_domain->iommus)); ... kfree(rk_domain); } This triggers the WARN_ON() and frees rk_domain and its page directory while iommu->node is still linked on rk_domain->iommus and iommu->domain still points to the freed rk_domain (which is later dereferenced in rk_iommu_enable() and rk_iommu_irq()). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
