Re: [PATCH v10 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 2 potential issue(s) to consider:
- [High] Incomplete Implementation / Dead Code: `core->fw_dev` is never assigned, rendering the direct IOMMU mapping feature entirely dead code.
- [Medium] Potential NULL pointer dereference of `core->pas_ctx` in `iris_fw_iommu_unmap` during asynchronous error cleanup.
--

--- Patch [6]: [PATCH v10 06/14] media: iris: Enable Secure PAS support with IOMMU managed by Linux ---
commit 4079c73feb157bb490aaa33ca1d6e4970df1c4f7
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 handles the SMC mapping and brings up
the co-processor directly. This patch adds support to the Iris driver to
manage the firmware context device and IOMMU mapping directly 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_core.h b/drivers/media/platform/qcom/iris/iris_core.h
> --- a/drivers/media/platform/qcom/iris/iris_core.h
> +++ b/drivers/media/platform/qcom/iris/iris_core.h
> @@ -85,6 +87,8 @@ struct iris_core {
>  	struct device				*dev;
>  	struct device				*np_dev;
>  	struct device				*p_dev;
> +	struct device				*fw_dev;
> +	struct qcom_pas_context			*pas_ctx;
>  	void __iomem				*reg_base;

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c

[ ... ]

> @@ -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]
Is core->fw_dev ever assigned a value in the driver?

It appears to be declared in struct iris_core, which is zero-initialized,
but I don't see any logic populating it anywhere in the codebase. Because it
remains NULL, !!core->fw_dev will always be false. Does this unintentionally
make the subsequent direct IOMMU mapping logic unreachable 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);

[ ... ]

> +static void iris_fw_iommu_unmap(struct iris_core *core)
> +{
> +	struct iommu_domain *domain;
> +
> +	if (!core->fw_dev)
> +		return;
> +
> +	domain = iommu_get_domain_for_dev(core->fw_dev);
> +	if (domain)
> +		iommu_unmap(domain, IRIS_FW_START_ADDR, core->pas_ctx->mem_size);

[Severity: Medium]
Could this lead to a NULL pointer dereference on core->pas_ctx if
core->fw_dev is properly initialized in the future?

If an early failure occurs during iris_load_fw_to_memory (for instance, if
of_reserved_mem_region_to_resource fails), core->pas_ctx is never allocated.
If a hardware watchdog or spurious interrupt fires concurrently,
iris_hfi_isr_handler could overwrite the state to IRIS_CORE_ERROR and
schedule iris_sys_error_handler. The error handler calls iris_core_deinit()
followed by iris_fw_unload(), which invokes this function.

Should there be a check to ensure core->pas_ctx is non-NULL before
accessing core->pas_ctx->mem_size?

> +}
> +
>  int iris_fw_load(struct iris_core *core)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-glymur-v10-0-de451559e88b@oss.qualcomm.com?part=6