Re: [PATCH 03/12] iommu: qcom_iommu: extract context bank programming into a helper

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The `qcom_iommu` parameter in the new helper `qcom_iommu_program_ctx` is never used.

Pre-existing issues:
- [High] A use-after-free (UAF) condition exists on the error path of `qcom_iommu_init_domain` where a dangling pointer is left in `ctx->domain` and accessed by the IRQ handler.
--

--- Patch [3]: [PATCH 03/12] iommu: qcom_iommu: extract context bank programming into a helper ---
commit 7d60ff7e8f25f085f5b849ae6ddac1aa47c3809d
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Subject: iommu: qcom_iommu: extract context bank programming into a helper

This patch extracts the inline context bank programming sequence into a new
helper function qcom_iommu_program_ctx(). It also stores the computed
register values into the qcom_iommu_ctx struct. This allows the registers to
be reprogrammed upon runtime resume for MSM8974 QSMMU instances.

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

[ ... ]

> @@ -213,6 +218,32 @@ static irqreturn_t qcom_iommu_fault(int irq, void *dev)
>  	return IRQ_HANDLED;
>  }
>  
> +static void qcom_iommu_program_ctx(struct qcom_iommu_dev *qcom_iommu,
> +				   struct qcom_iommu_ctx *ctx)
> +{

[Severity: Low]
Is the qcom_iommu parameter needed here? It does not appear to be used
anywhere inside this helper function.

> +	/* Disable context bank before programming */
> +	iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);

[ ... ]

> @@ -301,7 +313,9 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>  		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>  			reg |= ARM_SMMU_SCTLR_E;
>  
> -		iommu_writel(ctx, ARM_SMMU_CB_SCTLR, reg);
> +		ctx->sctlr = reg;
> +
> +		qcom_iommu_program_ctx(qcom_iommu, ctx);
>  
>  		ctx->domain = domain;
>  	}

[Severity: High]
This is a pre-existing issue, but does the error path in
qcom_iommu_init_domain() handle partially initialized context banks correctly?

If an early iteration of the context bank setup succeeds, ctx->domain is set:

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_init_domain() {
    ...
    qcom_iommu_program_ctx(qcom_iommu, ctx);
    ctx->domain = domain;
    ...
}

If a subsequent iteration fails, the function jumps to out_clear_iommu
but leaves ctx->domain pointing to the domain.

When the IOMMU core frees the domain, could a subsequent hardware fault
trigger a use-after-free when the IRQ handler dereferences it?

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

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