linux-kernel - Re: [PATCH 0/6] leds: fix broken devres usage

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:   Mon, 1 Jun 2020 18:09:23 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Johan Hovold <johan@...nel.org>
Cc:     Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Pavel Machek <pavel@....cz>, Dan Murphy <dmurphy@...com>,
        Amitoj Kaur Chawla <amitoj1606@...il.com>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/6] leds: fix broken devres usage

On Mon, Jun 1, 2020 at 5:29 PM Johan Hovold <johan@...nel.org> wrote:
> On Mon, Jun 01, 2020 at 05:08:40PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 1, 2020 at 5:01 PM Johan Hovold <johan@...nel.org> wrote:
> > > On Mon, Jun 01, 2020 at 04:51:01PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Jun 1, 2020 at 4:42 PM Johan Hovold <johan@...nel.org> wrote:
> > > > >
> > > > > Several MFD child drivers register their class devices directly under
> > > > > the parent device (about half of the MFD LED drivers do so).
> > > > >
> > > > > This means you cannot blindly do devres conversions so that
> > > > > deregistration ends up being tied to the parent device, something which
> > > > > leads to use-after-free on driver unbind when the class device is
> > > > > released while still being registered (and, for example, oopses on later
> > > > > parent MFD driver unbind or LED class callbacks, or resource leaks and
> > > > > name clashes on child driver reload).
> > > >
> > > > Shouldn't MFD take reference count for their children?
> > >
> > > That's not the issue here. The child driver is allocating memory for the
> > > class device (for example using devres), and that will end up being
> > > freed on unbind while said device is still registered. The child driver
> > > may then even be unloaded. No extra reference can fix this.
> >
> > Okay, I didn't still get how dropping devres will help here.
> >
> > Say, we have
> >
> > ->probe()
> > {
> >  return devm_foo_register();
> > }
> >
> > and no ->remove()
> >
> > vs.
> >
> > ->probe()
> > {
> >   return foo_register();
> > }
> >
> > ->remove()
> > {
> >  foo_unregister();
> > }
> >
> > So, basically what you seem to workaround is that ->remove() is not
> > getting called?
>
> Any driver which frees a resource before making sure it's no longer used
> it is just plain broken. Unfortunately, devres makes this harder to
> reason about and people get it wrong. This is roughly the current
> situation with these drivers:
>
>         drv->probe(dev)
>           foo = devm_kzalloc(dev);

>           devm_foo_register(dev->parent, foo);  // NOTE: dev->parent

A-ha! Thanks for this detail.
But why are they doing so?

>         drv->remove(dev)
>         devres_release_all(dev)
>           kfree(foo);                           // foo still registered

> but foo remains registered until the parent driver is unbound.

Since the last fixes against kobject elimination, shouldn't be this
simple fixed by not supplying dev->parent above?

-- 
With Best Regards,
Andy Shevchenko