Re: [PATCH v9 06/14] media: iris: Enable Secure PAS support with IOMMU managed by Linux

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing cleanup of PAS metadata DMA allocation introduces a memory leak on every firmware load/unload and load error.
- [High] The `core->fw_dev` pointer is added to support Linux-managed IOMMU mapping for the firmware context device, but it is never initialized anywhere in the driver.

Pre-existing issues:
- [High] Missing cancel_delayed_work_sync() for sys_error_handler in driver remove path leads to Use-After-Free.
--

--- Patch [6]: [PATCH v9 06/14] media: iris: Enable Secure PAS support with IOMMU managed by Linux ---
commit d784ba63809fa89007c5ca51f86bf7421bf5d466
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

    media: iris: Enable Secure PAS support with IOMMU managed by Linux
    
    On platforms where a hypervisor is present, all Secure Monitor Calls
    (SMC) are intercepted. For qcom_scm_pas_auth_and_reset(), the hypervisor
    registers a Shared Memory (SHM) bridge over the Peripheral Image Loader
    (PIL) memory region so that TrustZone (TZ) can access it, forwards the
    authentication SMC to TZ, and upon return maps the PIL region and
    triggers the co-processor bring-up sequence.

    Link: https://lore.kernel.org/lkml/20260105-kvmrprocv10-v10-0-022e96815380@oss.qualcomm.com/

> diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> index 1a476146d7580..56c477b36b619 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
[ ... ]
> @@ -15,11 +17,14 @@
>  #define IRIS_PAS_ID				9
>  
>  #define MAX_FIRMWARE_NAME_SIZE	128
> +#define IRIS_FW_START_ADDR	0
>  
>  static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
>  {
> +	struct device *fw_dev = core->fw_dev ? core->fw_dev : core->dev;

[Severity: High]
Is core->fw_dev initialized anywhere before this point? It appears to be
allocated as part of struct iris_core using devm_kzalloc(), but never
assigned a value in the driver, meaning it will always be NULL and this will
always fall back to core->dev.

>  	const struct firmware *firmware = NULL;
> -	struct device *dev = core->dev;
> +	struct qcom_pas_context	*ctx;
> +	struct iommu_domain *domain;
>  	struct resource res;
[ ... ]
> @@ -53,9 +65,23 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
>  		goto err_release_fw;
>  	}
>  
> -	ret = qcom_mdt_load(dev, firmware, fw_name,
> -			    IRIS_PAS_ID, mem_virt, mem_phys, res_size, NULL);
> +	core->pas_ctx->use_tzmem = !!core->fw_dev;

[Severity: High]
Since core->fw_dev is never initialized, will use_tzmem always be false,
causing the entire iommu_map() block below to become dead code?

> +	ret = qcom_mdt_pas_load(core->pas_ctx, firmware, fw_name, mem_virt, NULL);
> +	if (ret)
> +		goto err_mem_unmap;
> +
> +	if (core->pas_ctx->use_tzmem) {
> +		domain = iommu_get_domain_for_dev(fw_dev);
> +		if (!domain) {
> +			ret = -ENODEV;
> +			goto err_mem_unmap;

[Severity: High]
If this error path is taken, will it leak the PAS metadata DMA allocation?
It seems the caller is required to free the metadata allocated by
qcom_mdt_pas_load() using qcom_pas_metadata_release(), but this call
is omitted during cleanup and error handling.

> +		}
> +
> +		ret = iommu_map(domain, IRIS_FW_START_ADDR, mem_phys, res_size,
> +				IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV, GFP_KERNEL);
> +	}
>  
> +err_mem_unmap:
>  	memunmap(mem_virt);
>  err_release_fw:
>  	release_firmware(firmware);
> @@ -77,13 +115,13 @@ int iris_fw_load(struct iris_core *core)
>  	ret = iris_load_fw_to_memory(core, fwpath);
>  	if (ret) {
>  		dev_err(core->dev, "firmware download failed\n");
> -		return -ENOMEM;
> +		return ret;
>  	}
>  
> -	ret = qcom_scm_pas_auth_and_reset(IRIS_PAS_ID);
> +	ret = qcom_pas_prepare_and_auth_reset(core->pas_ctx);
>  	if (ret)  {
>  		dev_err(core->dev, "auth and reset failed: %d\n", ret);
> -		return ret;
> +		goto err_unmap;

[Severity: High]
Does this error path also need to call qcom_pas_metadata_release()? The
metadata does not appear to be released here or in the normal success path.

>  	}
>  
>  	for (i = 0; i < core->iris_platform_data->tz_cp_config_data_size; i++) {
> @@ -94,20 +132,31 @@ int iris_fw_load(struct iris_core *core)
>  						     cp_config->cp_nonpixel_size);
>  		if (ret) {
>  			dev_err(core->dev, "qcom_scm_mem_protect_video_var failed: %d\n", ret);
> -			qcom_scm_pas_shutdown(IRIS_PAS_ID);
> -			return ret;
> +			goto err_pas_shutdown;
>  		}
>  	}
>  
> +	return 0;
> +
> +err_pas_shutdown:
> +	qcom_pas_shutdown(IRIS_PAS_ID);
> +err_unmap:
> +	iris_fw_iommu_unmap(core);
> +
>  	return ret;
>  }
>  
>  int iris_fw_unload(struct iris_core *core)
>  {
> -	return qcom_scm_pas_shutdown(IRIS_PAS_ID);
> +	int ret;
> +
> +	ret = qcom_pas_shutdown(IRIS_PAS_ID);
> +	iris_fw_iommu_unmap(core);
> +
> +	return ret;
>  }

[Severity: High]
Does iris_fw_unload() also need to call qcom_pas_metadata_release()?
If the PAS metadata context is not released, won't it cause a memory leak
on every firmware load/unload cycle?


[Severity: High]
This is a pre-existing issue, but does iris_remove() need to cancel the
sys_error_handler delayed work before destroying resources?

If a hardware fatal error interrupt schedules the work, and the driver is
unbound before the delay expires, it looks like iris_remove() will destroy
the mutex and free the core struct without calling cancel_delayed_work_sync().
Could this lead to a use-after-free when the delayed work eventually executes?

drivers/media/platform/qcom/iris/iris_probe.c:iris_remove() {
...
	iris_core_deinit(core);
	...
	iris_deinit_cb_devs(core);

	mutex_destroy(&core->lock);
}

drivers/media/platform/qcom/iris/iris_probe.c:iris_sys_error_handler() {
	struct iris_core *core =
			container_of(work, struct iris_core, sys_error_handler.work);

	iris_core_deinit(core);
	iris_core_init(core);
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-glymur-v9-0-8cf2cbe12a07@oss.qualcomm.com?part=6