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

[Jassi Brar]

On Wed, Feb 1, 2023 at 10:25 PM Hang Zhang <zh.nvgt@xxxxxxxxx> wrote:
>
> On Tue, Dec 27, 2022 at 10:46 PM Hang Zhang <zh.nvgt@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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.