Re: [PATCH v2 5/7] remoteproc: sunxi: add Allwinner XuanTie RISC-V remoteproc driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] remoteproc: sunxi: unbalanced disable_irq via crash_irq_enabled race
- [High] remoteproc: sunxi: race on kick_msg and immediate txdone corrupts mailbox
- [High] remoteproc: sunxi: double mapping of DT regions with conflicting attributes
- [High] remoteproc: sunxi: premature core execution due to broken reset fallback logic
- [High] remoteproc: sunxi: UAF of virtqueues due to late mailbox interrupts
- [High] remoteproc: sunxi: UAF of priv in probe error path due to workqueue teardown
- [High] remoteproc: sunxi: UAF of rproc in remove due to crash_irq_enabled data race
- [Medium] remoteproc: sunxi: da_to_va translates unmatched ATT addresses as host PAs
- [Medium] remoteproc: sunxi: missing teardown of crash IRQ on start failure leaks state
- [Medium] remoteproc: sunxi: missing write flush of boot address causes execution race
--

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

remoteproc: sunxi: add Allwinner XuanTie RISC-V remoteproc driver

Add remoteproc driver for the auxiliary Alibaba T-Head XuanTie E907
RISC-V co-processor found on Allwinner A523, A527, and T527 SoCs.

> diff --git a/drivers/remoteproc/sunxi_rproc.c b/drivers/remoteproc/sunxi_rproc.c
> index 0000000000000..fbb0ee8a91105
> --- /dev/null
> +++ b/drivers/remoteproc/sunxi_rproc.c
[ ... ]
> +static irqreturn_t sunxi_rproc_crash_handler(int irq, void *data)
> +{
> +	struct sunxi_rproc *priv = data;
> +	struct rproc *rproc = priv->rproc;
> +
> +	dev_err(priv->dev, "Hardware crash event received from %s core!\n",
> +		priv->cfg ? priv->cfg->name : "remote");
> +	if (priv->crash_irq_enabled) {
> +		disable_irq_nosync(irq);
> +		priv->crash_irq_enabled = false;
> +	}
> +	rproc_report_crash(rproc, RPROC_FATAL_ERROR);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
Could this lockless check of priv->crash_irq_enabled race with
sunxi_rproc_stop() or sunxi_rproc_remove()?

If a hardware crash occurs concurrently with the core being stopped or removed,
both threads can evaluate priv->crash_irq_enabled as true. Both paths will
independently disable the IRQ, incrementing the disable depth to 2.

When the remote core is restarted via sunxi_rproc_start(), enable_irq() is
only called once, which leaves the crash IRQ permanently masked at the
hardware level.

Also, could this race result in a Use-After-Free during removal?

If the interrupt handler clears the flag before sunxi_rproc_remove() evaluates
it, the remove path skips disable_irq(). It then continues to rproc_del() and
frees the rproc structure while the interrupt handler is concurrently executing
rproc_report_crash(rproc, RPROC_FATAL_ERROR).

[ ... ]
> +int sunxi_rproc_prepare(struct rproc *rproc)
> +{
> +	struct sunxi_rproc *priv = rproc->priv;
> +	const struct sunxi_rproc_cfg *cfg = priv->cfg ? priv->cfg : &sun55i_riscv_cfg;
> +	int ret;
> +
> +	/* 1. Deassert configuration & SRAM bus resets */
> +	if (priv->rst_cfg) {
> +		ret = reset_control_deassert(priv->rst_cfg);

[Severity: High]
Does this cause the core to start executing prematurely if rst_cfg acts as the
execution reset?

In sunxi_rproc_prepare(), rst_cfg is unconditionally deasserted to allow
register access. Later in sunxi_rproc_start(), if the optional rst_core is
missing, the code falls back to deasserting rst_cfg again:

int sunxi_rproc_start(struct rproc *rproc)
{
    ...
    if (priv->rst_core) {
        ret = reset_control_deassert(priv->rst_core);
    ...
    } else if (priv->rst_cfg) {
        ret = reset_control_deassert(priv->rst_cfg);

Because rst_cfg was already deasserted in prepare(), the remote core actually
starts executing before the firmware segments are fully loaded and before
the boot address is programmed.

[ ... ]
> +int sunxi_rproc_start(struct rproc *rproc)
> +{
[ ... ]
> +	/*
> +	 * Program boot vector while the core execution reset is held.
> +	 * The CFG block bus was un-gated during prepare() via rst_cfg.
> +	 */
> +	if (priv->cfg_va) {
> +		writel((u32)rproc->bootaddr, priv->cfg_va + cfg->boot_reg_offset);
> +		dev_dbg(priv->dev, "STA_ADD set to 0x%08x\n", (u32)rproc->bootaddr);
> +	}
> +
> +	/* Release core execution reset so the core begins execution at bootaddr */
> +	if (priv->rst_core) {
> +		ret = reset_control_deassert(priv->rst_core);
> +		if (ret) {
> +			dev_err(priv->dev, "failed to release core reset: %d\n", ret);
> +			return ret;
> +		}

[Severity: Medium]
Is a read-back of the CFG register necessary to explicitly flush the posted
boot address write?

Without flushing the writel() before deasserting the core reset, the SoC
interconnect might deliver the reset deassertion before the boot address
reaches the CFG block. This could cause the core to begin execution from a
stale or zero address.

[Severity: Medium]
If reset_control_deassert() fails, does this error path leave the crash IRQ
enabled for an offline core?

Returning early bypasses the required IRQ cleanup, leaving the IRQ active
and the system vulnerable to spurious interrupts during retry attempts.

[ ... ]
> +void sunxi_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct sunxi_rproc *priv = rproc->priv;
> +	int ret;
> +
> +	if (!priv->tx_chan)
> +		return;
> +
> +	/*
> +	 * Use priv->kick_msg rather than a stack-local variable. The mailbox
> +	 * controller runs with tx_block=false, so mbox_send_message() may
> +	 * queue the pointer and return before the hardware reads the message.
> +	 * A stack-local vqid would be a use-after-return at that point.
> +	 */
> +	priv->kick_msg = (u32)vqid;
> +	ret = mbox_send_message(priv->tx_chan, &priv->kick_msg);
> +	if (ret < 0)
> +		dev_err_ratelimited(priv->dev, "failed to send mailbox kick: %d\n", ret);
> +
> +	mbox_client_txdone(priv->tx_chan, 0);
> +}

[Severity: High]
Does this create a data race on priv->kick_msg during concurrent kicks?

Different virtqueues on multiple CPUs can execute sunxi_rproc_kick()
concurrently. Since they both assign to the single shared priv->kick_msg
variable without a lock, a concurrent kick will overwrite it before the
hardware processes the queue, causing dropped notifications.

Also, does calling mbox_client_txdone() immediately after a non-blocking send
break the mailbox queue pacing?

By falsely signaling TX completion, the framework will submit the next
message immediately, potentially clobbering the controller's TX registers
while the first message is still physically transmitting.

[ ... ]
> +void *sunxi_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is_iomem)
> +{
> +	struct sunxi_rproc *priv = rproc->priv;
> +	u64 sys;
[ ... ]
> +	/*
> +	 * 1. Translate core-local device addresses (DA) to system bus
> +	 * addresses (Host PA) using the SoC address translation table (ATT).
> +	 */
> +	if (sunxi_rproc_da_to_sys(priv, da, len, &sys, is_iomem) == 0) {
> +		if (priv->r_sram_va && sys >= priv->r_sram_phys &&
> +		    (sys + len) <= (priv->r_sram_phys + priv->r_sram_size))
> +			return (__force void *)(priv->r_sram_va + (sys - priv->r_sram_phys));
[ ... ]
> +	}
> +
> +	/*
> +	 * 2. Device Tree Memory Regions (Trace buffer, DRAM carveout, or
> +	 * dynamically-assigned SRAM regions whose host PA is supplied via DT).
> +	 */
> +	if (priv->trace_va && da >= priv->trace_phys &&

[Severity: Medium]
Should there be an early return here if the ATT translation succeeds but fails
to match a valid mapped memory window?

If sunxi_rproc_da_to_sys() returns 0 but the resulting sys address isn't found
in any mapped window, the code falls through to the Device Tree fallback
checks.

In the fallback block, it mistakenly compares the original device address (da)
against the host physical addresses (e.g. priv->trace_phys). If a da happens
to overlap numerically with a host PA, it will improperly translate the
address and return an invalid virtual pointer.

[ ... ]
> +static int sunxi_rproc_parse_memory_regions(struct rproc *rproc)
> +{
[ ... ]
> +		if (name && (strstr(name, "trace") || of_node_name_eq(rmem_np, "trace"))) {
> +			priv->trace_phys = res.start;
> +			priv->trace_size = resource_size(&res);
> +			priv->trace_va = devm_memremap(dev, res.start, resource_size(&res),
> +						       MEMREMAP_WB);
[ ... ]
> +		} else if (name && (strstr(name, "dram") || strstr(name, "vram"))) {
> +			priv->dram_phys = res.start;
> +			priv->dram_size = resource_size(&res);
> +			priv->dram_va = devm_memremap(dev, res.start, resource_size(&res),
> +						      MEMREMAP_WB);
[ ... ]
> +		}
> +
> +		/* Reuse existing SRAM mapping if region overlaps, else ioremap */
> +		if (priv->r_sram1_va && res.start == priv->r_sram1_phys)
> +			va = (__force void *)priv->r_sram1_va;
> +		else if (priv->r_sram_va && res.start == priv->r_sram_phys)
> +			va = (__force void *)priv->r_sram_va;
> +		else
> +			va = (__force void *)devm_ioremap_wc(dev, res.start, resource_size(&res));

[Severity: High]
Does this establish conflicting memory mappings for the trace and dram regions?

After creating the initial mapping using devm_memremap() with Write-Back
cacheability (MEMREMAP_WB), the loop lacks a continue statement or an else
branch.

Execution falls through to the fallback block where it maps the exact same
physical memory again using devm_ioremap_wc() (Write-Combining). Mapping the
same physical address space concurrently with mismatched caching attributes
is undefined behavior on ARM64 and can cause silent cache corruption.

[ ... ]
> +static int sunxi_rproc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +err_mbox_release:
> +	cancel_work_sync(&priv->vq_work);
> +	/*
> +	 * mbox_request_channel_byname() can return ERR_PTR on failure.
> +	 * Guard with IS_ERR() to avoid calling mbox_free_channel() with
> +	 * an invalid pointer, which would panic on the first dereference.
> +	 */
> +	if (priv->rx_chan && !IS_ERR(priv->rx_chan))
> +		mbox_free_channel(priv->rx_chan);

[Severity: High]
Does cancelling the workqueue before shutting down the mailbox channel
introduce a Use-After-Free risk?

Because mbox_free_channel() happens after cancel_work_sync(), the mailbox
remains active. If an RX interrupt fires immediately after the cancellation,
the callback can re-queue priv->vq_work.

When the probe function returns an error, devres will free the priv
structure. The workqueue would then execute the dangling work item and
dereference the freed pointer.

[ ... ]
> +static void sunxi_rproc_remove(struct platform_device *pdev)
> +{
> +	struct rproc *rproc = platform_get_drvdata(pdev);
> +	struct sunxi_rproc *priv = rproc->priv;
[ ... ]
> +	rproc_del(rproc);
> +
> +	if (priv->rx_chan) {
> +		mbox_free_channel(priv->rx_chan);
> +		priv->rx_chan = NULL;
> +	}

[Severity: High]
Does calling rproc_del() before tearing down the mailbox expose a window for
virtqueue Use-After-Free?

rproc_del() stops the remote processor and destroys all virtio
resources and virtqueues. However, the mailbox channel remains active.

If a late interrupt is received, the mailbox IRQ callback will queue vq_work,
which unconditionally calls rproc_vq_interrupt(). Executing this against the
already destroyed virtqueues will result in a crash.

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