Re: [PATCH 06/12] iommu: qcom_iommu: support non-TZ-managed instances

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing configuration data in `qcom_iommu_of_match` makes the non-secure code path unreachable.
- [High] Context bank registers are not restored during runtime resume for non-secure IOMMU instances.

Pre-existing issues:
- [High] Dangling pointer and potential UAF when `qcom_iommu_init_domain()` fails halfway through context bank initialization.
- [High] The shared interrupt handler `qcom_iommu_fault` can be invoked while the device is suspended or before a domain is attached, leading to unclocked register accesses and a NULL pointer dereference.
- [High] `qcom_iommu_unmap` calls `pm_runtime_get_sync`, which sleeps, but IOMMU unmap operations can be invoked from atomic context.
--

--- Patch [6]: [PATCH 06/12] iommu: qcom_iommu: support non-TZ-managed instances ---
commit e923920b01d0c5363e8c876cd9db3360e29f7e5a
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
iommu: qcom_iommu: support non-TZ-managed instances

This patch adds non-secure programming mode support for MSM8974 GPU IOMMU
instances that are not managed by TZ. It configures the global space at
runtime resume, clears global faults, and sets up stream mapping instead of
relying on secure calls.

> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 428b18697244c..3b07c4cf23fe1 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
[ ... ]
> @@ -327,7 +411,7 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>  	for (i = 0; i < fwspec->num_ids; i++) {
>  		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
>  
> -		if (!ctx->secure_init) {
> +		if (!qcom_iommu->non_secure && !ctx->secure_init) {

[Severity: High]
This is a pre-existing issue, but is there a dangling pointer and potential
use-after-free when qcom_iommu_init_domain() fails halfway through context
bank initialization?

If context bank initialization fails (for example, qcom_scm_restore_sec_cfg()
fails on a subsequent iteration), the function returns an error without
clearing ctx->domain for the already processed context banks. The IOMMU core
then frees the domain memory.

If a hardware fault interrupt fires, will qcom_iommu_fault() dereference the
freed ctx->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);
[ ... ]
> @@ -907,8 +991,12 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
>  
>  	if (of_property_read_u32(dev->of_node, "qcom,iommu-secure-id",
>  				 &qcom_iommu->sec_id)) {
> -		dev_err(dev, "missing qcom,iommu-secure-id property\n");
> -		return -ENODEV;
> +		if (!qcom_iommu->cfg) {
> +			dev_err(dev, "missing qcom,iommu-secure-id property\n");
> +			return -ENODEV;
> +		}

[Severity: High]
Does the missing configuration data in qcom_iommu_of_match make this
non-secure code path unreachable?

During device probe, of_device_get_match_data() returns NULL because the newly
introduced compatibles (like qcom,msm8974-gpu-iommu) and their struct
qcom_iommu_cfg data were not added to the qcom_iommu_of_match array in this
patch.

This causes qcom_iommu->cfg to evaluate to NULL, unconditionally failing
with -ENODEV here when probing a non-secure instance.

> +		/* The secure world does not manage this instance at all */
> +		qcom_iommu->non_secure = true;
>  	}
>  
>  	if (qcom_iommu_has_secure_context(qcom_iommu)) {
[ ... ]
> @@ -981,8 +1069,15 @@ static int __maybe_unused qcom_iommu_resume(struct device *dev)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (dev->pm_domain)
> -		return qcom_scm_restore_sec_cfg(qcom_iommu->sec_id, 0);
> +	if (qcom_iommu->non_secure) {
> +		ret = qcom_iommu_reset_ns(qcom_iommu);
> +		if (ret)
> +			return ret;

[Severity: High]
Are context bank registers restored during runtime resume for non-secure
IOMMU instances?

When the non-secure IOMMU is runtime resumed (e.g., after GDSC power
collapse), qcom_iommu_resume() restores the global registers via
qcom_iommu_reset_ns(), but it seems to completely omit iterating over and
reprogramming the attached context banks.

Since the context bank child devices lack PM operations, do they remain
disabled (SCTLR = 0) after resume, causing translation faults for all
non-secure instances?

> +	} else if (dev->pm_domain) {
> +		ret = qcom_scm_restore_sec_cfg(qcom_iommu->sec_id, 0);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but can the shared interrupt handler
qcom_iommu_fault() be invoked while the device is suspended or before a
domain is attached, leading to unclocked register accesses and a NULL
pointer dereference?

In qcom_iommu_ctx_probe(), a shared IRQ is requested before a domain is
attached:

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_ctx_probe() {
    ...
	ret = devm_request_irq(dev, irq,
			       qcom_iommu_fault,
			       IRQF_SHARED,
    ...
}

If another device sharing the IRQ fires, qcom_iommu_fault() executes and
accesses ARM_SMMU_CB_FSR. If the device is unclocked, this crashes the bus.

If clocked and a bootloader fault is pending, it dereferences a NULL
ctx->domain. The target patch seems to explicitly bypass clearing pending
bootloader faults for non-secure instances when qcom_iommu->cfg is present:

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_ctx_probe() {
    ...
	if (!ctx->secured_ctx && !qcom_iommu->cfg) {
		ret = pm_runtime_resume_and_get(dev->parent);
    ...
}

Will this ensure the crash triggers immediately upon shared IRQ firing?

[Severity: High]
This is a pre-existing issue, but does qcom_iommu_unmap() call
pm_runtime_get_sync() which sleeps, even though IOMMU unmap operations can
be invoked from atomic context?

The DMA API invokes the IOMMU unmap operation from an atomic context
(e.g., interrupt handlers):

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_unmap() {
    ...
	pm_runtime_get_sync(qcom_domain->iommu->dev);
	spin_lock_irqsave(&qcom_domain->pgtbl_lock, flags);
    ...
}

Because the device is not marked with pm_runtime_irq_safe(),
pm_runtime_get_sync() acquires a mutex and sleeps. Can this lead to a "BUG:
sleeping function called from invalid context" panic?

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