lkml.org 
[lkml]   [2024]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@quicinc.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@quicinc.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
>

\
 
 \ /
  Last update: 2024-05-27 18:17    [W:0.106 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site