Re: [PATCH v8 04/14] 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] Removing the `qcom_scm_is_available()` check in `qcom_pas_probe()` exposes the driver to a NULL pointer dereference in `qcom_scm_assign_mem()`.

Pre-existing issues:
- [High] `pas->dtb_firmware` is leaked on the success path because `release_firmware()` is never called.
- [High] If `qcom_pas_start()` fails after authenticating the DTB PAS, it never invokes `qcom_pas_shutdown()` for the DTB PAS, leaving it permanently running/authenticated in TrustZone.
--

commit 410365720c51e458aee76cb1ae2177033dc208d2
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>

remoteproc: qcom_q6v5_pas: Switch over to generic PAS TZ APIs

This commit switches the qcom_q6v5_pas client driver over to use generic PAS
TZ APIs. This allows support for multiple TZ implementation backends, and
updates the MDT loader PAS dependencies to prevent build issues.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 808e9609988d3..9eb1c4f6c2ab4 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
[ ... ]
> @@ -225,9 +226,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);
>  	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);
[ ... ]
>  		ret = qcom_mdt_pas_load(pas->dtb_pas_ctx, pas->dtb_firmware,
>  					pas->dtb_firmware_name, pas->dtb_mem_region,
>  					&pas->dtb_mem_reloc);
>  		if (ret)
>  			goto release_dtb_metadata;
>  	}
>  
>  	return 0;

[Severity: High]
This is a pre-existing issue, but does this code leak the DTB firmware buffer
on the success path?

If request_firmware() successfully allocates pas->dtb_firmware, it doesn't
appear to be freed with release_firmware() here before returning 0, nor
is it released anywhere in the teardown callbacks like qcom_pas_stop() or
qcom_pas_unprepare().

Could this lead to memory exhaustion if the remoteproc is repeatedly started
and stopped via sysfs?

>  
>  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;
[ ... ]
> @@ -307,7 +308,7 @@ static int qcom_pas_start(struct rproc *rproc)
>  		if (ret)
>  			goto disable_px_supply;
>  
> -		ret = qcom_scm_pas_prepare_and_auth_reset(pas->dtb_pas_ctx);
> +		ret = qcom_pas_prepare_and_auth_reset(pas->dtb_pas_ctx);
>  		if (ret) {
>  			dev_err(pas->dev,
>  				"failed to authenticate dtb image and release reset\n");
> @@ -326,7 +327,7 @@ static int qcom_pas_start(struct rproc *rproc)
>  	if (ret)
>  		goto release_pas_metadata;
[ ... ]
>  unmap_carveout:
>  	qcom_pas_unmap_carveout(rproc, pas->mem_phys, pas->mem_size);
>  release_pas_metadata:
> -	qcom_scm_pas_metadata_release(pas->pas_ctx);
> +	qcom_pas_metadata_release(pas->pas_ctx);
>  	if (pas->dtb_pas_id)
> -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> +		qcom_pas_metadata_release(pas->dtb_pas_ctx);

[Severity: High]
This isn't a bug introduced by this patch, but if a failure occurs after the
DTB PAS is successfully authenticated, could this strand the TrustZone state?

If qcom_pas_prepare_and_auth_reset(pas->dtb_pas_ctx) succeeds, but a
subsequent step like qcom_mdt_pas_load() fails, the code jumps to the
release_pas_metadata label.

The error unwind doesn't seem to invoke qcom_pas_shutdown(pas->dtb_pas_id)
for the DTB PAS, which might leave the DTB PAS session permanently active in
TrustZone until the next reboot.

>  
>  unmap_dtb_carveout:
>  	if (pas->dtb_pas_id)
[ ... ]
> @@ -743,7 +744,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;

[Severity: High]
By changing this dependency check from qcom_scm_is_available() to
qcom_pas_is_available(), is it possible to encounter a NULL pointer
dereference later in the probe sequence?

If the OP-TEE PAS backend is registered, qcom_pas_is_available() returns true,
allowing probe to proceed even if the qcom_scm driver is deferred or
unavailable.

Later in the sequence, the code calls:

qcom_pas_probe()
  qcom_pas_assign_memory_region()
    qcom_scm_assign_mem()

If qcom_scm_assign_mem() is called unconditionally when region_assign_count > 0,
it will attempt to use the __scm pointer to allocate TrustZone memory, which
might be NULL if the SCM driver isn't available.

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