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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 2 Feb 2023 00:51:17 -0500
From:   Hang Zhang <zh.nvgt@...il.com>
To:     Jassi Brar <jassisinghbrar@...il.com>
Cc:     linux-kernel@...r.kernel.org, sudeep.holla@....com,
        lee.jones@...aro.org
Subject: Re: [PATCH] mailbox: mailbox-test: fix potential use-after-free issues

On Thu, Feb 2, 2023 at 12:17 AM Jassi Brar <jassisinghbrar@...il.com> wrote:
>
> On Wed, Feb 1, 2023 at 10:25 PM Hang Zhang <zh.nvgt@...il.com> wrote:
> >
> > On Tue, Dec 27, 2022 at 10:46 PM Hang Zhang <zh.nvgt@...il.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@...il.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