Re: [PATCH] mailbox: mailbox-test: fix potential use-after-free issues

[Hang Zhang <zh.nvgt@gmail.com>]

On Thu, Feb 2, 2023 at 12:17 AM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Wed, Feb 1, 2023 at 10:25 PM Hang Zhang <zh.nvgt@gmail.com> wrote:
> >
> > On Tue, Dec 27, 2022 at 10:46 PM Hang Zhang <zh.nvgt@gmail.com> wrote:
> > >
> > > mbox_test_message_write() is the .write handler of the message
> > > debugfs interface, it operates on global pointers "tdev->signal"
> > > and "tdev->message" (e.g., allocation, dereference, free and
> > > nullification). However, these operations are not protected by any
> > > locks, making use-after-free possible in the concurrent setting.
> > > E.g., one invocation of the handler may have freed "tdev->signal"
> > > but being preempted before nullifying the pointer, then another
> > > invocation of the handler may dereference the now dangling pointer,
> > > causing use-after-free. Similarly, "tdev->message", as a shared
> > > pointer, may be manipulated by multiple invocations concurrently,
> > > resulting in unexpected issues such as use-after-free.
> > >
> > > Fix these potential issues by protecting the above operations with
> > > the spinlock "tdev->lock", which has already been deployed in other
> > > handlers of the debugfs interface (e.g., .read). This patch introduces
> > > the same lock to the .write handler.
> > >
> > > Signed-off-by: Hang Zhang <zh.nvgt@gmail.com>
> > > ---
> > >  drivers/mailbox/mailbox-test.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
> > > index 4555d678fadd..b2315261644a 100644
> > > --- a/drivers/mailbox/mailbox-test.c
> > > +++ b/drivers/mailbox/mailbox-test.c
> > > @@ -97,6 +97,7 @@ static ssize_t mbox_test_message_write(struct file *filp,
> > >         struct mbox_test_device *tdev = filp->private_data;
> > >         void *data;
> > >         int ret;
> > > +       unsigned long flags;
> > >
> > >         if (!tdev->tx_channel) {
> > >                 dev_err(tdev->dev, "Channel cannot do Tx\n");
> > > @@ -110,9 +111,12 @@ static ssize_t mbox_test_message_write(struct file *filp,
> > >                 return -EINVAL;
> > >         }
> > >
> > > +       spin_lock_irqsave(&tdev->lock, flags);
> > >         tdev->message = kzalloc(MBOX_MAX_MSG_LEN, GFP_KERNEL);
> >
> This is bad.  atomic context should not do things like alloc.
> Also, please look up MAINTAINERS and cc authors.
>
> thanks.

Hi Jassi, thank you for your quick reply! We now cc'ed the major authors
of the file identified from the commit history. While this patch is bad as
you pointed out (sorry for that), do you think the concurrency issue here
is valid and worth a fix? If so, we can seek to improve the patch with the
help of the developers. Thank you very much!

Best,
Hang