linux-kernel - Re: [PATCH 1/2] drivers: core: Don't try to use a dead glue

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, 7 Jul 2018 18:51:00 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Benjamin Herrenschmidt <benh@...nel.crashing.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Joel Stanley <joel@....id.au>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] drivers: core: Don't try to use a dead glue_dir

On Fri, Jun 29, 2018 at 12:21:51PM +1000, Benjamin Herrenschmidt wrote:
> Under some circumstances (such as when using kobject debugging)
> a gluedir whose kref is 0 might remain in the class kset for
> a long time. The reason is that we don't actively remove glue
> dirs when they become empty, but instead rely on the implicit
> removal done by kobject_release(), which can happen some amount
> of time after the last kobject_put().
> 
> Using such a dead object is a bad idea and will lead to warnings
> and crashes.
> 
> Unfortunately that can happen in get_device_parent() if the
> last child of a glue dir was removed and a new one added
> before the glue dir gets fully released().
> 
> This prevents this by making get_device_parent() only "find"
> a glue dir whose refcount is non-0.
> 
> While this fixes the crash, it doesn't fully fix the problem,
> instead the race will now result in an error attempting to
> use a duplicate file name in sysfs. A fix for that will come
> separately.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> ---
> 
> (Adding lkml, I just realized I completely forgot to CC it in
> the first place on this whole conversation, blame the 1am debugging
> session)
> 
>  drivers/base/core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index b610816eb887..e9eff2099896 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1517,11 +1517,13 @@ static struct kobject *get_device_parent(struct device *dev,
>  
>  		/* find our class-directory at the parent and reference it */
>  		spin_lock(&dev->class->p->glue_dirs.list_lock);
> -		list_for_each_entry(k, &dev->class->p->glue_dirs.list, entry)
> +		list_for_each_entry(k, &dev->class->p->glue_dirs.list, entry) {
>  			if (k->parent == parent_kobj) {
> -				kobj = kobject_get(k);
> -				break;
> +				kobj = kobject_get_unless_zero(k);
> +				if (kobj)
> +					break;

A parent directory _should_ not ever be able to be removed before the
object being removed was, as we should have had a reference to it,
right?  So I don't see how this can get hit "in real life".

Yes, enabling kobject debugging does keep objects around for a long time
in order to try to help figure out where people are messing up their
usage of them.  What subsystem is doing this in a way that causes
problems here?  Shouldn't we fix that up instead?

thanks,

greg k-h