Re: [PATCH v3 2/7] mailbox: sun55i: Add Allwinner A523 message box driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mailbox: sun55i: TX IRQ race in sun55i_msgbox_shutdown() on shared route
- [High] mailbox: sun55i: UAF and race returning -EBUSY in sun55i_msgbox_send_data()
- [Medium] mailbox: sun55i: lock drop race enabling TX IRQ in sun55i_msgbox_send_data()
--

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

mailbox: sun55i: Add Allwinner A523 message box driver

This commit adds a mailbox controller driver for the Allwinner sun55i (A523)
message box, supporting 12 hardware FIFO channels between the Cortex-A55 host
and the CPUS, HiFi4 DSP, and XuanTie E906 processors with either polled or
write-IRQ TX completion.

> diff --git a/drivers/mailbox/sun55i-msgbox.c b/drivers/mailbox/sun55i-msgbox.c
> new file mode 100644
> index 0000000000000..1618c16219fc2
> --- /dev/null
> +++ b/drivers/mailbox/sun55i-msgbox.c
> @@ -0,0 +1,613 @@

[ ... ]

> +static void sun55i_msgbox_tx_irq_put(struct sun55i_msgbox *mbox, int local_n)
> +{
> +	struct sun55i_tx_irq *tx = &mbox->tx_irqs[local_n];
> +
> +	mutex_lock(&mbox->tx_irq_lock);
> +	if (!--tx->users) {
> +		free_irq(tx->irq, tx);
> +	} else {
> +		/* The line stays in use: let a running handler finish with us */
> +		synchronize_irq(tx->irq);
> +	}
> +	mutex_unlock(&mbox->tx_irq_lock);
> +}
> +
> +static int sun55i_msgbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct sun55i_msgbox *mbox = to_sun55i_msgbox(chan);
> +	int n = chan - mbox->controller.chans;
> +	int local_n, p, remote_id, remote_n;
> +	unsigned long flags;
> +	u32 msg = 0;
> +	bool full;
> +
> +	if (data)
> +		memcpy(&msg, data, sizeof(msg));
> +
> +	sun55i_msgbox_chan_to_route(n, &local_n, &p, &remote_id, &remote_n);
> +
> +	/*
> +	 * Under the lock: in timer mode the poll timer may write a held
> +	 * message into the same FIFO from another CPU (last_tx_done()).
> +	 */
> +	spin_lock_irqsave(&mbox->lock, flags);
> +
> +	/*
> +	 * The core calls send_data() only with no request active, so a message
> +	 * still held here is one it gave up on (a blocking send timed out):
> +	 * drop it rather than send it after this newer one.
> +	 */
> +	__clear_bit(n, &mbox->tx_held);
> +
> +	/*
> +	 * Writing a full FIFO corrupts its count, so never do it. In write IRQ
> +	 * mode the IRQ armed below retries the message once a slot is free. In
> +	 * timer mode the core polls only a channel whose send succeeded, so
> +	 * hold the message and let last_tx_done() write it.
> +	 */
> +	full = sun55i_msgbox_tx_count(mbox, remote_id, remote_n, p) >= SUN55I_FIFO_MAX;
> +	if (!full) {
> +		sun55i_msgbox_writel(msg, mbox->regs[remote_id] +
> +				     SUN55I_MSGBOX_MSG_FIFO(remote_n, p));
> +	} else if (!mbox->tx_done_irq) {
> +		mbox->tx_held_msg[n] = msg;
> +		__set_bit(n, &mbox->tx_held);
> +	}
> +
> +	spin_unlock_irqrestore(&mbox->lock, flags);

[Severity: Medium]
Can dropping mbox->lock here before calling sun55i_msgbox_tx_irq_enable()
cause a premature TX completion when a previous blocking send timed out?

If an earlier blocking send timed out in mbox_send_message() ->
tx_tick(chan, -ETIME), SUN55I_MSGBOX_WR_IRQ_EN_BIT(p) stays enabled in the
remote bank. When the remote processor later reads a message and frees a slot,
SUN55I_MSGBOX_WR_IRQ_PEND_BIT(p) is latched in
SUN55I_MSGBOX_WRITE_IRQ_STATUS(remote_n).

When the next sun55i_msgbox_send_data() writes to SUN55I_MSGBOX_MSG_FIFO and
fills the FIFO to SUN55I_FIFO_MAX, releasing mbox->lock before clearing the
latched status bit in sun55i_msgbox_tx_irq_enable() opens a window where
sun55i_msgbox_tx_irq() on another CPU can acquire mbox->lock, see both
WR_IRQ_EN_BIT(p) and the stale latched WR_IRQ_PEND_BIT(p) set, and call
mbox_chan_txdone() to complete the new message while the FIFO is still full.

Should the write IRQ status clear and enable in sun55i_msgbox_tx_irq_enable()
be performed under the same mbox->lock critical section as the FIFO write?

> +
> +	/*
> +	 * A kick is a doorbell, the data is in shared memory: it is done once
> +	 * the FIFO accepted it and has room for the next one. The write IRQ
> +	 * reports exactly that, at once or when the receiver reads a message.
> +	 */
> +	if (mbox->tx_done_irq) {
> +		sun55i_msgbox_tx_irq_enable(mbox, remote_id, remote_n, p, true);
> +		return full ? -EBUSY : 0;
> +	}

[Severity: High]
What happens in write-IRQ mode (mbox->tx_done_irq) when the hardware FIFO is
full and sun55i_msgbox_send_data() arms the write IRQ and returns -EBUSY?

In drivers/mailbox/mailbox.c, msg_submit() only sets chan->active_req = data
and decrements chan->msg_count when send_data() returns 0:

drivers/mailbox/mailbox.c:msg_submit() {
	...
	if (chan->cl->tx_prepare)
		chan->cl->tx_prepare(chan->cl, data);
	/* Try to submit a message to the MBOX controller */
	err = chan->mbox->ops->send_data(chan, data);
	if (!err) {
		chan->active_req = data;
		chan->msg_count--;
	}
	...
}

When send_data() returns -EBUSY, chan->active_req remains NULL and the
caller's data pointer stays in chan->msg_data[]. If a blocking send in
mbox_send_message() then times out in wait_for_completion_timeout(), it calls
tx_tick(chan, -ETIME):

drivers/mailbox/mailbox.c:tx_tick() {
	...
	spin_lock_irqsave(&chan->lock, flags);
	mssg = chan->active_req;
	chan->active_req = NULL;
	spin_unlock_irqrestore(&chan->lock, flags);

	/* Submit next message */
	msg_submit(chan);

	if (!mssg)
		return;
	...
}

Because chan->active_req is NULL, tx_tick() does not remove the timed-out
message from chan->msg_data[]. Instead, mbox_send_message() returns -ETIME to
the caller while its message pointer (often stack-allocated in blocking mode)
is still queued in chan->msg_data[] with the write IRQ enabled. When the
remote processor later drains a FIFO slot, sun55i_msgbox_tx_irq() ->
mbox_chan_txdone() -> tx_tick() -> msg_submit() will call cl->tx_prepare(),
sun55i_msgbox_send_data() (which runs memcpy(&msg, data, sizeof(msg))), and
cl->tx_done() on the dangling pointer.

Even without a timeout, while chan->active_req is NULL and the first message
waits in chan->msg_data[] for the write IRQ, a second mbox_send_message() call
on another CPU can run msg_submit() just as a FIFO slot frees: CPU 0 sees
chan->active_req == NULL, calls cl->tx_prepare() a second time on the first
message, writes it into the single free FIFO slot (refilling the FIFO), and
sets chan->active_req. The concurrent sun55i_msgbox_tx_irq() on CPU 1 then
calls mbox_chan_txdone() and prematurely completes the message while the FIFO
is full.

Should write-IRQ mode also buffer the message in mbox->tx_held_msg[n] and
return 0 when the FIFO is full, writing the held message from
sun55i_msgbox_tx_irq() once a slot is free?

[ ... ]

> +static void sun55i_msgbox_shutdown(struct mbox_chan *chan)
> +{
> +	struct sun55i_msgbox *mbox = to_sun55i_msgbox(chan);
> +	void __iomem *base = mbox->regs[SUN55I_PROC_ARM];
> +	int n = chan - mbox->controller.chans;
> +	int local_n, p, remote_id, remote_n;
> +	u32 stale[SUN55I_FIFO_MAX];
> +	unsigned long flags;
> +	u32 val;
> +
> +	sun55i_msgbox_chan_to_route(n, &local_n, &p, &remote_id, &remote_n);
> +
> +	/* Disable the receive IRQ; a message held for a full FIFO is dropped */
> +	spin_lock_irqsave(&mbox->lock, flags);
> +	val = sun55i_msgbox_readl(base + SUN55I_MSGBOX_READ_IRQ_ENABLE(local_n));
> +	val &= ~SUN55I_MSGBOX_RD_IRQ_EN_BIT(p);
> +	sun55i_msgbox_writel(val, base + SUN55I_MSGBOX_READ_IRQ_ENABLE(local_n));
> +	__clear_bit(n, &mbox->tx_held);
> +	spin_unlock_irqrestore(&mbox->lock, flags);
> +
> +	/* The handler delivers outside the lock: let it finish with this channel */
> +	synchronize_irq(mbox->irq);
> +
> +	/* Release the line only if this channel's startup() got it */
> +	if (test_and_clear_bit(n, &mbox->tx_irq_held)) {
> +		sun55i_msgbox_tx_irq_enable(mbox, remote_id, remote_n, p, false);
> +		sun55i_msgbox_tx_irq_put(mbox, local_n);
> +		/*
> +		 * A TX-done handler that was already running submits the next
> +		 * message through send_data(), which arms the IRQ again. It has
> +		 * finished now (free_irq() or synchronize_irq() above), so
> +		 * disarm once more.
> +		 */
> +		sun55i_msgbox_tx_irq_enable(mbox, remote_id, remote_n, p, false);
> +	}

[Severity: High]
Can a second sun55i_msgbox_tx_irq() invocation still race with channel
teardown here when another channel on the same route remains active
(tx->users > 1)?

In mbox_free_channel(), chan->mbox->ops->shutdown(chan) runs before chan->cl
is cleared, and queued messages may still be present in chan->msg_data[]. When
tx->users > 1 on local_n, sun55i_msgbox_tx_irq_put() only calls
synchronize_irq(tx->irq) and leaves tx->irq enabled at the interrupt
controller.

If an in-flight sun55i_msgbox_tx_irq() was already past mbox->lock on another
CPU, it calls:

sun55i_msgbox_tx_irq()
  mbox_chan_txdone()
    tx_tick()
      msg_submit()
        sun55i_msgbox_send_data()
          sun55i_msgbox_tx_irq_enable(mbox, remote_id, remote_n, p, true)

Because the write IRQ is level-triggered whenever at least one FIFO slot is
free, re-enabling SUN55I_MSGBOX_WR_IRQ_EN_BIT(p) in sun55i_msgbox_send_data()
can immediately assert tx->irq again as soon as the first handler returns.
Waiters in sun55i_msgbox_tx_irq_put()'s synchronize_irq(tx->irq) only wait for
that first handler invocation to complete, so a second sun55i_msgbox_tx_irq()
can fire before the second sun55i_msgbox_tx_irq_enable(..., false) runs and
execute mbox_chan_txdone() -> tx_tick() concurrently with mbox_free_channel()
setting chan->cl = NULL (or re-arm the write IRQ again if more messages were
queued).

Would checking test_bit(n, &mbox->tx_irq_held) under mbox->lock before arming
the write IRQ in sun55i_msgbox_send_data() avoid re-enabling the IRQ after
sun55i_msgbox_shutdown() has begun?

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