Re: [PATCH 05/12] iommu: qcom_iommu: handle the SMMU global register space

[sashiko-bot@kernel.org]

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

New issues:
- [High] Context bank state is not reprogrammed upon runtime resume, leading to translation failures for instances in power domains.
- [High] Skipping the FSR clear at probe time allows a pending bootloader fault to trigger an interrupt before `ctx->domain` is set, causing a NULL pointer dereference.

Pre-existing issues:
- [High] The shared IRQ handler accesses hardware registers without checking if the device is active, and there is no IRQ synchronization in the suspend path.
- [High] Unbinding the parent IOMMU device causes a NULL pointer dereference in the child context device's remove callback.
--

--- Patch [5]: [PATCH 05/12] iommu: qcom_iommu: handle the SMMU global register space ---
commit bf26052ef6d0329129f71ba618d88c9f30a2725a
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

iommu: qcom_iommu: handle the SMMU global register space

On msm8974 the OS has to access the SMMU global register space: the
non-secured instances need it fully programmed and even the TZ-managed
ones expose OS-writable implementation-defined registers in it. The
driver only knows about the msm8916-style "SMMU local" region in
resource 0, and has no accessors for the global space.

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

[ ... ]

> @@ -213,6 +256,35 @@ static irqreturn_t qcom_iommu_fault(int irq, void *dev)
>  	return IRQ_HANDLED;
>  }
>  

[Severity: High]
This is a pre-existing issue, but shouldn't the shared IRQ handler check if
the device is active before accessing hardware registers?

The interrupt line is registered with IRQF_SHARED. If another device sharing
the line asserts an interrupt while the IOMMU's power domain is suspended,
qcom_iommu_fault() executes and accesses ARM_SMMU_CB_FSR without waking the
device via pm_runtime_get_if_active(). Accessing unpowered MMIO registers
can trigger an asynchronous external abort or bus stall.

Also, qcom_iommu_suspend() disables clocks without calling
synchronize_irq(), allowing race conditions if the shared handler is running
mid-flight.

> +static void qcom_iommu_program_ctx(struct qcom_iommu_dev *qcom_iommu,
> +				   struct qcom_iommu_ctx *ctx)
> +{
> +	/* Disable context bank before programming */
> +	iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);

[Severity: High]
Is it necessary to reprogram this cached state during runtime resume?

When the PM domain collapses and subsequently resumes, the non-secure context
banks remain unconfigured, breaking any further DMA translations.

Looking at qcom_iommu_resume(), it restores secure configurations but
doesn't restore the OS-managed (non-secure) context banks by looping over
them and calling qcom_iommu_program_ctx().

[ ... ]

> @@ -749,9 +773,10 @@ static int qcom_iommu_ctx_probe(struct platform_device *pdev)
>  		ctx->secured_ctx = true;
>  
>  	/* clear IRQs before registering fault handler, just in case the
> -	 * boot-loader left us a surprise:
> +	 * boot-loader left us a surprise.  Instances with a power domain
> +	 * may not be accessible yet; they are reset at first resume.
>  	 */
> -	if (!ctx->secured_ctx) {
> +	if (!ctx->secured_ctx && !qcom_iommu->cfg) {

[Severity: High]
If a bootloader fault is pending and we bypass clearing the Fault Status
Register (FSR) here, could it trigger an interrupt before ctx->domain is set?

When devm_request_irq() enables the shared IRQ line a few lines later, the
pending interrupt could fire immediately. The handler qcom_iommu_fault()
executes and calls report_iommu_fault(ctx->domain, ...).

Because ctx->domain is only assigned later during device attachment
(qcom_iommu_init_domain), it is currently NULL, which report_iommu_fault()
will dereference.

>  		ret = pm_runtime_resume_and_get(dev->parent);
>  		if (ret)
>  			return ret;

[ ... ]

[Severity: High]
This is a pre-existing issue, but can unbinding the parent IOMMU device cause
a NULL pointer dereference in the child context device's remove callback?

When the parent driver is unbound, qcom_iommu_device_remove() sets its private
data to NULL via platform_set_drvdata(pdev, NULL). Subsequently, devres tears
down the child devices, invoking qcom_iommu_ctx_remove().

This child callback calls dev_get_drvdata(pdev->dev.parent) to retrieve the
parent's data, which is now NULL, and immediately dereferences it:

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_ctx_remove() {
    ...
    struct qcom_iommu_dev *qcom_iommu = dev_get_drvdata(pdev->dev.parent);
    ...
    qcom_iommu->ctxs[ctx->asid] = NULL;
    ...
}

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