Re: [PATCH v7 06/15] remoteproc: qcom_q6v5_mss: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incorrect probe deferral check on PAS instead of SCM leads to potential NULL pointer dereferences and infinite probe deferral loops.
- [High] Lockless and unprotected access to the generic PAS backend API can lead to Use-After-Free or TOCTOU races.

Pre-existing issues:
- [High] Missing probe deferral for platforms that require PAS memory setup but do not require memory protection.
- [High] The driver fails to initialize `qproc->need_pas_mem_setup` from the device match data, skipping PAS memory setup entirely on platforms that need it.
--

commit 1d2ed84159dbf869652e837f7c8e10ba7f8e650b
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>

remoteproc: qcom_q6v5_mss: Switch to generic PAS TZ APIs

Switch qcom_q6v5_mss 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.

> 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 does the driver fail to initialize
qproc->need_pas_mem_setup from the device match data?

In q6v5_probe(), various fields from the desc match data are copied to
the qproc structure, but desc->need_pas_mem_setup is completely omitted.
Since qproc is allocated via devm_rproc_alloc() using kzalloc,
qproc->need_pas_mem_setup will always be evaluated as false here, skipping
the required setup for all platforms that rely on 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]
Can this lead to a Use-After-Free or TOCTOU race if the PAS backend
module is unregistered concurrently?

Looking at qcom_pas_mem_setup() in drivers/firmware/qcom/qcom_pas.c, it
uses a lockless ops_ptr variable that is checked and dereferenced
sequentially:

int qcom_pas_mem_setup(...)
{
    if (!ops_ptr)
        return -ENODEV;

    return ops_ptr->mem_setup(...);
}

If the PAS backend module is unloaded concurrently, ops_ptr could be
nullified between the check and the dereference, without any device
links or module references preventing it.

>  		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]
Does changing this check to use qcom_pas_is_available() break the
guarantee that SCM is fully initialized?

The desc->need_mem_protection flag indicates the platform requires
qcom_scm_assign_mem() to configure memory protection, which directly
dereferences the global __scm->dev pointer.

If an alternative PAS backend like OP-TEE registers the PAS service,
q6v5_probe() will proceed even if SCM is absent. When
q6v5_xfer_mem_ownership() later calls qcom_scm_assign_mem(), it would
dereference an uninitialized __scm pointer, leading to a panic.

Conversely, could platforms that require SCM memory protection but lack
a PAS backend be trapped in an infinite -EPROBE_DEFER loop?

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing probe
deferral for platforms that require PAS memory setup but do not require
memory protection?

For platforms where need_pas_mem_setup is true and need_mem_protection
is false (e.g., MSM8937, MSM8940, MSM8953), the driver skips the
deferral check entirely. If the remoteproc is started before the PAS
backend initializes, qcom_pas_mem_setup() will fail with -ENODEV and
prevent the remote processor from starting.

>  		return -EPROBE_DEFER;
>  
>  	mba_image = desc->hexagon_mba_image;

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