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

[Vishnu Reddy <busanna.reddy@oss.qualcomm.com>]

On 7/15/2026 8:07 PM, sashiko-bot@kernel.org wrote:
> 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?

core->fw_dev will initialized in iris_vpu3x_init_cb_devs function before
reaching here. It's there in patch-11.

>> +	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.

It was discussed here, not to add metadata release call in video driver.
https://lore.kernel.org/all/20260415074140.46bg27tzbuyzjq3d@hu-mojha-hyd.qualcomm.com/

>> +		}
>> +
>> +		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.

Above reply applicable here also.

>>  	}
>>  
>>  	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?

Yes, this is addressed with
https://lore.kernel.org/all/20260604054708.42426-1-dennylin0707@gmail.com/

> 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);
> }
>