Re: [PATCH] mailbox: fix a UAF bug in msg_submit()

From: Xianting TIan
Date: Tue Aug 17 2021 - 21:41:03 EST

在 2021/8/18 上午7:33, Jassi Brar 写道:
On Tue, Aug 17, 2021 at 3:01 AM Xianting TIan
<xianting.tian@xxxxxxxxxxxxxxxxx> wrote:

在 2021/8/17 下午1:58, Xianting TIan 写道:
在 2021/8/17 下午12:29, Jassi Brar 写道:
On Fri, Aug 6, 2021 at 7:15 AM Xianting Tian
<xianting.tian@xxxxxxxxxxxxxxxxx> wrote:
We met a UAF issue during our mailbox testing.

In synchronous mailbox, we use mbox_send_message() to send a message
and wait for completion. mbox_send_message() calls msg_submit() to
send the message for the first time, if timeout, it will send the
message in tx_tick() for the second time.

Seems like your controller's .send_data() returns error. Can you
please explain why it does so? Because
send_data() only _accepts_ data for further transmission... which
should seldom be a problem.
Thanks for the comments,

We developed virtio-mailbox for heterogeneous virtualization system.

virtio-mailbox is based on the mialbox framework.

In virtio framework, its send func 'virtqueue_add_outbuf()' may return
error, which caused .send_data() return error. And also contains
other csenarios.

But I think mailbox framework shouldn't depend on .send_data() always
return OK, as .send_data() is implemented by mailbox hardware
manufacturer, which is not controlled by mailbox framework itself.

As I said, send_data() is basically "accepted for transfer", and not
"transferred fine".

You said 'seldom', but it still possible we can meet such issue. sucn
as flexrm_send_data() of drivers/mailbox/bcm-flexrm-mailbox.c.

The api is used not just in synchronous mode.
And the flexrm driver relies on ACK method, not the synchronous one.

I think mailbox framework should be work normaly no matter
.send_data() returns ok or not ok. Do you think so? thanks
Normal is your controller driver should be ready after .startup() and
accepts the first message submitted to it.
If it does that, everything would work.

Another solution is to ignore the return value of .send_data() in


err = chan->mbox->ops->send_data(chan, data);
if (!err) {
chan->active_req = data;


chan->mbox->ops->send_data(chan, data);
chan->active_req = data;

Yes, I also have something like this in mind. Definitely not the hack
in your original post.

But the side effect of the solution is obvious, as if send failed in the
first time, it will have no chance to sent it in tx_tick() for the
second time. That is to say, no retry.

The api doesn't retry. The .last_tx_done() is supposed to tell the
api when is it ok to send a message.

The following should work for you (though I believe your code needs
fixing), which anyway, should have been there.

thanks for the comment, so you apply below solution to kernel code?

I will use it as our solution.

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 3e7d4b20ab34..9824a51b82fa 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -75,10 +75,12 @@ static void msg_submit(struct mbox_chan *chan)
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) {
+ if (!err)
chan->active_req = data;
- chan->msg_count--;
- }
+ /* done with another message */
+ chan->msg_count--;
spin_unlock_irqrestore(&chan->lock, flags);