linux-kernel - Re: [PATCH v2 3/3] drivers: base: Free devm resources when unregistering a device

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] [day] [month] [year] [list]

Message-ID: <ktpru2pormh4fgkwxjpidk3vrlg3qh47tmye5l4vk6slutd25p@fwtkiizh3fa4>
Date:   Thu, 20 Jul 2023 13:57:28 +0200
From:   Maxime Ripard <mripard@...nel.org>
To:     David Gow <davidgow@...gle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] drivers: base: Free devm resources when
 unregistering a device

On Wed, Jul 19, 2023 at 05:13:58PM +0800, David Gow wrote:
> On Wed, 28 Jun 2023 at 17:50, Maxime Ripard <mripard@...nel.org> wrote:
> >
> > From: David Gow <davidgow@...gle.com>
> >
> > In the current code, devres_release_all() only gets called if the device
> > has a bus and has been probed.
> >
> > This leads to issues when using bus-less or driver-less devices where
> > the device might never get freed if a managed resource holds a reference
> > to the device. This is happening in the DRM framework for example.
> >
> > We should thus call devres_release_all() in the device_del() function to
> > make sure that the device-managed actions are properly executed when the
> > device is unregistered, even if it has neither a bus nor a driver.
> >
> > This is effectively the same change than commit 2f8d16a996da ("devres:
> > release resources on device_del()") that got reverted by commit
> > a525a3ddeaca ("driver core: free devres in device_release") over
> > use-after-free concerns.
> >
> > It's not clear whether those concerns are legitimate though, but I would
> > expect drivers not to register new resources in their device-managed
> > actions.
> 
> It might be clearer to notice that this patch effectively combines the
> two patches above, freeing _both_ on device_del() and
> device_release(). This should give us the best of both worlds.

You're right I'll add that part to the commit log.

> I'm not aware of a use-after-free issue that could result here, though
> it's possible there's a double free I'm missing now that we are
> freeing things twice. My understanding is that commit a525a3ddeaca
> ("driver core: free devres in device_release") was more to avoid a
> leak than a use-after-free, but I could be wrong.

Yeah, I'm not sure where I got the UAF from. I probably
misread/misremembered.

Maxime