Re: [PATCH 2/4] remoteproc: qcom: pas: Support secondary images

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] remoteproc: missing shutdown of auxiliary processors on start failure
- [Medium] remoteproc: TrustZone metadata leak on start error path
- [Low] remoteproc: unconditional PAS ID assignment breaks conditional logic
--

commit 3e8f99d1a610c4628e17e809412591bdc8f4873f
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

remoteproc: qcom: pas: Support secondary images

The PAS remoteproc driver currently supports one primary image and its
optional DTB. OOBMSS additionally requires the secondary image and DTB
to be loaded, authenticated, mapped, shut down, and released through
their own PAS identifiers and carveouts. This commit adds generic PAS
support for these secondary images.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index ca8e61254c449..604dfa947340e 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
[ ... ]
> @@ -326,7 +417,7 @@ static int qcom_pas_start(struct rproc *rproc)
>  	ret = qcom_mdt_pas_load(pas->pas_ctx, pas->firmware, rproc->firmware,
>  				&pas->mem_reloc);
>  	if (ret)
> -		goto release_pas_metadata;
> +		goto unmap_dtb_carveout;

[Severity: Medium]
Does this code leak TrustZone metadata if the primary firmware load fails?

In qcom_pas_start(), qcom_mdt_pas_load() allocates TrustZone metadata via
__qcom_mdt_pas_init(). If segment copying fails subsequently, this metadata
needs to be explicitly released.

By changing the jump from goto release_pas_metadata to goto unmap_dtb_carveout,
the release_pas_metadata cleanup label is bypassed, leaving the pas_ctx
metadata allocated.

If rproc_start() fails during a crash recovery (rproc_boot_recovery()), the
core does not automatically invoke the ops->unprepare() hook. Consequently,
can this cause the TrustZone memory to leak until the system is fully
rebooted?

>  
>  	qcom_pil_info_store(pas->info_name, pas->mem_phys, pas->mem_size);
[ ... ]
> @@ -361,12 +456,27 @@ static int qcom_pas_start(struct rproc *rproc)
>  	qcom_pas_unmap_carveout(rproc, pas->mem_phys, pas->mem_size);
>  release_pas_metadata:
>  	qcom_pas_metadata_release(pas->pas_ctx);
> -	if (pas->dtb_pas_id)
> -		qcom_pas_metadata_release(pas->dtb_pas_ctx);
> -
>  unmap_dtb_carveout:
>  	if (pas->dtb_pas_id)
> -		qcom_pas_unmap_carveout(rproc, pas->dtb_mem_phys, pas->dtb_mem_size);
> +		qcom_pas_unmap_carveout(rproc, pas->dtb_mem_phys,
> +					pas->dtb_mem_size);
> +release_dtb_metadata:
> +	if (pas->dtb_pas_id)
> +		qcom_pas_metadata_release(pas->dtb_pas_ctx);
> +unmap_secondary_carveout:
> +	if (pas->secondary_pas_id)
> +		qcom_pas_unmap_carveout(rproc, pas->secondary_mem_phys,
> +					pas->secondary_mem_size);

[Severity: High]
Does this error path rip away backing memory without halting the successfully
started auxiliary processors?

In qcom_pas_start(), multiple PAS components are authenticated and booted
sequentially via qcom_pas_prepare_and_auth_reset(). If a later component
fails to load, the code jumps to these error labels to unmap the memory.

However, there is no call to qcom_pas_shutdown() on this error path before
unmapping the carveout. Since the remoteproc core does not call ops->stop()
if ops->start() fails, will the processors be left executing in TrustZone
while their backing memory is removed, potentially leading to fatal memory
translation faults?

> +release_secondary_metadata:
> +	if (pas->secondary_pas_id)
> +		qcom_pas_metadata_release(pas->secondary_pas_ctx);
> +unmap_secondary_dtb_carveout:
> +	if (pas->secondary_dtb_pas_id)
> +		qcom_pas_unmap_carveout(rproc, pas->secondary_dtb_mem_phys,
> +					pas->secondary_dtb_mem_size);
[ ... ]
> @@ -868,6 +1055,8 @@ static int qcom_pas_probe(struct platform_device *pdev)
>  	pas->pas_id = desc->pas_id;
>  	pas->lite_pas_id = desc->lite_pas_id;
>  	pas->lite_dtb_pas_id = desc->lite_dtb_pas_id;
> +	pas->secondary_pas_id = desc->secondary_pas_id;
> +	pas->secondary_dtb_pas_id = desc->secondary_dtb_pas_id;

[Severity: Low]
Can this unconditional assignment break device probe if the firmware name
property is missing?

Here, pas->secondary_pas_id and secondary_dtb_pas_id are unconditionally
assigned from the descriptor. Later in qcom_pas_probe(), the exact same
assignments are duplicated inside conditional blocks:

    if (secondary_fw_name) {
        pas->secondary_firmware_name = secondary_fw_name;
        pas->secondary_pas_id = desc->secondary_pas_id;
    }

Because of the initial unconditional assignment, if a valid descriptor
provides an ID but the firmware name property is missing or NULL,
pas->secondary_firmware_name remains NULL while pas->secondary_pas_id is
non-zero.

Will this cause qcom_pas_load() to later call request_firmware() with a NULL
name, failing with -EINVAL and preventing boot instead of gracefully
disabling the absent secondary features?

>  	pas->info_name = desc->sysmon_name;
>  	pas->smem_host_id = desc->smem_host_id;
>  	pas->decrypt_shutdown = desc->decrypt_shutdown;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-oob-support-glymur-v1-0-96cc8ffcced3@oss.qualcomm.com?part=2