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 A3828175A7F; Sun, 27 Sep 2026 00:33:03 +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=1790469184; cv=none; b=fED2rrkWdzFx5MMCOvDqH9mbpG+fGotHfLQu6z2fKmgOahHMe59+W6M0As9ai+W0fyuFBc+I2zrKve4hKmhePgM1okU+YRTXLBzAzM2BS3zepiBY6PcvTakeAzchHktZ8BgDWAXAh4bswDeWKbDeB/AK7W94xfxG0GH50e8AcpE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790469184; c=relaxed/simple; bh=WwqvmEa1vg+WDFnnzZ53bexVQ4i8spCkEiUtnM7Q5i8=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=JulIJTZZP/ybhKUnVz9nruMoCUjvcWk7rVyZY5fM8kniUEM1vig03Y1R4Vw1wHSFjNZtq5/r4gxbY13NQd/kVbfKJmP+TLMQNdZxrfXMOJpMJ4LAGKOog+H3d99yGJD6LhPU5TmQNX/BQXa4qi06jqKvlv3/yq6C+NwglYXSBOY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=EBHG3tOn; 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="EBHG3tOn" Received: by smtp.kernel.org (Postfix) with ESMTPSA id D69591F000FF; Sun, 27 Sep 2026 00:33:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1790469183; bh=AowVa/f84DDfgFos0j1aelU2mgZ113mob+KTxM8h7N8=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=EBHG3tOnxjXJdJqkjZvwaVgrTZ7nHBqaKUfVdMds5wRHztLU4T1tPh2uOCqMLpbv+ mW3c0ty1usBwlDH21p9AYSeogoBbZ4UmHep1GFv8kBiMFgmWHPHhyw5wyBzjrZwKN+ osCaocgOv6WqRuMeHktiID/9QEpHjhSB2G/vsnLQhwp09vmC5kwkFAz1BzvI8+eKwY hQNlOTfbRXmwCpQqb249wfGRlVmlh1IqfMBvT6a9BNPT742jgcUsjgc0aLdQGhDYl1 bu6uUZevqCcxGrFubtgz60YHNiedwkOxt44IbTY3mZywZGuqMXsfNRdSr38+Krg8k/ Cy1D0hN9pE+bQ== From: sashiko-bot@kernel.org Subject: Re: [PATCH v2 2/7] mailbox: sun55i: add Allwinner sun55i/sun60i 4-port Message Box driver Reply-To: sashiko-reviews@lists.linux.dev To: "Tim Michals" Cc: conor+dt@kernel.org, robh@kernel.org, linux-sunxi@lists.linux.dev, devicetree@vger.kernel.org In-Reply-To: <20260927002021.797069-3-tcmichals@gmail.com> References: <20260922034711.190253-1-tcmichals@gmail.com> <20260927002021.797069-1-tcmichals@gmail.com> <20260927002021.797069-3-tcmichals@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Sun, 27 Sep 2026 00:33:02 +0000 Message-Id: <20260927003302.D69591F000FF@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 4 potential issue(= s) to consider: - [High] mailbox: sun55i: out-of-bounds array write in probe due to unbound= ed DT irq_cnt - [High] mailbox: sun55i: incorrect last_tx_done polling condition breaks T= X semantics - [High] mailbox: sun55i: NULL pointer dereference in IRQ handler during te= ardown - [High] mailbox: sun55i: lockless multi-IRQ concurrency causes FIFO underf= low -- commit d294abbd2180fd12203dc082ff7e6d574533c041 Author: Tim Michals mailbox: sun55i: add Allwinner sun55i/sun60i 4-port Message Box driver This commit adds a mailbox driver for the 4-port hardware Message Box controller found on Allwinner sun55i and sun60i SoCs. It implements hardware FIFO communication across 12 logical channels for multiple heterogeneous co= res. > diff --git a/drivers/mailbox/sun55i-msgbox.c b/drivers/mailbox/sun55i-msg= box.c > new file mode 100644 > index 0000000000000..928e52e08bad4 > --- /dev/null > +++ b/drivers/mailbox/sun55i-msgbox.c > @@ -0,0 +1,375 @@ [ ... ] > +irqreturn_t sun55i_msgbox_irq(int irq, void *dev_id) > +{ > + struct sun55i_msgbox *mbox =3D dev_id; > + irqreturn_t ret =3D IRQ_NONE; > + int i, local_n, p, chan_idx; > + > + for (local_n =3D 0; local_n < SUN55I_NUM_ROUTES; local_n++) { > + void __iomem *local_base =3D mbox->regs[0]; [Severity: High] Does this lockless loop allow multiple concurrent handlers on different CPU= s to inspect and process the same hardware channels simultaneously? Since sun55i_msgbox_probe() registers the exact same handler for multiple I= RQs using request_irq(), if two remote processors send messages concurrently, b= oth IRQs fire, executing the handler on different CPUs simultaneously. Both CPUs would see the same pending bit and race to drain the exact same channel, leading to a TOCTOU race where the second CPU reads from an empty = FIFO, resulting in a hardware underflow and delivery of a corrupt message. > + u32 en, stat, pending; > + > + en =3D readl(local_base + SUNXI_MSGBOX_READ_IRQ_ENABLE(local_n)); > + stat =3D readl(local_base + SUNXI_MSGBOX_READ_IRQ_STATUS(local_n)); > + pending =3D en & stat; [ ... ] > +static bool sun55i_msgbox_last_tx_done(struct mbox_chan *chan) > +{ > + struct sun55i_msgbox *mbox =3D to_sun55i_msgbox(chan); > + int n =3D chan - mbox->controller.chans; > + int local_n, p, remote_id, remote_n; > + u32 count; > + > + sun55i_chan_to_route(n, &local_n, &p, &remote_id, &remote_n); > + > + count =3D readl(mbox->regs[remote_id] + SUNXI_MSGBOX_MSG_STATUS(remote_= n, p)) & MSG_NUM_MASK; > + return count < SUN55I_FIFO_MAX; > +} [Severity: High] Does returning true when the FIFO is not full falsely signal transmission completion to the polling framework? The mailbox framework polls last_tx_done() to know when a message has been consumed by the remote processor. Returning true while the message is still= in the FIFO (count < SUN55I_FIFO_MAX) falsely signals completion. The framework will then prematurely send subsequent messages, leading to a hardware FIFO overflow and dropped IPC messages. [ ... ] > +static int sun55i_msgbox_probe(struct platform_device *pdev) > +{ [ ... ] > + irq_cnt =3D platform_irq_count(pdev); > + if (irq_cnt < 0) { > + ret =3D irq_cnt; > + goto err_assert_reset; > + } > + > + for (i =3D 0; i < irq_cnt; i++) { > + int irq =3D platform_get_irq(pdev, i); > + > + if (irq < 0) { > + ret =3D irq; > + goto err_free_irqs; > + } > + > + ret =3D request_irq(irq, sun55i_msgbox_irq, > + IRQF_SHARED, dev_name(dev), mbox); > + if (ret) { > + dev_err(dev, "failed to request irq %d: %d\n", irq, ret); > + goto err_free_irqs; > + } > + mbox->irqs[i] =3D irq; > + mbox->num_irqs =3D i + 1; > + } [Severity: High] Can this write overflow the mbox->irqs array if irq_cnt exceeds 4? The driver retrieves irq_cnt directly from the device tree via platform_irq_count() without bounds checking. If a device tree provides 5 or more interrupts, the unconditional loop will write out of bounds of the fixed-size 4-element mbox->irqs array, corrupting memory of adjacent fields= in the sun55i_msgbox structure. [ ... ] > +static void sun55i_msgbox_remove(struct platform_device *pdev) > +{ > + struct sun55i_msgbox *mbox =3D platform_get_drvdata(pdev); > + int local_n, i; > + > + mbox_controller_unregister(&mbox->controller); > + > + /* Mask hardware interrupts and free IRQs before asserting reset and di= sabling clock */ [Severity: High] Does unregistering the controller here before masking hardware interrupts a= nd freeing IRQs leave a race window open for a NULL pointer dereference? During device removal, mbox_controller_unregister() calls mbox_free_channel= (), which invokes sun55i_msgbox_shutdown() and clears the chan->cl pointer. While sun55i_msgbox_shutdown() masks the interrupt via MMIO, it fails to synchronize the kernel IRQ subsystem via synchronize_irq(). If an interrupt handler is already running concurrently on another CPU, it = will proceed to read the FIFO and invoke mbox_chan_received_data() from sun55i_msgbox_irq(), which dereferences the now-NULL chan->cl->rx_callback pointer. > + for (local_n =3D 0; local_n < SUN55I_NUM_ROUTES; local_n++) > + writel(0, mbox->regs[0] + SUNXI_MSGBOX_READ_IRQ_ENABLE(local_n)); > + > + for (i =3D 0; i < mbox->num_irqs; i++) > + free_irq(mbox->irqs[i], mbox); > + > + reset_control_assert(mbox->reset); > + clk_disable_unprepare(mbox->clk); > +} --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260927002021.7970= 69-1-tcmichals@gmail.com?part=3D2