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

On 7/26/2026 11:38 AM, [email protected] wrote:
> 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 <[email protected]>
>
> 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/[email protected]/
>
>> 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?

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);
> [ ... ]
>
>> +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?

If iris_load_fw_to_memory() fails for any reason — including pas_ctx
allocation failure — the caller does not invoke iris_fw_iommu_unmap(),
so core->pas_ctx will never be NULL here.

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