linux-kernel - Re: [PATCH v3] i2c: dev: don't allow user-space to deadlock the kernel

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=MdyLfai3svyfQ2=0M4HqhWQHa1tr-xCWBrk5rECQNizwg@mail.gmail.com>
Date:   Sat, 28 Jan 2023 21:16:59 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Wolfram Sang <wsa@...nel.org>, Bartosz Golaszewski <brgl@...ev.pl>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v3] i2c: dev: don't allow user-space to deadlock the kernel

On Sat, Jan 28, 2023 at 8:02 PM Wolfram Sang <wsa@...nel.org> wrote:
>
> Hi Bartosz,
>
> > > https://lkml.iu.edu/hypermail/linux/kernel/1501.2/01700.html
> > >
> > > There are still drivers using i2c_del_adapter()+kfree(), so removing the
> > > completion could cause use-after-free there, or?
> > >
> >
> > Ugh, what a mess... I was mostly focused on the character device side
> > of it but now I realized the true extent of the problem.
>
> Still, thanks for trying. Really!
>
> > It's all because the adapter struct really should be allocated by
> > i2c_add_adapter() and bus drivers should only really provide some
> > structure containing the adapter description for the subsystem the
> > lifetime of which would not affect the adapter itself. This way the
> > adapter (embedding struct device) would be freed by device type's
> > .release() like we do over in the GPIO subsystem. Instead the adapter
> > struct is allocated by drivers at .probe() meaning it will get dropped
> > at .remove().
>
> Or, like SPI does, use controller_alloc() which initializes the parts
> needed by the core, returns to the driver which needs to setup the
> private data, and finally calls controller_register().
>

If we could add a helper like struct *i2c_adapter_get_device(struct
i2c_adapter *adap) and convert all users who access adap.dev directly
to using it instead in a sweeping change across the subsystem - that
would already be a huge improvement as this would allow us to later
move struct device memory into the subsystem and free it in .release()
and not allow provider drivers to free it at .remove(). Something to
consider. Let me know if that's an interesting option.

> > I don't have a good solution. I've been thinking about it for an hour
> > and every solution requires sweeping changes across the entire
> > subsystem. Or else we'd introduce a parallel solution that would do
> > the right thing and wait in perpetuity until all drivers convert -
> > like with i2e probe_new() which is after all much simpler.
>
> Thank you for spending time on another solution. "Perpetuity" is a good
> word to put it :/
>
> > Anyway, that's all I've got. We probably need to drop this change and
> > live with what we have now.
>
> I am curious to see if this finding will make it into your FOSDEM talk.
> Looking already forward to it!
>

Ha! Yeah, definitely. I went down that rabbit hole a while ago and am
bothered by the amount of latent bugs I'm finding. I sent some patches
for GPIO and SPI (and this one for I2C). See you there, I guess?

> Happy hacking,
>
>    Wolfram
>

Bart