Re: [PATCH v2 03/10] blktrace: fix debugfs use after free

[Luis Chamberlain]

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?

> > 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.

> 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?

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.

> 
> > +		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.

> > +			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.

> > +			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.

> > +		if (dir != q->debugfs_dir) {
> > +			dput(dir);
> > +			pr_warn("%s: expected dentry dir != q->debugfs_dir\n",
> > +				buts->name);
> > +			return NULL;
> 
> Why are you testing to see if debugfs really is working properly?
> SHould all users do crazy things like this (hint, rhetorical
> question...)

No, this can happen with the race I mentioned above.

> > +		}
> > +		bt->backing_dir = q->debugfs_dir;
> > +		return bt->backing_dir;
> > +	}
> > +
> > +	/*
> > +	 * If not using blktrace on the gendisk, we are going to create a
> > +	 * temporary debugfs directory for it, however this cannot be shared
> > +	 * between two concurrent blktraces since the path is not unique, so
> > +	 * ensure this is only done once.
> > +	 */
> >  	dir = debugfs_lookup(buts->name, blk_debugfs_root);
> > -	if (!dir)
> > -		bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> > +	if (dir) {
> > +		pr_warn("%s: temporary blktrace debugfs directory already present\n",
> > +			buts->name);
> > +		dput(dir);
> > +		return NULL;
> > +	}
> > +
> > +	bt->dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> > +	if (!bt->dir) {
> > +		pr_warn("%s: temporary blktrace debugfs directory could not be created\n",
> > +			buts->name);
> 
> Again, do not test the return value, you do not care.  I've been
> removing these checks from everywhere.

Sure, the question still stands on what *should* the kernel do if the
blktrace setup failed to create the debugfs files.

  Luis