Re: [PATCH 5/7] remoteproc: sunxi: add allwinner riscv remoteproc driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] remoteproc: integer overflow in da_to_va() enables arbitrary kernel memory read/write
- [High] remoteproc: redundant memory regions parsing creates conflicting WC and WB aliases
- [High] remoteproc: panic on deferred rx mailbox probe due to ERR_PTR deref and uninit work
- [High] remoteproc: incorrect bounds check intercepts Space 1 memory accesses
- [High] remoteproc: bus fault during recovery due to writing registers under reset
- [High] remoteproc: UAF in workqueue due to inverted stop and remove teardown order
- [High] remoteproc: stack use-after-free by passing local variable to mbox_send_message()
--

commit e3392de86ee7ea419c734b1a14f8a424ed2fb7b6
Author: Tim Michals <tcmichals@gmail.com>
Subject: remoteproc: sunxi: add allwinner riscv remoteproc driver

This commit adds a remoteproc driver for the XuanTie E906/E907 RISC-V remote
processor on Allwinner SoCs. It integrates with the Common Clock Framework
and Reset Framework, maps internal TCM and dedicated SRAM windows, and connects
with hardware mailboxes for VirtIO RPMsg IPC.

> diff --git a/drivers/remoteproc/sunxi_rproc.c b/drivers/remoteproc/sunxi_rproc.c
> new file mode 100644
> index 0000000000000..4cbffc7b8277a
> --- /dev/null
> +++ b/drivers/remoteproc/sunxi_rproc.c
[ ... ]
> +static int sunxi_rproc_start(struct rproc *rproc)
> +{
> +	struct sunxi_rproc *priv = rproc->priv;
> +	int ret;
> +
> +	dev_info(priv->dev, "Starting %s core at entry 0x%llx\n",
> +		 priv->cfg ? priv->cfg->name : "remote", (u64)rproc->bootaddr);
> +
> +	if (rproc->bootaddr > U32_MAX)
> +		return -EINVAL;
> +
> +	/* XuanTie RISC-V boot sequence */
> +	if (priv->cfg_va) {
> +		writel((u32)rproc->bootaddr, priv->cfg_va + E906_STA_ADD_REG);
> +		dev_dbg(priv->dev, "STA_ADD set to 0x%08x\n", (u32)rproc->bootaddr);
> +	}
> +
> +	/* Release core reset to begin execution */
> +	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;
> +		}
> +	} else if (priv->rst_cfg) {
> +		ret = reset_control_deassert(priv->rst_cfg);

[Severity: High]
If we trigger a recovery sequence (which calls stop() and then start() directly
without calling prepare()), rst_cfg will still be asserted here. Doesn't
writing to STA_ADD_REG slightly above while the interconnect bus is in reset
cause a synchronous external abort (bus fault)?

[ ... ]
> +static int sunxi_rproc_stop(struct rproc *rproc)
> +{
> +	struct sunxi_rproc *priv = rproc->priv;
> +
> +	cancel_work_sync(&priv->vq_work);
> +
> +	dev_info(priv->dev, "Halting %s core...\n",
> +		 priv->cfg ? priv->cfg->name : "remote");
> +
> +	if (priv->rst_core)
> +		reset_control_assert(priv->rst_core);

[Severity: High]
Does calling cancel_work_sync() before actually halting the core or disabling
the mailbox interrupt leave a window for the work to be rescheduled?

[ ... ]
> +static void sunxi_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct sunxi_rproc *priv = rproc->priv;
> +	int ret;
> +
> +	if (!priv->tx_chan)
> +		return;
> +
> +	ret = mbox_send_message(priv->tx_chan, (void *)&vqid);

[Severity: High]
Since priv->cl.tx_block is set to false during probe, mbox_send_message() will
place the pointer in a queue and return immediately if the hardware mailbox is
busy. Does passing a pointer to the local stack variable vqid cause a stack
use-after-free when the mailbox asynchronous ticker later reads this pointer
after the function has already returned?

[ ... ]
> +static void *sunxi_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is_iomem)
> +{
> +	struct sunxi_rproc *priv = rproc->priv;
> +
> +	if (len == 0)
> +		return NULL;
> +
> +	/* 1. Dedicated MCU Local SRAM Space 0 (Resource "r_sram" / "sram") */
> +	if (priv->r_sram_va) {
> +		/* Host physical address view (e.g., 0x07280000, 0x07200000, 0x00020000) */
> +		if (da >= priv->r_sram_phys &&
> +		    (da + len) <= (priv->r_sram_phys + priv->r_sram_size)) {

[Severity: Critical]
The da value is a 64-bit address read directly from the ELF segment header.
If a maliciously crafted ELF provides a very large da (for example, close to
U64_MAX), does the addition da + len overflow and wrap around? This would
bypass the upper limit check and allow arbitrary kernel memory to be mapped
and overwritten during the firmware loading phase.

> +			if (is_iomem)
> +				*is_iomem = true;
> +			return priv->r_sram_va + (da - priv->r_sram_phys);
> +		}
> +		/* Core DA view: 0x40000000 */
> +		if (da >= 0x40000000 &&
> +		    (da + len) <= (0x40000000 + priv->r_sram_size)) {

[Severity: High]
The address 0x40000000 is the start address for SRAM Space 1
(E907_SRAM_SPACE1_DA), whereas Space 0 should map to 0x3ff80000 or 0x3ffc0000.
Does checking da >= 0x40000000 here intercept valid Space 1 accesses and
incorrectly translate them into Space 0's mapping?

[ ... ]
> +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 = priv->r_sram1_va;
> +		else if (priv->r_sram_va && res.start == priv->r_sram_phys)
> +			va = priv->r_sram_va;
> +		else
> +			va = devm_ioremap_wc(dev, res.start, resource_size(&res));

[Severity: High]
The trace and dram regions were already mapped earlier in
sunxi_rproc_register_mem(). Does re-mapping them here overwrite those initial
pointers and leak the mappings?

Also, there is no return or continue inside the if blocks handling trace and
dram. This allows execution to fall through to the else branch, mapping the
exact same physical region a third time using devm_ioremap_wc(). Does having
simultaneous Write-Back (WB) and Write-Combining (WC) aliases to the same
physical memory violate ARM64 constraints?

[ ... ]
> +static int sunxi_rproc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	if (priv->tx_chan) {
> +		priv->rx_chan = mbox_request_channel_byname(&priv->cl, "rx");
> +		if (IS_ERR(priv->rx_chan)) {
> +			if (PTR_ERR(priv->rx_chan) == -EPROBE_DEFER) {
> +				ret = -EPROBE_DEFER;
> +				goto err_mbox_release;
> +			}
> +			dev_info(dev, "no rx mailbox channel configured\n");
> +			priv->rx_chan = NULL;
> +		}
> +	}
[ ... ]
> +err_mbox_release:
> +	cancel_work_sync(&priv->vq_work);
> +	if (priv->rx_chan)
> +		mbox_free_channel(priv->rx_chan);

[Severity: High]
If we hit the -EPROBE_DEFER path above, the code jumps directly to
err_mbox_release before INIT_WORK() is called on priv->vq_work. Doesn't calling
cancel_work_sync() on an uninitialized work struct trigger a BUG or warning?

Furthermore, priv->rx_chan holds an ERR_PTR in this failure path. Because it
is not NULL, the if (priv->rx_chan) check will succeed, and mbox_free_channel()
will be called with the ERR_PTR, causing a kernel panic when it dereferences it.

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

[Severity: High]
Similar to the logic in stop(), the work synchronization barrier is executed
before the interrupt sources are stopped. Can a late interrupt re-queue the
work just before the driver context is freed by devres, leading to a
use-after-free when the delayed work eventually executes?

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