Re: [PATCH v9 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 2 potential issue(s) to consider:
- [High] Replacing the SCM availability check with a generic PAS check during probe breaks the strict SCM dependency for memory protection, leading to a NULL pointer dereference in `qcom_scm_assign_mem()`.
- [High] Missing module reference counting in the generic PAS API exposes client drivers to a TOCTOU race and use-after-free if the backend module is unloaded.
--

commit f9f49708d56de20022405acdb58867dd0cffe7bb
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) {
> -		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]
Is there a time-of-check to time-of-use race in the generic PAS API that could
lead to a use-after-free or NULL pointer dereference?

The client driver switches to using the newly introduced generic PAS API
without any mechanism to prevent backend unloads.

Looking at qcom_pas_mem_setup() in drivers/firmware/qcom/qcom_pas.c, the API
locklessly dereferences ops_ptr after checking it, without holding a module
reference or synchronization:

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);
}

If a privileged user unloads the backend module (like qcom_pas_tee)
concurrently with the client driver starting the remoteproc, ops_ptr could be
set to NULL between the check and the dereference.

Does the generic PAS API need module reference counting (like try_module_get())
or device links to prevent the backend module from being unloaded while in use?

>  		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 this change break the memory protection dependency on the SCM driver?

If desc->need_mem_protection is true, but the SCM driver is not available
(for example, if the OP-TEE PAS backend probes first, or if SCM fails to
probe), q6v5_probe() will now observe qcom_pas_is_available() as true and
incorrectly succeed without deferring probe for SCM.

When the remoteproc is later started, q6v5_xfer_mem_ownership()
unconditionally calls qcom_scm_assign_mem(), which directly dereferences
the uninitialized __scm pointer:

drivers/firmware/qcom/qcom_scm.c:qcom_scm_assign_mem() {
...
    void *ptr __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
                                                    ptr_sz, GFP_KERNEL);
...
}

Could this lead to a NULL pointer dereference in qcom_scm_assign_mem() on
configurations that use OP-TEE PAS or have SCM probe delayed?

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

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