Re: [PATCH v5 07/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 5 potential issue(s) to consider:
- [High] Missing `qcom_scm_pas_metadata_release()` causes PAS metadata buffer memory leak.
- [High] Unbounded memory leak due to dynamic `devm_qcom_scm_pas_context_alloc()` inside `iris_load_fw_to_memory()`.
- [High] Skipped `iris_fw_iommu_unmap()` if `qcom_scm_pas_shutdown()` fails causes permanent device failure.
- [High] Directly calling `iommu_map()` on a default DMA domain bypasses the DMA API.
- [Medium] Original error codes from `iris_load_fw_to_memory()` are unconditionally masked with `-ENOMEM`.
--

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

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

This commit extends the Iris driver to handle IOMMU and SHM bridge
configurations for firmware load and resources mapping in the absence
of a proprietary hypervisor.

[ ... ]
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
[ ... ]
> @@ -29,14 +33,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, pas_id, mem_phys, res_size);
> +	if (IS_ERR(pas_ctx))
> +		return PTR_ERR(pas_ctx);

devm_qcom_scm_pas_context_alloc() ties the memory to the fw_dev unbind event.
If iris_load_fw_to_memory() is called dynamically (e.g., when userspace opens
the device) and fails later in the function, this devres allocation isn't
freed. Could repeated failures here lead to an unbounded memory leak?

> +
> +	ret = request_firmware(&firmware, fw_name, fw_dev);
>  	if (ret)
>  		return ret;
>  
[ ... ]
> @@ -52,9 +60,27 @@ 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,
> -			    pas_id, mem_virt, mem_phys, res_size, NULL);
> +	pas_ctx->use_tzmem = !!core->fw_dev;
> +	ret = qcom_mdt_pas_load(pas_ctx, firmware, fw_name, mem_virt, NULL);
> +	if (ret)
> +		goto err_mem_unmap;
> +
> +	if (pas_ctx->use_tzmem) {
> +		domain = iommu_get_domain_for_dev(fw_dev);
> +		if (!domain) {
> +			ret = -ENODEV;
> +			goto err_mem_unmap;
> +		}
> +
> +		ret = iommu_map(domain, IRIS_FW_START_ADDR, mem_phys, res_size,
> +				IOMMU_READ | IOMMU_WRITE | IOMMU_PRIV, GFP_KERNEL);

iommu_get_domain_for_dev() will return the default DMA domain attached to
fw_dev. Is it safe to call iommu_map() directly on a DMA domain? 

This bypasses the DMA API's IOVA allocator and might trigger a kernel warning.
Should the driver allocate and attach an unmanaged domain instead?

> +		if (ret)
> +			goto err_mem_unmap;
> +	}
>  
> +	core->pas_ctx = pas_ctx;
> +
> +err_mem_unmap:
>  	memunmap(mem_virt);
>  err_release_fw:
>  	release_firmware(firmware);
[ ... ]
> @@ -79,10 +117,10 @@ int iris_fw_load(struct iris_core *core)
>  		return -ENOMEM;
>  	}

Now that iris_load_fw_to_memory() has been updated to return specific error
codes like -EINVAL, -ENODEV, or PTR_ERR(), is there a reason to continue
masking all of them with -ENOMEM here?

>  
> -	ret = qcom_scm_pas_auth_and_reset(core->iris_platform_data->pas_id);
> +	ret = qcom_scm_pas_prepare_and_auth_reset(core->pas_ctx);

qcom_mdt_pas_load() allocates a secure DMA buffer for firmware metadata via
qcom_scm_pas_init_image(). Does this driver need to call
qcom_scm_pas_metadata_release() after authentication here to avoid leaking the
metadata buffer?

>  	if (ret)  {
>  		dev_err(core->dev, "auth and reset failed: %d\n", ret);
> -		return ret;
> +		goto err_unmap;
>  	}
>  
[ ... ]
> @@ -93,17 +131,31 @@ int iris_fw_load(struct iris_core *core)
[ ... ]
>  int iris_fw_unload(struct iris_core *core)
>  {
> -	return qcom_scm_pas_shutdown(core->iris_platform_data->pas_id);
> +	int ret;
> +
> +	ret = qcom_scm_pas_shutdown(core->pas_ctx->pas_id);
> +	if (ret)
> +		return ret;
> +
> +	iris_fw_iommu_unmap(core);
> +
> +	return ret;

If qcom_scm_pas_shutdown() fails and returns early, iris_fw_iommu_unmap() is
skipped. Will this leave the IOMMU mapping active at IRIS_FW_START_ADDR,
causing the next iommu_map() attempt during initialization to fail with
-EEXIST?

>  }
>  
>  int iris_set_hw_state(struct iris_core *core, bool resume)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509-glymur-v5-0-7fbb340c5dbd@oss.qualcomm.com?part=7