Re: [PATCH v8 05/14] remoteproc: qcom_q6v5_mss: Switch 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()` removes the SCM dependency check, leading to a NULL pointer dereference during memory protection.
- [High] The generic PAS API removes the hard module dependency on the backend provider, exposing client drivers to a TOCTOU race condition and Use-After-Free.

Pre-existing issues:
- [High] Missing initialization of `need_pas_mem_setup` makes PAS memory setup silently skipped.
--

commit 286e03507d8f96a33a10d31ad2cb432432734d78
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>
Subject: remoteproc: qcom_q6v5_mss: Switch to generic PAS TZ APIs

This commit switches the qcom_q6v5_mss client driver to use the generic PAS
TZ APIs. This enables support for multiple TZ implementation backends, such
as QTEE based SCM PAS service and OP-TEE based PAS service.

> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index ae78f5c7c1b69..96888007faa88 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
[ ... ]
> @@ -1480,7 +1481,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	}
>  
>  	if (qproc->need_pas_mem_setup) {

[Severity: High]
This is a pre-existing issue, but I noticed that need_pas_mem_setup is never
populated from the match descriptor in q6v5_probe(). Since qproc is
allocated via devm_rproc_alloc() and zero-initialized, this boolean will
always be false.

Does this prevent qcom_pas_mem_setup() from ever being called even on
platforms that require it?

> -		ret = qcom_scm_pas_mem_setup(MPSS_PAS_ID, qproc->mpss_phys, qproc->mpss_size);
> +		ret = qcom_pas_mem_setup(MPSS_PAS_ID, qproc->mpss_phys, qproc->mpss_size);

[Severity: High]
Does this new generic PAS API safely handle backend module unloading?

Previously, calling qcom_scm_pas_mem_setup() created a hard symbol
dependency, preventing the backend module from being unloaded. The generic
framework uses a lockless ops_ptr without acquiring module references:

drivers/firmware/qcom/qcom_pas.c:qcom_pas_mem_setup() {
    if (!ops_ptr)
        return -ENODEV;

    return ops_ptr->mem_setup(ops_ptr->dev, pas_id, addr, size);
}

Could the backend module be unloaded after the ops_ptr check but before or
during the mem_setup() call, leading to a use-after-free or NULL pointer
dereference?

>  		if (ret) {
>  			dev_err(qproc->dev,
>  				"setting up mpss memory failed: %d\n", ret);
[ ... ]
> @@ -2077,7 +2078,7 @@ static int q6v5_probe(struct platform_device *pdev)
>  	if (!desc)
>  		return -EINVAL;
>  
> -	if (desc->need_mem_protection && !qcom_scm_is_available())
> +	if (desc->need_mem_protection && !qcom_pas_is_available())

[Severity: High]
Is it safe to replace the SCM availability check with the PAS availability
check here?

If need_mem_protection is true, q6v5_mpss_load() will unconditionally call
q6v5_xfer_mem_ownership(), which eventually relies on qcom_scm_assign_mem()
and dereferences the internal SCM structures.

If a non-SCM PAS backend is available but SCM is uninitialized, the PAS
check will succeed here, but the subsequent memory protection operations
will trigger a NULL pointer dereference.

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