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

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

[Hang Zhang]

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);
-	if (!tdev->message)
-		return -ENOMEM;
+	if (!tdev->message) {
+		ret = -ENOMEM;
+		goto out_1;
+	}
 
 	ret = copy_from_user(tdev->message, userbuf, count);
 	if (ret) {
@@ -143,6 +147,8 @@ static ssize_t mbox_test_message_write(struct file *filp,
 	kfree(tdev->signal);
 	kfree(tdev->message);
 	tdev->signal = NULL;
+out_1:
+	spin_unlock_irqrestore(&tdev->lock, flags);
 
 	return ret < 0 ? ret : count;
 }
-- 
2.39.0

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

[Hang Zhang]

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);
> -       if (!tdev->message)
> -               return -ENOMEM;
> +       if (!tdev->message) {
> +               ret = -ENOMEM;
> +               goto out_1;
> +       }
>
>         ret = copy_from_user(tdev->message, userbuf, count);
>         if (ret) {
> @@ -143,6 +147,8 @@ static ssize_t mbox_test_message_write(struct file *filp,
>         kfree(tdev->signal);
>         kfree(tdev->message);
>         tdev->signal = NULL;
> +out_1:
> +       spin_unlock_irqrestore(&tdev->lock, flags);
>
>         return ret < 0 ? ret : count;
>  }
> --
> 2.39.0
>

Hi Jassi, could we get some comments from you about this patch?
To avoid potential confusion, we want to also clarify that this UAF
issue was originally flagged by our static analyzer, so we do not have
PoC or dynamic execution traces here, however, we have performed
a careful manual code review to confirm this issue and we think that
it seems apparent, as currently there is no concurrency protection
for the .write handler, while it should have some (e.g., the .read
handler does have some lock protection). Please feel free to let us
know if we missed anything here, thank you very much for your help!

Best,
Hang

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@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.

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

[Hang Zhang]

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