Re: [PATCH v6 5/6] remoteproc: qcom: pas: Add late attach support for subsystems

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Calling `rproc_report_crash()` followed by returning an error from `.attach()` causes critical Use-After-Free vulnerabilities.
- [High] Returning `-EINVAL` when the subsystem is not ready breaks the intended fallback to offline boot and causes device probe failure.
- [Medium] The `handover_irq` is left enabled on multiple error paths, causing an unbalanced IRQ enablement depth.
--

commit 1111bc6bad772baf2e724e0d339a047f01859d2f
Author: Jingyi Wang <jingyi.wang@oss.qualcomm.com>

remoteproc: qcom: pas: Add late attach support for subsystems

This commit adds late attach support for subsystems brought out of reset by
early bootloaders. It introduces qcom_pas_attach() to check subsystem state
via SMP2P bits, reporting crashes or falling back to offline mode.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c

[ ... ]

> @@ -510,6 +511,57 @@ static unsigned long qcom_pas_panic(struct rproc *rproc)
>  	return qcom_q6v5_panic(&pas->q6v5);
>  }
>  
> +static int qcom_pas_attach(struct rproc *rproc)
> +{
> +	int ret;
> +	struct qcom_pas *pas = rproc->priv;
> +	bool ready_state;
> +	bool crash_state;
> +
> +	pas->q6v5.handover_issued = true;
> +	enable_irq(pas->q6v5.handover_irq);
> +
> +	pas->q6v5.running = true;
> +	ret = irq_get_irqchip_state(pas->q6v5.fatal_irq,
> +				    IRQCHIP_STATE_LINE_LEVEL, &crash_state);
> +
> +	if (ret)
> +		goto disable_running;

If ret is non-zero, we jump to disable_running and return an error without
calling disable_irq(). Does this leave the handover_irq enabled?

Since the attach fails, the remoteproc core never calls the stop callback,
meaning qcom_q6v5_unprepare() is bypassed. This might permanently leak the
IRQ enable count.

> +
> +	if (crash_state) {
> +		dev_err(pas->dev, "Subsystem has crashed before driver probe\n");
> +		rproc_report_crash(rproc, RPROC_FATAL_ERROR);
> +		ret = -EINVAL;
> +		goto disable_running;
> +	}

Could calling rproc_report_crash() and then returning an error lead to
use-after-free conditions?

rproc_report_crash() asynchronously queues the crash_handler work on
rproc_recovery_wq. If auto_boot is true, the attach failure causes
rproc_add() to abort, and devres will free the rproc object. The queued
crash handler is not explicitly cancelled during cleanup, so it may
dereference the freed rproc memory when it executes.

If auto_boot is false, rproc isn't freed, but rproc_attach() cleans up its
failure by calling rproc_disable_iommu(), which frees rproc->domain. The
async crash handler later executes, fails to abort on the RPROC_DETACHED
state, sets the state to RPROC_CRASHED, and triggers recovery. The recovery
process calls rproc_stop() -> qcom_pas_stop() -> qcom_pas_unmap_carveout(),
which unconditionally dereferences the freed rproc->domain via
iommu_unmap().

Additionally, does this path leave the handover_irq enabled as well?

> +
> +	ret = irq_get_irqchip_state(pas->q6v5.ready_irq,
> +				    IRQCHIP_STATE_LINE_LEVEL, &ready_state);
> +
> +	if (ret)
> +		goto disable_running;
> +
> +	if (unlikely(!ready_state)) {
> +		/*
> +		 * The bootloader may not support early boot, mark the state as
> +		 * RPROC_OFFLINE so that the PAS driver can load the firmware and
> +		 * start the remoteproc.
> +		 */
> +		dev_err(pas->dev, "Failed to get subsystem ready interrupt\n");
> +		pas->rproc->state = RPROC_OFFLINE;
> +		disable_irq(pas->q6v5.handover_irq);
> +		ret = -EINVAL;
> +		goto disable_running;
> +	}

Does returning -EINVAL here prevent the intended fallback to offline boot?

If the device has auto_boot enabled, rproc_add() treats the -EINVAL from
the auto-boot attempt as a fatal error. It executes goto rproc_remove_dev,
which destroys the remoteproc device entirely and fails the driver probe.
This makes the intended fallback impossible, as the device is never
registered.

> +
> +	return 0;
> +
> +disable_running:
> +	pas->q6v5.running = false;
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-knp-soccp-v6-0-cf5d0e194b5f@oss.qualcomm.com?part=5