Re: [PATCH v7 05/12] media: iris: Enable Secure PAS support with IOMMU managed by Linux

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional devm_qcom_scm_pas_context_alloc of pas_ctx causes a memory leak during repeated iris_core_init failures.
- [High] Double invocation of qcom_scm_pas_shutdown in the qcom_scm_mem_protect_video_var error path.
- [High] PAS metadata memory is permanently leaked upon every firmware load.
- [Medium] iris_fw_unload() skips iris_fw_iommu_unmap() if qcom_scm_pas_shutdown() fails, leaking the IOMMU mapping.

Pre-existing issues:
- [High] The sys_error_handler delayed work is never canceled during driver removal, risking use-after-free and locking crashes.
--

commit 85711dbb43a8f02c3f39064784fe37381f80c2c5
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

media: iris: Enable Secure PAS support with IOMMU managed by Linux

On platforms without a hypervisor, Linux drives the firmware loading and
authentication steps directly. This commit extends the Iris driver to support
Secure Peripheral Authentication Service (PAS) on such platforms by creating
the firmware context device and mapping the IOMMU when the firmware device
SID is present.

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..8bdc9273036ce 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c

[ ... ]

> @@ -30,14 +34,18 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
>  	if (strlen(fw_name) >= MAX_FIRMWARE_NAME_SIZE - 4)
>  		return -EINVAL;
>  
> -	ret = of_reserved_mem_region_to_resource(dev->of_node, 0, &res);
> +	ret = of_reserved_mem_region_to_resource(core->dev->of_node, 0, &res);
>  	if (ret)
>  		return ret;
>  
>  	mem_phys = res.start;
>  	res_size = resource_size(&res);
>  
> -	ret = request_firmware(&firmware, fw_name, dev);
> +	pas_ctx = devm_qcom_scm_pas_context_alloc(fw_dev, IRIS_PAS_ID, mem_phys, res_size);

[Severity: High]
Does this unconditional devm_qcom_scm_pas_context_alloc() cause a memory leak
during repeated iris_core_init() failures?

If a subsequent step in the firmware load fails (e.g., request_firmware()
returning an error), iris_core_init() returns an error, but the device remains
bound.

Because iris_core_init() is called upon opening the V4L2 device, userspace
could repeatedly open the device, failing each time and allocating a new
pas_ctx without freeing the previous one.

> +	if (IS_ERR(pas_ctx))
> +		return PTR_ERR(pas_ctx);
> +
> +	ret = request_firmware(&firmware, fw_name, fw_dev);
>  	if (ret)
>  		return ret;

[ ... ]

> @@ -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_scm_pas_prepare_and_auth_reset(core->pas_ctx);

[Severity: High]
Does this permanently leak the PAS metadata memory upon every successful
firmware load?

qcom_mdt_pas_load() delegates to qcom_scm_pas_init_image(), which allocates
DMA or TZ memory for the firmware metadata (ELF header and hashes).

The SCM API requires the caller to free this memory using
qcom_scm_pas_metadata_release() once authentication is complete. 

It appears the iris driver never invokes this release function.

>  	if (ret)  {
>  		dev_err(core->dev, "auth and reset failed: %d\n", ret);
> -		return ret;
> +		goto err_unmap;
>  	}
>  
>  	for (i = 0; i < core->iris_platform_data->tz_cp_config_data_size; i++) {

[ ... ]

> @@ -95,16 +133,31 @@ int iris_fw_load(struct iris_core *core)
>  		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_scm_pas_shutdown(core->pas_ctx->pas_id);

[Severity: High]
Does this result in a double invocation of qcom_scm_pas_shutdown() on the
qcom_scm_mem_protect_video_var() error path?

The code calls qcom_scm_pas_shutdown(IRIS_PAS_ID) explicitly before jumping to
the err_pas_shutdown label, which calls qcom_scm_pas_shutdown() again.

Since core->pas_ctx->pas_id is initialized to IRIS_PAS_ID, this results in the
shutdown function being called twice consecutively for the same PAS ID, which
could trigger TrustZone errors or corrupt Secure Monitor state.

> +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_scm_pas_shutdown(core->pas_ctx->pas_id);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Will this early return skip the required IOMMU unmap logic if
qcom_scm_pas_shutdown() fails?

Since the caller (iris_core_deinit()) ignores the return value of
iris_fw_unload(), the driver proceeds with teardown while permanently leaking
the firmware's IOMMU IOVA mapping.

Software resource cleanup should likely continue even if a hardware/SCM
command fails.

> +
> +	iris_fw_iommu_unmap(core);
> +
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed that the sys_error_handler delayed
work is never canceled during driver removal.

Looking at iris_remove() in drivers/media/platform/qcom/iris/iris_probe.c:

static void iris_remove(struct platform_device *pdev)
{
    ...

The driver initializes core->sys_error_handler on a delayed workqueue in
iris_probe(), but iris_remove() unregisters devices and destroys core->lock
without calling cancel_delayed_work_sync(&core->sys_error_handler).

If the delayed work executes concurrently with or after iris_remove(), will it
access freed driver state and attempt to acquire the destroyed mutex, leading
to a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-glymur-v7-0-afaa55d11fe0@oss.qualcomm.com?part=5