linux-kernel - Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject

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

Message-ID: <20190803071555.GB24757@kroah.com>
Date:   Sat, 3 Aug 2019 09:15:55 +0200
From:   "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
To:     Michael Kelley <mikelley@...rosoft.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "boqun.feng" <boqun.feng@...il.com>,
        kimbrownkd <kimbrownkd@...il.com>
Subject: Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add

On Fri, Aug 02, 2019 at 08:19:37PM +0000, Michael Kelley wrote:
> From: gregkh@...uxfoundation.org <gregkh@...uxfoundation.org> Sent: Thursday, August 1, 2019 11:34 PM
> > On Thu, Aug 01, 2019 at 11:53:53PM +0000, Michael Kelley wrote:
> > > If alloc_descs fails before irq_sysfs_init has run, free_desc in the
> > > cleanup path will call kobject_del even though the kobject has not
> > > been added with kobject_add. Fix this by making the call to
> > > kobject_del conditional on whether irq_sysfs_init has run.
> > >
> > > This problem surfaced because commit aa30f47cf666
> > > ("kobject: Add support for default attribute groups to kobj_type")
> > > makes kobject_del stricter about pairing with kobject_add. If the
> > > pairing is incorrrect, a WARNING and backtrace occur in
> > > sysfs_remove_group because there is no parent.
> > >
> > > Fixes: ecb3f394c5db ("genirq: Expose interrupt information through sysfs")
> > > Signed-off-by: Michael Kelley <mikelley@...rosoft.com>
> > > ---
> > >  kernel/irq/irqdesc.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> > > index 9484e88..5447760 100644
> > > --- a/kernel/irq/irqdesc.c
> > > +++ b/kernel/irq/irqdesc.c
> > > @@ -438,7 +438,8 @@ static void free_desc(unsigned int irq)
> > >  	 * The sysfs entry must be serialized against a concurrent
> > >  	 * irq_sysfs_init() as well.
> > >  	 */
> > > -	kobject_del(&desc->kobj);
> > > +	if (irq_kobj_base)
> > > +		kobject_del(&desc->kobj);
> > 
> > But now you leak the memory of desc as there is no chance it could be
> > freed, because the kobject release function is never called :(
> 
> In the alloc_descs error path, when irq_kobj_base is still NULL, the
> kobject code sequence is:
> 	kobject_init()   [as called by alloc_desc]
> 	kobject_put()   [as called by delayed_free_desc]
> 
> So I don't think anything leaks.
> 
> If irq_kobj_base is not NULL, the kobject code sequence is:
> 	kobject_init()   [as called by alloc_desc]
> 	kobject_add()  [as called by irq_sysfs_add]
> 	kobject_del()   [as called by free_desc]
> 	kobject_put()   [as called by delayed_free_desc]
> 
> Again, everything is paired up properly.
> 
> > 
> > Relying on irq_kobj_base to be present or not seems like an odd test
> > here.
> > 
> 
> It's the same test that is used in irq_sysfs_add to decide whether to
> call kobject_add.  So it makes everything paired up and symmetrical.

Ugh, that's a tangled mess and totally not obvious at all.  I'm sure
there's a good reason for all of that, and I really don't want to know
:)

Anyway, yes, you are right, the patch is fine, sorry for the noise.

Acked-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>

greg k-h