Re: [PATCH v9 04/14] remoteproc: qcom_q6v5_pas: Switch over to generic PAS TZ APIs

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional call to SCM-specific qcom_scm_assign_mem() causes NULL pointer dereference with non-SCM backends.
- [High] Missing module reference counting in the generic PAS API introduces a Use-After-Free and race condition.

Pre-existing issues:
- [High] Asynchronous pm_runtime_put() is canceled during device removal, leaving hardware powered on.
- [High] The DTB firmware buffer is leaked on the successful boot path.
- [High] The DTB PAS backend authentication session is not shut down if starting the main image fails.
--

commit 5fe417b2557886cf59a2b2dcdc429ef1c4165522
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>

remoteproc: qcom_q6v5_pas: Switch over to generic PAS TZ APIs
    
Switch qcom_q6v5_pas client driver over to generic PAS TZ APIs. Generic PAS
TZ service allows to support multiple TZ implementation backends like QTEE
based SCM PAS service, OP-TEE based PAS service and any further future TZ
backend service.

Since qcom_q6v5_pas depends on MDT loader for PAS firmware loading, it
has to be switched over to generic PAS APIs in this commit to avoid any
build issues.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 808e9609988d3..9eb1c4f6c2ab4 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c

[ ... ]

> @@ -247,7 +248,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
>  	return 0;
>  

[Severity: High]
This is a pre-existing issue, but does this leak the dtb_firmware buffer
on the successful boot path?

If the loading process succeeds, the function returns 0 without releasing
the buffer allocated earlier by request_firmware(). Since the remoteproc
core only automatically frees the main firmware buffer, wouldn't the
auxiliary dtb_firmware buffer be left allocated every time the remote
processor boots?

>  release_dtb_metadata:
> -	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> +	qcom_pas_metadata_release(pas->dtb_pas_ctx);
>  	release_firmware(pas->dtb_firmware);
>  
>  	return ret;

[ ... ]

> @@ -352,9 +353,9 @@ static int qcom_pas_start(struct rproc *rproc)
>  unmap_carveout:
>  	qcom_pas_unmap_carveout(rproc, pas->mem_phys, pas->mem_size);
>  release_pas_metadata:
> -	qcom_scm_pas_metadata_release(pas->pas_ctx);
> +	qcom_pas_metadata_release(pas->pas_ctx);
>  	if (pas->dtb_pas_id)
> -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> +		qcom_pas_metadata_release(pas->dtb_pas_ctx);
>  
>  unmap_dtb_carveout:
>  	if (pas->dtb_pas_id)

[Severity: High]
This is a pre-existing issue, but does the error rollback path miss shutting
down the DTB PAS backend?

If a subsequent step in qcom_pas_start() fails after the DTB image is
mapped and authenticated, the error path jumps to release_pas_metadata and
unmap_dtb_carveout.

However, it doesn't appear to invoke qcom_pas_shutdown() for the dtb_pas_id.
Could this leave the DTB backend authentication session active and prevent
the subsystem from loading again until the device reboots?

[ ... ]

> @@ -403,7 +404,7 @@ static int qcom_pas_stop(struct rproc *rproc)
>  	if (ret == -ETIMEDOUT)
>  		dev_err(pas->dev, "timed out on wait\n");
>  
> -	ret = qcom_scm_pas_shutdown(pas->pas_id);
> +	ret = qcom_pas_shutdown(pas->pas_id);

[Severity: High]
Could this lead to memory corruption if the backend module is unloaded?

With the switch to the generic PAS API, struct qcom_pas_ops lacks an owner
field, and the wrapper functions don't take a reference to the backend
module using try_module_get().

If the backend is unloaded concurrently, the global ops_ptr would be
cleared, causing qcom_pas_shutdown() to return -ENODEV instead of shutting
down the hardware.

Wouldn't the remote processor continue executing while its carveout memory
is unmapped and freed by Linux?

>  	if (ret && pas->decrypt_shutdown)
>  		ret = qcom_pas_shutdown_poll_decrypt(pas);
>  
>  	if (ret)

[ ... ]

> @@ -743,7 +744,7 @@ static int qcom_pas_probe(struct platform_device *pdev)
>  	if (!desc)
>  		return -EINVAL;
>  
> -	if (!qcom_scm_is_available())
> +	if (!qcom_pas_is_available())
>  		return -EPROBE_DEFER;
>  

[Severity: High]
Can this cause a NULL pointer dereference with non-SCM backends?

By broadening the check to qcom_pas_is_available(), probe can proceed when
backed by non-SCM services like OP-TEE. However, further down in the probe
sequence, qcom_pas_assign_memory_region() unconditionally calls the
SCM-specific qcom_scm_assign_mem() (if region_assign_idx is present).

If qcom_scm did not probe, the internal pointer inside
qcom_scm_assign_mem() would be NULL, leading to a kernel panic when
dereferenced.

>  	fw_name = desc->firmware_name;
>  	ret = of_property_read_string(pdev->dev.of_node, "firmware-name",

[ ... ]

> @@ -835,16 +836,16 @@ static int qcom_pas_probe(struct platform_device *pdev)
>  
>  	qcom_add_ssr_subdev(rproc, &pas->ssr_subdev, desc->ssr_name);
>  
> -	pas->pas_ctx = devm_qcom_scm_pas_context_alloc(pas->dev, pas->pas_id,
> -						       pas->mem_phys, pas->mem_size);
> +	pas->pas_ctx = devm_qcom_pas_context_alloc(pas->dev, pas->pas_id,
> +						   pas->mem_phys, pas->mem_size);
>  	if (IS_ERR(pas->pas_ctx)) {
>  		ret = PTR_ERR(pas->pas_ctx);
>  		goto remove_ssr_sysmon;
>  	}
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the device teardown path
cancel pending asynchronous suspends?

If probe fails and jumps to the teardown labels, qcom_pas_pds_disable()
is called, which drops the usage counter for power domains using the
asynchronous pm_runtime_put(). This is followed shortly by
pm_runtime_disable() in qcom_pas_pds_detach().

Since pm_runtime_disable() cancels pending asynchronous suspend work,
wouldn't this leave the hardware in an incorrect active power state?
Should the synchronous pm_runtime_put_sync() be used instead?

> -	pas->dtb_pas_ctx = devm_qcom_scm_pas_context_alloc(pas->dev, pas->dtb_pas_id,
> -							   pas->dtb_mem_phys,
> -							   pas->dtb_mem_size);
> +	pas->dtb_pas_ctx = devm_qcom_pas_context_alloc(pas->dev, pas->dtb_pas_id,
> +						       pas->dtb_mem_phys,
> +						       pas->dtb_mem_size);
>  	if (IS_ERR(pas->dtb_pas_ctx)) {
>  		ret = PTR_ERR(pas->dtb_pas_ctx);
>  		goto remove_ssr_sysmon;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702115835.167602-1-sumit.garg@kernel.org?part=4