Message-ID: <20200421070048.GD347130@kroah.com>
Date:   Tue, 21 Apr 2020 09:00:48 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     axboe@...nel.dk, viro@...iv.linux.org.uk, bvanassche@....org,
        rostedt@...dmis.org, mingo@...hat.com, jack@...e.cz,
        ming.lei@...hat.com, nstange@...e.de, akpm@...ux-foundation.org,
        mhocko@...e.com, yukuai3@...wei.com, linux-block@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Omar Sandoval <osandov@...com>,
        Hannes Reinecke <hare@...e.com>,
        Michal Hocko <mhocko@...nel.org>,
        syzbot+603294af2d01acfdd6da@...kaller.appspotmail.com
Subject: Re: [PATCH v2 03/10] blktrace: fix debugfs use after free

On Mon, Apr 20, 2020 at 08:41:56PM +0000, Luis Chamberlain wrote:
> On Mon, Apr 20, 2020 at 10:16:15PM +0200, Greg KH wrote:
> > On Sun, Apr 19, 2020 at 07:45:22PM +0000, Luis Chamberlain wrote:
> > 
> > This patch triggered gmail's spam detection, your changelog text is
> > whack...
> 
> Oh? What do you think triggered it?

No idea.

> 
> > > diff --git a/block/blk-debugfs.c b/block/blk-debugfs.c
> > > index 19091e1effc0..d84038bce0a5 100644
> > > --- a/block/blk-debugfs.c
> > > +++ b/block/blk-debugfs.c
> > > @@ -3,6 +3,9 @@
> > >  /*
> > >   * Shared request-based / make_request-based functionality
> > >   */
> > > +
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > >  #include <linux/kernel.h>
> > >  #include <linux/blkdev.h>
> > >  #include <linux/debugfs.h>
> > > @@ -13,3 +16,30 @@ void blk_debugfs_register(void)
> > >  {
> > >  	blk_debugfs_root = debugfs_create_dir("block", NULL);
> > >  }
> > > +
> > > +int __must_check blk_queue_debugfs_register(struct request_queue *q)
> > > +{
> > > +	struct dentry *dir = NULL;
> > > +
> > > +	/* This can happen if we have a bug in the lower layers */
> > > +	dir = debugfs_lookup(kobject_name(q->kobj.parent), blk_debugfs_root);
> > > +	if (dir) {
> > > +		pr_warn("%s: registering request_queue debugfs directory twice is not allowed\n",
> > > +			kobject_name(q->kobj.parent));
> > > +		dput(dir);
> > > +		return -EALREADY;
> > > +	}
> > > +
> > > +	q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
> > > +					    blk_debugfs_root);
> > > +	if (!q->debugfs_dir)
> > > +		return -ENOMEM;
> > 
> > Why doesn't the directory just live in the request queue, or somewhere
> > else, so that you save it when it is created and then that's it.  No
> > need to "look it up" anywhere else.
> 
> Its already there. And yes, after my changes it is technically possible
> to just re-use it directly. But this is complicated by a few things. One
> is that at this point in time, asynchronous request_queue removal is
> still possible, and so a race was exposed where a requeust_queue may be
> lingering but its old device is gone. That race is fixed by reverting us
> back to synchronous request_queue removal, therefore ensuring that the
> debugfs dir exists so long as the device does.
> 
> I can remove the debugfs_lookup() *after* we revert to synchronous
> request_queue removal, or we just re-order the patches so that the
> revert happens first. That should simplify this patch.
> 
> The code in this patch was designed to help dispute the logic behind
> the CVE, in particular it shows exactly where debugfs_dir *is* the
> one found by debugfs_lookup(), and shows the real issue behind the
> removal.
> 
> But yeah, now that that is done, I hope its clear to all, and I think
> this patch can be simplified if we just revert the async requeust_queue
> removal first.

Don't try to "dispute" crazyness, that's not what kernel code is for.
Just do the right thing, and simply saving off the pointer to the
debugfs file when created is the "correct" thing to do, no matter what.
No race conditions or anything else can happen when you do that.

> > Or do you do that in later patches?  I only see this one at the moment,
> > sorry...
> > 
> > >  static struct dentry *blk_trace_debugfs_dir(struct blk_user_trace_setup *buts,
> > > +					    struct request_queue *q,
> > >  					    struct blk_trace *bt)
> > >  {
> > >  	struct dentry *dir = NULL;
> > >  
> > > +	/* This can only happen if we have a bug on our lower layers */
> > > +	if (!q->kobj.parent) {
> > > +		pr_warn("%s: request_queue parent is gone\n", buts->name);
> > 
> > A kobject always has a parent, unless it has not been registered yet, so
> > I don't know what you are testing could ever happen.
> 
> Or it has been kobject_del()'d?

If that happened, how in the world are you in this function anyway, as
the request_queue is an invalid pointer at that point in time???

> A deferred requeust_queue removal shows this is possible, the parent is
> taken underneath from us because the refcounting of this kobject is
> already kobject_del()'d, and its actual removal scheduled for later.
> The parent is taken underneath from us prior to the scheduled removal
> completing.

No, a parent's reference is always valid while the child pointer is
alive.

There is an odd race condition right now that we are working on fixing
if you notice another lkml thread, but that race will soon be fixed.  So
no need for every single user in the kernel to try to test for something
like this (hint, this check is still wrong as with this logic, what
could prevent parent from going away right _after_ you check it???)

Just remove this invalid check please.

> > > +		return NULL;
> > > +	}
> > > +
> > > +	/*
> > > +	 * From a sysfs kobject perspective, the request_queue sits on top of
> > > +	 * the gendisk, which has the name of the disk. We always create a
> > > +	 * debugfs directory upon init for this gendisk kobject, so we re-use
> > > +	 * that if blktrace is going to be done for it.
> > > +	 */
> > > +	if (blk_trace_target_disk(buts->name, kobject_name(q->kobj.parent))) {
> > > +		if (!q->debugfs_dir) {
> > > +			pr_warn("%s: expected request_queue debugfs_dir is not set\n",
> > > +				buts->name);
> > 
> > What is userspace supposed to be able to do if they see this warning?
> 
> Userspace doesn't parse warnings, but the NULL ensures it won't crash
> the kernel. The warn informs the kernel of a possible block layer bug.

Again, the code should not care if the pointer is NULL, as only debugfs
deals with those pointers (and it can handle a NULL pointer just fine.)

> > > +			return NULL;
> > > +		}
> > > +		/*
> > > +		 * debugfs_lookup() is used to ensure the directory is not
> > > +		 * taken from underneath us. We must dput() it later once
> > > +		 * done with it within blktrace.
> > > +		 */
> > > +		dir = debugfs_lookup(buts->name, blk_debugfs_root);
> > > +		if (!dir) {
> > > +			pr_warn("%s: expected request_queue debugfs_dir dentry is gone\n",
> > > +				buts->name);
> > 
> > Again, can't we just save the pointer when we create it and not have to
> > look it up again?
> 
> Only if we do the revert of the requeust_queue removal first.

Your end goal should be no more debugfs_lookup() calls.  Hopefully by
the end of this patch series, that is the result.

> > > +			return NULL;
> > > +		}
> > > +		 /*
> > > +		 * This is a reaffirmation that debugfs_lookup() shall always
> > > +		 * return the same dentry if it was already set.
> > > +		 */
> > 
> > I'm all for reaffirmation and the like, but really, is this needed???
> 
> To those who were still not sure that the issue was not a debugfs issue
> I hoped this to make it clear. But indeed, if we revert back to
> synchronous request_queue removal, that should simplify this.

Again, don't pander to crazies :)

thanks,

greg k-h

Powered by blists - more mailing lists