Re: [PATCH v3 4/7] remoteproc: sunxi: Add Allwinner A523 XuanTie E906 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] remoteproc: sunxi: missing IRQ sync in sunxi_rproc_stop() races crash handler
- [High] remoteproc: sunxi: race and UAF in sunxi_rproc_vq_work() on notifyids
--

commit f895edd61f6cf2e80a5341959b347c747139fe48
Author: Tim Michals <tcmichals@gmail.com>

remoteproc: sunxi: Add Allwinner A523 XuanTie E906 driver

This commit adds a remoteproc driver for the XuanTie E906 RISC-V core on
Allwinner A523, A527, and T527 SoCs, handling power domains, clocks, resets,
SRAM mappings, memory-region carveouts, mailbox notifications, and crash
reporting.

> diff --git a/drivers/remoteproc/sunxi_rproc.c b/drivers/remoteproc/sunxi_rproc.c
> new file mode 100644
> index 0000000000000..1cee7110f0b8c
> --- /dev/null
> +++ b/drivers/remoteproc/sunxi_rproc.c
> @@ -0,0 +1,855 @@

[ ... ]

> +VISIBLE_IF_KUNIT
> +int sunxi_rproc_vq_notify(int id, void *ptr, void *data)
> +{
> +	KUNIT_STATIC_STUB_REDIRECT(sunxi_rproc_vq_notify, id, ptr, data);
> +	rproc_vq_interrupt(data, id);
> +	return 0;
> +}
> +EXPORT_SYMBOL_IF_KUNIT(sunxi_rproc_vq_notify);
> +
> +/* One channel carries all notifications: check every vring */
> +VISIBLE_IF_KUNIT
> +void sunxi_rproc_vq_work(struct work_struct *work)
> +{
> +	struct sunxi_rproc *priv = container_of(work, struct sunxi_rproc, vq_work);
> +
> +	idr_for_each(&priv->rproc->notifyids, sunxi_rproc_vq_notify, priv->rproc);
> +}
> +EXPORT_SYMBOL_IF_KUNIT(sunxi_rproc_vq_work);

[Severity: High]
As reported by Sashiko on v2 [1], in drivers/remoteproc/sunxi_rproc.c, can
sunxi_rproc_vq_work() race with rproc_resource_cleanup() during
rproc_shutdown(), rproc_boot_recovery(), or sunxi_rproc_remove() ->
rproc_del() and trigger a use-after-free?

Because priv->mbox_chan stays active across remoteproc stops, crash
recoveries, and throughout rproc_del() in sunxi_rproc_remove(), and neither
sunxi_rproc_stop() nor sunxi_rproc_unprepare() cancels priv->vq_work,
sunxi_rproc_vq_work() can run concurrently with rproc_resource_cleanup().

In sunxi_rproc_vq_work(), idr_for_each() iterates priv->rproc->notifyids
without holding priv->rproc->lock (unlike stm32_rproc_mb_vq_work() or
imx_dsp_rproc_vq_work()) and without RCU read-side protection (which cannot
be held across sunxi_rproc_vq_notify() because rproc_vq_interrupt() invokes
virtqueue callbacks that can sleep):

  lib/idr.c:idr_for_each()
    radix_tree_for_each_slot(slot, &idr->idr_rt, &iter, 0)
      fn(id, rcu_dereference_raw(*slot), data)

While sunxi_rproc_vq_notify() is executing, a concurrent teardown runs:

  rproc_resource_cleanup()
    platform_device_unregister(rvdev->pdev)
      rproc_virtio_remove()
        rproc_free_vring()
          idr_remove(&rproc->notifyids, rvring->notifyid)

If removing the vring frees the underlying struct radix_tree_node via
call_rcu(), the grace period can finish while sunxi_rproc_vq_notify() sleeps,
leaving radix_tree_next_slot() in idr_for_each() to dereference a freed slot
pointer.

In addition, although __rproc_virtio_del_vqs() clears rvring->vq and runs
synchronize_srcu(&rproc->vq_srcu) during rproc_stop_subdevices(), the rvring
entry stays in rproc->notifyids until platform_device_unregister(rvdev->pdev)
removes it and immediately frees the devm-allocated struct rproc_vdev
without another synchronize_srcu(&rproc->vq_srcu). If rproc_vq_interrupt()
looks up rvring in rproc->notifyids just before idr_remove(), it can
dereference rvring->vq after rvdev has been freed.

Should sunxi_rproc_vq_work() hold mutex_lock(&priv->rproc->lock) around
idr_for_each(), or should the driver synchronize and stop queuing vq_work
before rproc_resource_cleanup() tears down the vrings?

> +
> +/*
> + * Mailbox rx callbacks run in atomic context, but the virtio/rpmsg receive
> + * path takes mutexes, so the vring notifications are handled from a work item.
> + */
> +VISIBLE_IF_KUNIT
> +void sunxi_rproc_mb_rx_callback(struct mbox_client *cl, void *data)
> +{
> +	struct sunxi_rproc *priv = container_of(cl, struct sunxi_rproc, cl);
> +
> +	queue_work(priv->workqueue, &priv->vq_work);
> +}
> +EXPORT_SYMBOL_IF_KUNIT(sunxi_rproc_mb_rx_callback);
> +
> +VISIBLE_IF_KUNIT
> +irqreturn_t sunxi_rproc_crash_handler(int irq, void *data)
> +{
> +	struct sunxi_rproc *priv = data;
> +
> +	/*
> +	 * Stays masked until the next start(); recovery runs stop() first. A
> +	 * stop() that already disarmed it has disabled the line itself.
> +	 */
> +	if (test_and_clear_bit(SUNXI_RPROC_CRASH_IRQ_ARMED, &priv->flags))
> +		disable_irq_nosync(irq);
> +
> +	rproc_report_crash(priv->rproc, RPROC_FATAL_ERROR);
> +
> +	return IRQ_HANDLED;
> +}
> +EXPORT_SYMBOL_IF_KUNIT(sunxi_rproc_crash_handler);

[ ... ]

> +VISIBLE_IF_KUNIT
> +int sunxi_rproc_start(struct rproc *rproc)
> +{
> +	struct sunxi_rproc *priv = rproc->priv;
> +	const struct sunxi_rproc_cfg *cfg = priv->cfg;
> +	int ret;
> +
> +	dev_dbg(priv->dev, "Starting %s core at entry 0x%llx\n",
> +		cfg->name, (u64)rproc->bootaddr);
> +
> +	if (rproc->bootaddr > U32_MAX)
> +		return -EINVAL;
> +
> +	/* Program the reset vector and flush it before releasing the core */
> +	sunxi_rproc_writel((u32)rproc->bootaddr, priv->cfg_va + cfg->boot_reg_offset);
> +	sunxi_rproc_readl(priv->cfg_va + cfg->boot_reg_offset);
> +
> +	ret = reset_control_deassert(priv->rst_core);
> +	if (ret) {
> +		dev_err(priv->dev, "failed to release core reset: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (priv->crash_irq > 0 &&
> +	    !test_and_set_bit(SUNXI_RPROC_CRASH_IRQ_ARMED, &priv->flags))
> +		enable_irq(priv->crash_irq);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_IF_KUNIT(sunxi_rproc_start);
> +
> +static int sunxi_rproc_stop(struct rproc *rproc)
> +{
> +	struct sunxi_rproc *priv = rproc->priv;
> +	const struct sunxi_rproc_cfg *cfg = priv->cfg;
> +
> +	dev_dbg(priv->dev, "Halting %s core...\n", cfg->name);
> +
> +	/* Assert reset so the core stops executing */
> +	reset_control_assert(priv->rst_core);
> +
> +	if (priv->crash_irq > 0 &&
> +	    test_and_clear_bit(SUNXI_RPROC_CRASH_IRQ_ARMED, &priv->flags))
> +		disable_irq(priv->crash_irq);
> +
> +	return 0;
> +}

[Severity: High]
As reported by Sashiko on v2 [2], in drivers/remoteproc/sunxi_rproc.c, what
happens if sunxi_rproc_stop() runs right after sunxi_rproc_crash_handler()
clears SUNXI_RPROC_CRASH_IRQ_ARMED, while the threaded handler is still
executing?

Because sunxi_rproc_crash_handler() is a preemptible IRQ thread, it can clear
SUNXI_RPROC_CRASH_IRQ_ARMED and be preempted before calling
disable_irq_nosync(irq) or rproc_report_crash(priv->rproc, RPROC_FATAL_ERROR):

  CPU0 (IRQ thread)                    CPU1 (rproc_shutdown / recovery)
  sunxi_rproc_crash_handler()
    test_and_clear_bit(
      SUNXI_RPROC_CRASH_IRQ_ARMED,
      &priv->flags) -> true
                                       sunxi_rproc_stop()
                                         test_and_clear_bit(
                                           SUNXI_RPROC_CRASH_IRQ_ARMED,
                                           &priv->flags) -> false
                                         /* returns without waiting */

If a subsequent rproc_boot() calls sunxi_rproc_start() before CPU0 reaches
disable_irq_nosync(irq), sunxi_rproc_start() sets SUNXI_RPROC_CRASH_IRQ_ARMED
and calls enable_irq(priv->crash_irq) while the IRQ disable depth is still 0,
triggering an unbalanced enable warning in __enable_irq(). When CPU0 then
runs disable_irq_nosync(irq), the IRQ is left disabled while
SUNXI_RPROC_CRASH_IRQ_ARMED is set, and the next sunxi_rproc_stop() will
increment the disable depth to 2, leaving the crash IRQ permanently masked on
later boots.

Furthermore, because sunxi_rproc_stop() does not call
synchronize_irq(priv->crash_irq) when test_and_clear_bit() returns false, the
in-flight sunxi_rproc_crash_handler() thread can call rproc_report_crash()
after the stop (and a subsequent restart) has already completed.

Should sunxi_rproc_stop() still call synchronize_irq(priv->crash_irq) when
priv->crash_irq > 0 and SUNXI_RPROC_CRASH_IRQ_ARMED was already cleared by
the crash handler?

[1] https://lore.kernel.org/r/20260927003338.CB0E21F000FF@smtp.kernel.org
[2] https://lore.kernel.org/r/20260927003338.CB0E21F000FF@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101185735.762559-1-tcmichals@gmail.com?part=4