Re: [PATCH v2 3/4] remoteproc: qcom: pas: Add coredump support for secondary processors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] remoteproc: qcom: coredump completely aborted if secondary memory map fails
- [High] mm: UAF of rproc in qcom_pas_probe() if rproc_add() fails
--

commit 78eeb93e0081fe18e5c14c65d05d13fbc15c17c8
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

remoteproc: qcom: pas: Add coredump support for secondary processors

Add coredump support for PAS remoteproc instances with a secondary
processor by registering fixed dump segments for both primary and
secondary carveouts at probe time and mapping them during collection.
The attached-state path is for debug dump collection and does not
provide operational device recovery.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 0bce2f0d5b4aa..374cafd04b625 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
[ ... ]
> @@ -716,13 +772,21 @@ static void qcom_pas_coredump(struct rproc *rproc)
>  {
>  	struct qcom_pas *pas = rproc->priv;
>  
> -	pas->mem_region = qcom_pas_ctx_map(pas->pas_ctx);
> +	if (!pas->mem_region)
> +		pas->mem_region = qcom_pas_ctx_map(pas->pas_ctx);
>  	if (!pas->mem_region)
>  		return;
>  
> +	if (qcom_pas_has_secondary(pas) && !pas->secondary_mem_region) {
> +		pas->secondary_mem_region =
> +			qcom_pas_ctx_map(pas->secondary_pas_ctx);
> +		if (!pas->secondary_mem_region) {
> +			dev_err(pas->dev, "unable to map secondary memory for coredump\n");
> +			return;
> +		}
> +	}
> +
>  	rproc_coredump(rproc);

[Severity: High]
If the secondary memory mapping fails here, does the early return
unintentionally bypass the rproc_coredump() call completely? 

Since the primary processor memory was successfully mapped just above,
and the remoteproc framework's segment copy gracefully handles unmapped
segments, should it log the error and still proceed to dump the primary
core's memory rather than aborting?

[ ... ]
> @@ -1133,6 +1197,12 @@ static int qcom_pas_probe(struct platform_device *pdev)
>  	if (desc->early_boot)
>  		pas->rproc->state = RPROC_DETACHED;
>  
> +	if (qcom_pas_has_secondary(pas)) {
> +		ret = qcom_pas_register_dump_segments(rproc, pas);
> +		if (ret)
> +			goto remove_ssr_sysmon;
> +	}
> +
>  	ret = rproc_add(rproc);
>  	if (ret)
>  		goto remove_ssr_sysmon;
> @@ -1144,6 +1214,7 @@ static int qcom_pas_probe(struct platform_device *pdev)
>  	return 0;
>  
>  remove_ssr_sysmon:
> +	rproc_coredump_cleanup(rproc);

[Severity: High]
Can this sequence cause a use-after-free if rproc_add() fails?

Looking at rproc_add(), if device_add() fails internally, it explicitly
calls put_device() which drops the initial reference created by
devm_rproc_alloc(), freeing the rproc structure:

rproc_add() {
    ...
    ret = device_add(dev);
    if (ret < 0) {
        put_device(dev);
        goto rproc_remove_cdev;
    }
    ...
}

If we jump to remove_ssr_sysmon on rproc_add() failure, doesn't
rproc_coredump_cleanup() then try to access the already freed
rproc structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-oob-support-glymur-v2-0-715435f52dd7@oss.qualcomm.com?part=3