From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 77B791D54FA; Sun, 27 Sep 2026 00:33:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790469220; cv=none; b=t31m1LkMlGTKPPPnw1qw6dQCwd1OLVyajAwLXJ4lwaiIApDcT2NN6hj8hI19UXrI8tgSY/EhmJwl5FV+46pqevdkmPE+mhrbWO3WqdgRj/mGNDdgR+Cy2Te7Rc3wC3lmpnWCV/saJTd/+luq9ojVEzg/Da771TWrC1c9cFmfmbM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790469220; c=relaxed/simple; bh=q327XeqqI8gQTR3uth+c3vZx5GDF2iWRYzc7FHKricg=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=k200UtMGYWcDck1WT9WNzThCBpDWrz2SVqF9fGp156xVF+D2+sGeHmeVIhpFkAYXQSS5f44TyHGTK89Hcal0+8ieYoPPC9yQg/hWVk0VFDVUIPuYzchSFWNkfxIFm3eBYTM2UTPFDHxBh1OrUh3c8MJ8x7i3Gha2+zzm8GuyKq4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=baC2EbqK; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="baC2EbqK" Received: by smtp.kernel.org (Postfix) with ESMTPSA id CB0E21F000FF; Sun, 27 Sep 2026 00:33:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1790469219; bh=bzJh2atWvK8VAlXleJPm8Es8rlqFmh+gHDftz+bJXlc=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=baC2EbqKYE+JaEiuJMpxTNzGg7MpRoKt5QdQzpI1mKp7kMuauZ/CaYq5HZXSr8sPE UgzRtE0F2T0TyEEoEmtEc5kSgY3gs9iyxvgkNfLFj2U/uu8sxAtXKQYSykoZEVqg07 uSr5rSQxH7nnT5Cm8W7KauiskyWBqzPXJlRmjlcE57/zeMzgCp12RJIGdMQi5xVyH8 8/oc2FQpl4nL09Uia7AAlNl9EVjVoPwK5oQNGNK7Vd78nkCb4LCjsfZ2IUKgeBVN0V DZTjA/4oxpW3xiaONsz99TOqSkMBEkq2avDcnJGQDgwbxoO2P1txGipCo4Cf7XsFJs urSeQqtkNHWDQ== From: sashiko-bot@kernel.org Subject: Re: [PATCH v2 5/7] remoteproc: sunxi: add Allwinner XuanTie RISC-V remoteproc driver Reply-To: sashiko-reviews@lists.linux.dev To: "Tim Michals" Cc: devicetree@vger.kernel.org, robh@kernel.org, conor+dt@kernel.org, linux-sunxi@lists.linux.dev In-Reply-To: <20260927002021.797069-6-tcmichals@gmail.com> References: <20260922034711.190253-1-tcmichals@gmail.com> <20260927002021.797069-1-tcmichals@gmail.com> <20260927002021.797069-6-tcmichals@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Sun, 27 Sep 2026 00:33:38 +0000 Message-Id: <20260927003338.CB0E21F000FF@smtp.kernel.org> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: 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 ra= ce - [High] remoteproc: sunxi: race on kick_msg and immediate txdone corrupts = mailbox - [High] remoteproc: sunxi: double mapping of DT regions with conflicting a= ttributes - [High] remoteproc: sunxi: premature core execution due to broken reset fa= llback logic - [High] remoteproc: sunxi: UAF of virtqueues due to late mailbox interrupts - [High] remoteproc: sunxi: UAF of priv in probe error path due to workqueu= e 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 a= s host PAs - [Medium] remoteproc: sunxi: missing teardown of crash IRQ on start failur= e leaks state - [Medium] remoteproc: sunxi: missing write flush of boot address causes ex= ecution race -- commit e007f2bf04fe297a7d71605516a9a9ee48e727ef Author: Tim Michals 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 =3D data; > + struct rproc *rproc =3D 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 =3D 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 remo= ved, 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() evalua= tes it, the remove path skips disable_irq(). It then continues to rproc_del() a= nd frees the rproc structure while the interrupt handler is concurrently execu= ting rproc_report_crash(rproc, RPROC_FATAL_ERROR). [ ... ] > +int sunxi_rproc_prepare(struct rproc *rproc) > +{ > + struct sunxi_rproc *priv =3D rproc->priv; > + const struct sunxi_rproc_cfg *cfg =3D priv->cfg ? priv->cfg : &sun55i_r= iscv_cfg; > + int ret; > + > + /* 1. Deassert configuration & SRAM bus resets */ > + if (priv->rst_cfg) { > + ret =3D 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 =3D reset_control_deassert(priv->rst_core); ... } else if (priv->rst_cfg) { ret =3D reset_control_deassert(priv->rst_cfg); Because rst_cfg was already deasserted in prepare(), the remote core actual= ly 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 bootadd= r */ > + if (priv->rst_core) { > + ret =3D 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 =3D 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=3Dfalse, 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 =3D (u32)vqid; > + ret =3D mbox_send_message(priv->tx_chan, &priv->kick_msg); > + if (ret < 0) > + dev_err_ratelimited(priv->dev, "failed to send mailbox kick: %d\n", re= t); > + > + 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 se= nd 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 =3D 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) =3D=3D 0) { > + if (priv->r_sram_va && sys >=3D priv->r_sram_phys && > + (sys + len) <=3D (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 >=3D priv->trace_phys && [Severity: Medium] Should there be an early return here if the ATT translation succeeds but fa= ils to match a valid mapped memory window? If sunxi_rproc_da_to_sys() returns 0 but the resulting sys address isn't fo= und 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 =3D res.start; > + priv->trace_size =3D resource_size(&res); > + priv->trace_va =3D devm_memremap(dev, res.start, resource_size(&res), > + MEMREMAP_WB); [ ... ] > + } else if (name && (strstr(name, "dram") || strstr(name, "vram"))) { > + priv->dram_phys =3D res.start; > + priv->dram_size =3D resource_size(&res); > + priv->dram_va =3D 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 =3D=3D priv->r_sram1_phys) > + va =3D (__force void *)priv->r_sram1_va; > + else if (priv->r_sram_va && res.start =3D=3D priv->r_sram_phys) > + va =3D (__force void *)priv->r_sram_va; > + else > + va =3D (__force void *)devm_ioremap_wc(dev, res.start, resource_size(= &res)); [Severity: High] Does this establish conflicting memory mappings for the trace and dram regi= ons? 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 =3D platform_get_drvdata(pdev); > + struct sunxi_rproc *priv =3D rproc->priv; [ ... ] > + rproc_del(rproc); > + > + if (priv->rx_chan) { > + mbox_free_channel(priv->rx_chan); > + priv->rx_chan =3D 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_wor= k, which unconditionally calls rproc_vq_interrupt(). Executing this against the already destroyed virtqueues will result in a crash. --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260927002021.7970= 69-1-tcmichals@gmail.com?part=3D5