Re: [PATCH v2] gpiolib: cdev: Fix use after free in lineinfo_changed_notify

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sun, May 5, 2024 at 4:12 PM Zhongqiu Han <quic_zhonhan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx/
>
>  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
>