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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200601142947.GG19480@localhost>
Date:   Mon, 1 Jun 2020 16:29:47 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Johan Hovold <johan@...nel.org>,
        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 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

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

but foo remains registered until the parent driver is unbound.

Johan