Re: [PATCH v2 2/7] mailbox: sun55i: add Allwinner sun55i/sun60i 4-port Message Box driver

[sashiko-bot@kernel.org]

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 unbounded DT irq_cnt
- [High] mailbox: sun55i: incorrect last_tx_done polling condition breaks TX semantics
- [High] mailbox: sun55i: NULL pointer dereference in IRQ handler during teardown
- [High] mailbox: sun55i: lockless multi-IRQ concurrency causes FIFO underflow
--

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

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 cores.

> diff --git a/drivers/mailbox/sun55i-msgbox.c b/drivers/mailbox/sun55i-msgbox.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 = dev_id;
> +	irqreturn_t ret = IRQ_NONE;
> +	int i, local_n, p, chan_idx;
> +
> +	for (local_n = 0; local_n < SUN55I_NUM_ROUTES; local_n++) {
> +		void __iomem *local_base = mbox->regs[0];

[Severity: High]
Does this lockless loop allow multiple concurrent handlers on different CPUs to
inspect and process the same hardware channels simultaneously?

Since sun55i_msgbox_probe() registers the exact same handler for multiple IRQs
using request_irq(), if two remote processors send messages concurrently, both
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 = readl(local_base + SUNXI_MSGBOX_READ_IRQ_ENABLE(local_n));
> +		stat = readl(local_base + SUNXI_MSGBOX_READ_IRQ_STATUS(local_n));
> +		pending = en & stat;
[ ... ]
> +static bool sun55i_msgbox_last_tx_done(struct mbox_chan *chan)
> +{
> +	struct sun55i_msgbox *mbox = to_sun55i_msgbox(chan);
> +	int n = 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 = 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 = platform_irq_count(pdev);
> +	if (irq_cnt < 0) {
> +		ret = irq_cnt;
> +		goto err_assert_reset;
> +	}
> +
> +	for (i = 0; i < irq_cnt; i++) {
> +		int irq = platform_get_irq(pdev, i);
> +
> +		if (irq < 0) {
> +			ret = irq;
> +			goto err_free_irqs;
> +		}
> +
> +		ret = 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] = irq;
> +		mbox->num_irqs = 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 = platform_get_drvdata(pdev);
> +	int local_n, i;
> +
> +	mbox_controller_unregister(&mbox->controller);
> +
> +	/* Mask hardware interrupts and free IRQs before asserting reset and disabling clock */

[Severity: High]
Does unregistering the controller here before masking hardware interrupts and
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 = 0; local_n < SUN55I_NUM_ROUTES; local_n++)
> +		writel(0, mbox->regs[0] + SUNXI_MSGBOX_READ_IRQ_ENABLE(local_n));
> +
> +	for (i = 0; i < mbox->num_irqs; i++)
> +		free_irq(mbox->irqs[i], mbox);
> +
> +	reset_control_assert(mbox->reset);
> +	clk_disable_unprepare(mbox->clk);
> +}

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