linux-kernel - Re: [PATCH v2] gpiolib: cdev: Fix use after free in lineinfo_changed

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMRc=MdmOg6pJ7hvKSpkoTKjQny8xL5BFT2HNzgKgnjsCuwhwQ@mail.gmail.com>
Date: Tue, 7 May 2024 10:00:25 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Zhongqiu Han <quic_zhonhan@...cinc.com>
Cc: warthog618@...il.com, linus.walleij@...aro.org, linux-gpio@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] gpiolib: cdev: Fix use after free in lineinfo_changed_notify

On Sun, May 5, 2024 at 4:12 PM Zhongqiu Han <quic_zhonhan@...cinc.com> wrote:
>
> The use-after-free issue occurs as follows: when the GPIO chip device file
> is being closed by invoking gpio_chrdev_release(), watched_lines is freed
> by bitmap_free(), but the unregistration of lineinfo_changed_nb notifier
> chain failed due to waiting write rwsem. Additionally, one of the GPIO
> chip's lines is also in the release process and holds the notifier chain's
> read rwsem. Consequently, a race condition leads to the use-after-free of
> watched_lines.
>
> Here is the typical stack when issue happened:
>
> [free]
> gpio_chrdev_release()
>   --> bitmap_free(cdev->watched_lines)                  <-- freed
>   --> blocking_notifier_chain_unregister()
>     --> down_write(&nh->rwsem)                          <-- waiting rwsem
>           --> __down_write_common()
>             --> rwsem_down_write_slowpath()
>                   --> schedule_preempt_disabled()
>                     --> schedule()
>

The rwsem has been removed in v6.9-rc1. I assume you're targeting
stable branches with this change? Or does it still occur with the
recent SRCU rework? This is important to know before I send it
upstream.

Bart

> [use]
> st54spi_gpio_dev_release()
>   --> gpio_free()
>     --> gpiod_free()
>       --> gpiod_free_commit()
>         --> gpiod_line_state_notify()
>           --> blocking_notifier_call_chain()
>             --> down_read(&nh->rwsem);                  <-- held rwsem
>             --> notifier_call_chain()
>               --> lineinfo_changed_notify()
>                 --> test_bit(xxxx, cdev->watched_lines) <-- use after free
>
> The side effect of the use-after-free issue is that a GPIO line event is
> being generated for userspace where it shouldn't. However, since the chrdev
> is being closed, userspace won't have the chance to read that event anyway.
>
> To fix the issue, call the bitmap_free() function after the unregistration
> of lineinfo_changed_nb notifier chain.
>
> Fixes: 51c1064e82e7 ("gpiolib: add new ioctl() for monitoring changes in line info")
> Signed-off-by: Zhongqiu Han <quic_zhonhan@...cinc.com>
> ---
> v1 -> v2:
> - Drop the excessive stack log from commit message to make it more readable.
> - Add a note regarding the side effects of the use-after-free on commit message.
> - Link to v1: https://lore.kernel.org/lkml/20240501022612.1787143-1-quic_zhonhan@quicinc.com/
>
>  drivers/gpio/gpiolib-cdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index d09c7d728365..6b3a43e3ba47 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2799,11 +2799,11 @@ static int gpio_chrdev_release(struct inode *inode, struct file *file)
>         struct gpio_chardev_data *cdev = file->private_data;
>         struct gpio_device *gdev = cdev->gdev;
>
> -       bitmap_free(cdev->watched_lines);
>         blocking_notifier_chain_unregister(&gdev->device_notifier,
>                                            &cdev->device_unregistered_nb);
>         blocking_notifier_chain_unregister(&gdev->line_state_notifier,
>                                            &cdev->lineinfo_changed_nb);
> +       bitmap_free(cdev->watched_lines);
>         gpio_device_put(gdev);
>         kfree(cdev);
>
> --
> 2.25.1
>