linux-kernel - Re: [PATCH] gpiolib: cdev: fix NULL-pointer dereferences

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=MdHtJC4Tmn3KgcnefmHTrpXy=ROAAXJLN9uv=ouJ-hQSw@mail.gmail.com>
Date:   Fri, 25 Nov 2022 22:03:06 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Kent Gibson <warthog618@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH] gpiolib: cdev: fix NULL-pointer dereferences

On Fri, Nov 25, 2022 at 6:56 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Fri, Nov 25, 2022 at 05:48:02PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Nov 25, 2022 at 5:24 PM Kent Gibson <warthog618@...il.com> wrote:
>
> ...
>
> > Then at the subsystem level, the GPIO device struct would need a lock
> > that would be taken by every user-space operation AND the code
> > unregistering the device so that we don't do what you described (i.e.
> > if there's a thread doing a read(), then let's wait until it returns
> > before we drop the device).
>
> It's called a reference counting, basically you need to get device and then
> put when it makes sense.
>

Andy: I am aware of struct device reference counting but this isn't
it. You can count references all you want, but when I disconnect my
CP2112, the USB bus calls gpiochip_remove(), struct gpio_chip * inside
struct gpio_device is set to NULL and while the underlying struct
device itself is still alive, the GPIO chip is no longer usable.

Reference counting won't help because the device is no longer there,
so this behavior is correct but there's an issue with user-space still
being able to hold certain resources and we need to make sure that
when it tries to use them, we return an error instead of crashing.

I think that a good solution is to make sure, we cannot set gdev->gc
to NULL as long as there are user-space operations in progress. After
all, it's better to try to send a USB request to an unplugged device
than to dereference a NULL pointer. To that end, we could have a
user-space lock that would also be taken by gpiochip_remove().

But this is still a per-subsystem solution. Most other subsystems
suffer from the same issue.

Bartosz