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

[Sumit Garg <sumit.garg@kernel.org>]

On Tue, May 19, 2026 at 07:33:55AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Critical] Unconditionally calling `qcom_scm_assign_mem()` without checking SCM availability causes a NULL pointer dereference on non-SCM platforms.
> - [High] MDT firmware loading is not abstracted and continues to hardcode SCM APIs, causing both a build warning and functional failure on non-SCM backends.
> - [High] The DTB subsystem (dtb_pas_id) is authenticated but never shut down in the error paths of qcom_pas_start().
> - [High] The firmware object pas->dtb_firmware is leaked on the success path of qcom_pas_load().
> --
> 
> commit e21bc74de5d7e3215e46e9baa1c3a6b26f353409
> Author: Sumit Garg <sumit.garg@oss.qualcomm.com>
> 
> remoteproc: qcom_q6v5_pas: Switch over to generic PAS TZ APIs
> 
> This patch switches the qcom_q6v5_pas client driver over to generic PAS TZ
> APIs. This allows supporting multiple TrustZone implementation backends
> such as QTEE and OP-TEE.
> 
> > 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
> [ ... ]
> > @@ -118,8 +119,8 @@ struct qcom_pas {
> >  	struct qcom_rproc_ssr ssr_subdev;
> >  	struct qcom_sysmon *sysmon;
> >  
> > -	struct qcom_scm_pas_context *pas_ctx;
> > -	struct qcom_scm_pas_context *dtb_pas_ctx;
> > +	struct qcom_pas_context *pas_ctx;
> > +	struct qcom_pas_context *dtb_pas_ctx;
> >  };
> 
> Does changing this struct cause a compiler warning and functional regression
> for non-SCM backends?
> 
> Since this patch changes the context type to struct qcom_pas_context * but
> still passes it to qcom_mdt_pas_load(), it appears to cause an incompatible
> pointer type warning.
> 
> Furthermore, qcom_mdt_pas_load() internally hardcodes calls to SCM-specific
> APIs like qcom_scm_pas_init_image(), bypassing the generic PAS backend
> entirely. This seems to prevent the driver from working on non-SCM backends
> like OP-TEE.
> 
> I notice this is fixed later in the series in the commit "soc: qcom:
> mdtloader: Switch to generic PAS TZ APIs", but I am pointing it out here as
> it breaks the build and functionality at this step.

Ack, will rather merge the mdtloader patch into this one instead to
enable git bisection.

> 
> [ ... ]
> > @@ -250,7 +251,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
> >  	return 0;
> >  
> >  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;
> 
> Is there a pre-existing resource leak here?
> 
> When request_firmware() allocates the DTB firmware and qcom_mdt_pas_load()
> succeeds, the function returns 0 and skips the release_dtb_metadata label
> where release_firmware() is called.
> 
> The pas->dtb_firmware pointer appears to be retained but never freed in any
> teardown function like qcom_pas_unprepare() or qcom_pas_stop(). Could this
> permanently leak the firmware object and its backing buffer on every
> successful start?

Out of scope for this patch-set.

> 
> [ ... ]
> > @@ -339,13 +340,13 @@ static int qcom_pas_start(struct rproc *rproc)
> >  	ret = qcom_q6v5_wait_for_start(&pas->q6v5, msecs_to_jiffies(5000));
> >  	if (ret == -ETIMEDOUT) {
> >  		dev_err(pas->dev, "start timed out\n");
> > -		qcom_scm_pas_shutdown(pas->pas_id);
> > +		qcom_pas_shutdown(pas->pas_id);
> >  		goto unmap_carveout;
> >  	}
> 
> Does this error path miss shutting down the DTB subsystem?
> 
> While this was not introduced by this patch, if the DTB authentication
> step earlier in this function succeeds, but a subsequent step like
> qcom_q6v5_wait_for_start() fails, the error path shuts down the main PAS but
> misses calling qcom_pas_shutdown(pas->dtb_pas_id).
> 
> The code then jumps to unmap_dtb_carveout and unmaps the Linux-side memory,
> leaving the DTB subsystem active in TrustZone while its backing memory is
> unmapped.
> 

Out of scope for this patch-set.

> [ ... ]
> > @@ -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())
> >  		return -EPROBE_DEFER;
> >  
> >  	fw_name = desc->firmware_name;
> 
> Could this change lead to a NULL pointer dereference on non-SCM platforms?
> 
> By replacing the qcom_scm_is_available() check with qcom_pas_is_available(),
> the probe function can now continue on platforms using non-SCM backends like
> OP-TEE.
> 
> However, further down in probe, qcom_pas_assign_memory_region() is still
> unconditionally called if pas->region_assign_idx is set:
> 
> qcom_pas_assign_memory_region()
>    qcom_scm_assign_mem()
> 
> qcom_scm_assign_mem() accesses __scm->mempool without checking if __scm is
> NULL. On non-SCM platforms, __scm will be NULL, which seems like it would
> cause a kernel panic during probe for devices requiring memory region
> assignment.

qcom_scm_assign_mem() is still supported with an OP-TEE PAS backend too.

-Sumit