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:   Tue, 2 Jun 2020 11:32:55 +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 6:31 PM Johan Hovold <johan@...nel.org> wrote:
> On Mon, Jun 01, 2020 at 06:09:23PM +0300, Andy Shevchenko wrote:
> > 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:

...

> > > > > 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.
>
> > > 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?
>
> As I mentioned in a commit message, we have quite a few MFD drivers that
> register class devices under the parent device directly and have been
> doing so for a very long time.
>
> As this is reflected in sysfs and we may have users relying on the
> current topology, changing this shouldn't be taken too lightly (drivers
> may also depend on it). And in any case, it wouldn't be stable material
> to fix the regressions at hand.

I see.

> > >         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?
>
> No, that's a separate issue as it also changes the device tree.

Thanks for elaboration.

-- 
With Best Regards,
Andy Shevchenko