Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <1591142683.16819.44.camel@HansenPartnership.com>
Subject: Re: kobject_init_and_add is easy to misuse
From:   James Bottomley <James.Bottomley@HansenPartnership.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Matthew Wilcox <willy@infradead.org>,
        Wang Hai <wanghai38@huawei.com>, cl@linux.com,
        penberg@kernel.org, rientjes@google.com, iamjoonsoo.kim@lge.com,
        akpm@linux-foundation.org, khlebnikov@yandex-team.ru,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Date:   Tue, 02 Jun 2020 17:04:43 -0700
In-Reply-To: <1591134670.16819.18.camel@HansenPartnership.com>
References: <20200602115033.1054-1-wanghai38@huawei.com>
         <20200602121035.GL19604@bombadil.infradead.org>
         <1591111514.4253.32.camel@HansenPartnership.com>
         <20200602173603.GB3579519@kroah.com>
         <1591127656.16819.7.camel@HansenPartnership.com>
         <20200602200756.GA3933938@kroah.com>
         <1591134670.16819.18.camel@HansenPartnership.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 2020-06-02 at 14:51 -0700, James Bottomley wrote:
> 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.

Actually, I spoke too soon.  I did another analysis of the syzkaller
flow in b8eb718348b8 ("net-sysfs: Fix reference count leak in
rx|netdev_queue_add_kobject") and it turns out there is a single piece
of state that's not correctly unwound: the kobj->name which, thanks to
additions after kobject_init_and_add() was created, is now allocated
via kmalloc if it's not a rodata string and is always and freed in
kobject_cleanup via kfree_const().  This problem can be fixed by
unwinding the name allocation at the end of kobject_init_and_add() ...
or it could be unwound in kobject_add_varg, which would also make
kobject_add() unwind correctly.

The unwind step is to kfree_const(kobj->name); kobj->name = NULL; so it
won't interfere if the kobject_put() is called instead of a simple
kfree.

Would you prefer the unwind in kobject_init_and_add() like the patch
below or in kobject_add_varg()?


James

---

diff --git a/lib/kobject.c b/lib/kobject.c
index 65fa7bf70c57..9991baf43d27 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -472,6 +472,10 @@ int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype,
 	va_start(args, fmt);
 	retval = kobject_add_varg(kobj, parent, fmt, args);
 	va_end(args);
+	if (retval && kobj->name) {
+		kfree_const(kobj->name);
+		kobj->name = NULL;
+	}
 
 	return retval;
 }