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]
Message-ID: <1591134670.16819.18.camel@HansenPartnership.com>
Date:   Tue, 02 Jun 2020 14:51:10 -0700
From:   James Bottomley <James.Bottomley@...senPartnership.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Wang Hai <wanghai38@...wei.com>, cl@...ux.com,
        penberg@...nel.org, rientjes@...gle.com, iamjoonsoo.kim@....com,
        akpm@...ux-foundation.org, khlebnikov@...dex-team.ru,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: kobject_init_and_add is easy to misuse

On Tue, 2020-06-02 at 22:07 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 02, 2020 at 12:54:16PM -0700, James Bottomley wrote:
[...]
> > I think the only way we can make the failure semantics consistent
> > is to have the kobject_init() ones (so kfree on failure).  That
> > means for the add part, the function would have to unwind
> > everything it did from init on so kfree() is still an option.  If
> > people agree, then I can produce the patch ... it's just the
> > current drive to transform everyone who's doing kfree() into
> > kobject_put() would become wrong ...
> 
> Everyone should be putting their kfree into the kobject release
> anyway, right?

No, that's the problem ... for a static kobject you can't free it; and
the release path may make assumption which aren't valid depending on
the kobject state.

> Anyway, let's see your patch before I start to object further :)

My first thought was "what?  I got suckered into creating a patch",
thanks ;-)  But now I look, all the error paths do unwind back to the
initial state, so kfree() on error looks to be completely correct.  I
got confused by a bogus patch set like this:

https://lore.kernel.org/linux-scsi/20200528201353.14849-1-wu000273@umn.edu/

But it turns out the person sending the patch didn't understand the
network failure they quote:

b8eb718348b8 net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject

Has the problem precisely because the kobject is static.  The release
path clears it and that allows it to be readded.  I'll just reply to
the sender of the bogus patches.

James


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ