Re: [PATCH 04/12] iommu: qcom_iommu: support the short-descriptor pagetable format

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing rollback in qcom_iommu_init_domain leaves dangling ctx->domain pointers and active contexts, causing UAF in the fault handler.
- [High] Missing reset of `iova` base address in TLB invalidation loop causes missed TLB flushes for subsequent context banks.
- [High] Unprogrammed contexts cause NULL pointer dereference in fault handler.
--

--- Patch [4]: [PATCH 04/12] iommu: qcom_iommu: support the short-descriptor pagetable format ---
commit 058d015d111c3926c8cd1da34f2ceba59b5078db
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

iommu: qcom_iommu: support the short-descriptor pagetable format
    
The MSM8974-generation QSMMU does not walk the LPAE tables this
driver hardcodes: every access faults with an access flag fault
despite AF being set in the descriptors.

> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 5bbb51d9bd653..77c01f217d7ea 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c

[ ... ]

> @@ -298,12 +318,22 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>  			continue;
>  		}
>  
> -		ctx->ttbr0 = pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
> -			     FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid);
> -		ctx->tcr[0] = arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE;
> -		ctx->tcr[1] = arm_smmu_lpae_tcr2(&pgtbl_cfg);
> -		ctx->mair[0] = pgtbl_cfg.arm_lpae_s1_cfg.mair;
> -		ctx->mair[1] = pgtbl_cfg.arm_lpae_s1_cfg.mair >> 32;
> +		if (fmt == ARM_V7S) {
> +			ctx->ttbr0 = pgtbl_cfg.arm_v7s_cfg.ttbr;
> +			ctx->tcr[0] = pgtbl_cfg.arm_v7s_cfg.tcr;
> +			ctx->tcr[1] = 0;
> +			/* PRRR/NMRR share the MAIR0/MAIR1 offsets */
> +			ctx->mair[0] = pgtbl_cfg.arm_v7s_cfg.prrr;
> +			ctx->mair[1] = pgtbl_cfg.arm_v7s_cfg.nmrr;
> +			ctx->contextidr = ctx->asid;
> +		} else {
> +			ctx->ttbr0 = pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
> +				     FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid);
> +			ctx->tcr[0] = arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE;
> +			ctx->tcr[1] = arm_smmu_lpae_tcr2(&pgtbl_cfg);
> +			ctx->mair[0] = pgtbl_cfg.arm_lpae_s1_cfg.mair;
> +			ctx->mair[1] = pgtbl_cfg.arm_lpae_s1_cfg.mair >> 32;
> +		}
>  
>  		reg = ARM_SMMU_SCTLR_CFIE | ARM_SMMU_SCTLR_CFRE |
>  		      ARM_SMMU_SCTLR_AFE | ARM_SMMU_SCTLR_TRE |

[Severity: High]
This isn't a bug introduced by this patch, but does qcom_iommu_init_domain()
properly roll back if a later context bank fails to initialize?

If qcom_scm_restore_sec_cfg() fails for a later context bank, it jumps to the
out_clear_iommu error path without un-programming the earlier context banks
or resetting their ctx->domain pointers to NULL:

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_init_domain() {
...
			ret = qcom_scm_restore_sec_cfg(qcom_iommu->sec_id, ctx->asid);
			if (ret) {
				dev_err(qcom_iommu->dev, "secure init failed: %d\n", ret);
				goto out_clear_iommu;
			}
...
}

If a hardware fault subsequently occurs on one of the still-active context
banks, would qcom_iommu_fault() pass the dangling ctx->domain pointer to
report_iommu_fault(), potentially causing a use-after-free?

[Severity: High]
This is a pre-existing issue, but does qcom_iommu_tlb_inv_range_nosync() miss
TLB flushes for subsequent context banks?

In qcom_iommu_tlb_inv_range_nosync(), when there are multiple context banks,
the iova variable is incremented in the inner loop but isn't reset for the
next iteration of the outer loop:

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_tlb_inv_range_nosync() {
...
		iova = (iova >> 12) << 12;
		iova |= ctx->asid;
		do {
			iommu_writel(ctx, reg, iova);
			iova += granule;
		} while (s -= granule);
...
}

Could this heavily incremented iova value cause missed TLB flushes for
subsequent context banks, leading to stale TLB translations and incorrect
memory accesses?

[Severity: High]
This is also a pre-existing issue, but can unprogrammed contexts cause a NULL
pointer dereference in the fault handler?

During qcom_iommu_ctx_probe(), if a bootloader left the context active, a DMA
fault can fire before qcom_iommu_attach_dev() initializes the context and sets
ctx->domain. If this occurs, qcom_iommu_fault() passes a NULL ctx->domain to
report_iommu_fault():

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_fault() {
...
	if (report_iommu_fault(ctx->domain, ctx->dev, iova, 0)) {
...
}

Which then dereferences the domain pointer without checking if it is NULL:

drivers/iommu/iommu.c:report_iommu_fault() {
...
	if (domain->cookie_type == IOMMU_COOKIE_FAULT_HANDLER &&
	    domain->handler)
...
}

Would this lead to a kernel panic from a NULL pointer dereference during
device initialization?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809-msm8974-iommu-upstream-v1-0-87f5cd492560@oss.qualcomm.com?part=4