linux-kernel - Re: kobject_init_and

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