MIME-Version: 1.0
References: <20221228034617.58386-1-zh.nvgt@gmail.com>
In-Reply-To: <20221228034617.58386-1-zh.nvgt@gmail.com>
From:   Hang Zhang <zh.nvgt@gmail.com>
Date:   Wed, 1 Feb 2023 23:25:23 -0500
Message-ID: <CAO2zrtZjDj5U4CCwVWQcWfbXJa0hwRjjNRE7jtHshrEb-nEwEg@mail.gmail.com>
Subject: Re: [PATCH] mailbox: mailbox-test: fix potential use-after-free issues
Cc:     Jassi Brar <jassisinghbrar@gmail.com>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
To:     unlisted-recipients:; (no To-header on input)
Precedence: bulk

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