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]
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ