Received-SPF: pass (google.com: domain of linux-kernel+bounces-170844-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240505141156.2944912-1-quic_zhonhan@quicinc.com>
In-Reply-To: <20240505141156.2944912-1-quic_zhonhan@quicinc.com>
From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Tue, 7 May 2024 10:00:25 +0200
Message-ID: <CAMRc=MdmOg6pJ7hvKSpkoTKjQny8xL5BFT2HNzgKgnjsCuwhwQ@mail.gmail.com>
Subject: Re: [PATCH v2] gpiolib: cdev: Fix use after free in lineinfo_changed_notify
To: Zhongqiu Han <quic_zhonhan@quicinc.com>
Cc: warthog618@gmail.com, linus.walleij@linaro.org, linux-gpio@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, May 5, 2024 at 4:12=E2=80=AFPM Zhongqiu Han <quic_zhonhan@quicinc.c=
om> wrote:
>
> The use-after-free issue occurs as follows: when the GPIO chip device fil=
e
> 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 fre=
e
>
> 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 chrd=
ev
> is being closed, userspace won't have the chance to read that event anywa=
y.
>
> To fix the issue, call the bitmap_free() function after the unregistratio=
n
> 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 readab=
le.
> - Add a note regarding the side effects of the use-after-free on commit m=
essage.
> - 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 *inod=
e, struct file *file)
>         struct gpio_chardev_data *cdev =3D file->private_data;
>         struct gpio_device *gdev =3D 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
>