Re: [PATCH v3 07/10] iommu: Enable DMA default domains on (most) 32-bit ARM platforms

[sashiko-bot@kernel.org]

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 <mperttunen@nvidia.com>

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/20261026-b4-arm32-iommu-dma-cookie-v3-0-3901d74b1ff2@nvidia.com?part=7