Re: [PATCH v7 05/15] remoteproc: qcom_q6v5_pas: Switch over to generic PAS TZ APIs

[sashiko-bot@kernel.org]

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

New issues:
- [High] Replacing `qcom_scm_is_available()` with `qcom_pas_is_available()` breaks probe ordering with the SCM driver, leading to a potential NULL pointer dereference in `qcom_scm_assign_mem()`.
- [High] Lockless access to the global `ops_ptr` combined with missing module reference counting allows the PAS backend module to be unloaded while in use, leading to a NULL pointer dereference or Use-After-Free.

Pre-existing issues:
- [High] The dtb firmware object requested via request_firmware() is never released on the success path of qcom_pas_load().
--

commit 3637c6003564ac65554a8805148bf9af6b138d86
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>
Date:   Fri May 22 11:59:26 2026 +0000

    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 da27d1d3c9da6..847249c28c1be 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
[ ... ]
> @@ -228,9 +229,9 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
>  	pas->firmware = fw;
>  
>  	if (pas->lite_pas_id)
> -		qcom_scm_pas_shutdown(pas->lite_pas_id);
> +		qcom_pas_shutdown(pas->lite_pas_id);

[Severity: High]
Can the new generic PAS TZ APIs lead to a use-after-free or null pointer
dereference?

The backend implementation for qcom_pas_shutdown() and other generic APIs
seems to perform a lockless check of a global ops_ptr without any module
reference counting (e.g., try_module_get()) or synchronization.

If a user unloads the PAS backend module concurrently while a remoteproc
operation is in flight, could the pointer become null right after the check,
or the backing code be unmapped from memory?

>  	if (pas->lite_dtb_pas_id)
> -		qcom_scm_pas_shutdown(pas->lite_dtb_pas_id);
> +		qcom_pas_shutdown(pas->lite_dtb_pas_id);
>  
>  	if (pas->dtb_pas_id) {
>  		ret = request_firmware(&pas->dtb_firmware, pas->dtb_firmware_name, pas->dev);
> @@ -250,7 +251,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 function leak pas->dtb_firmware?

When request_firmware() succeeds for the DTB firmware, this function returns
0 without calling release_firmware(). There doesn't appear to be any
corresponding cleanup for this firmware object elsewhere in the driver's
lifecycle (e.g., in qcom_pas_unprepare() or qcom_pas_stop()).

Could this lead to gradual memory exhaustion every time the remoteproc is
loaded or restarted?

>  
>  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;
[ ... ]
> @@ -746,7 +747,7 @@ static int qcom_pas_probe(struct platform_device *pdev)
>  	if (!desc)
>  		return -EINVAL;
>  
> -	if (!qcom_scm_is_available())
> +	if (!qcom_pas_is_available())

[Severity: High]
Does this change break probe ordering with the SCM driver?

If an alternative PAS backend (such as OP-TEE) probes before the SCM driver,
qcom_pas_probe() will now proceed instead of deferring.

However, for platforms that specify memory region assignments
(region_assign_idx), qcom_pas_assign_memory_region() is still called
during probe, which unconditionally invokes qcom_scm_assign_mem().

Since qcom_scm_assign_mem() dereferences SCM state without initialization
checks, could calling it before the SCM driver has fully probed lead to a null
pointer dereference?

>  		return -EPROBE_DEFER;
>  
>  	fw_name = desc->firmware_name;

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