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

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]

Date:   Mon, 20 Apr 2020 00:04:36 +0000
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Bart Van Assche <bvanassche@....org>
Cc:     axboe@...nel.dk, viro@...iv.linux.org.uk,
        gregkh@...uxfoundation.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 Sun, Apr 19, 2020 at 02:55:42PM -0700, Bart Van Assche wrote:
> On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> > +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 */
> 
> What does "this" refer to? Which layers does "lower layers" refer to? Most
> software developers consider a module that calls directly into another
> module as a higher layer (callbacks through function pointers do not count;
> see also https://en.wikipedia.org/wiki/Modular_programming). According to
> that definition block drivers are a software layer immediately above the
> block layer core.
> 
> How about changing that comment into the following to make it unambiguous
> (if this is what you meant)?
> 
> 	/*
> 	 * Check whether the debugfs directory already exists. This can
> 	 * only happen as the result of a bug in a block driver.
> 	 */

But I didn't mean on a block driver. I meant the block core. In our
case, the async request_queue removal is an example. There is nothing
that block drivers could have done to help much with that.

I could just change "lower layers" to "block layer" ?

> > +	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;
> > +
> > +	return 0;
> > +}
> 
> kobject_name(q->kobj.parent) is used three times in the above function. How
> about introducing a local variable that holds the result of that expression?

Sure.

> > +static bool blk_trace_target_disk(const char *target, const char *diskname)
> > +{
> > +	if (strlen(target) != strlen(diskname))
> > +		return false;
> > +
> > +	if (!strncmp(target, diskname,
> > +		     min_t(size_t, strlen(target), strlen(diskname))))
> > +		return true;
> > +
> > +	return false;
> > +}
> 
> The above code looks weird to me. When the second if-statement is reached,
> it is guaranteed that 'target' and 'diskname' have the same length. So why
> to calculate the minimum length in the second if-statement of two strings
> that have the same length?

True, no need that that point. Will fix.

> Independent of what the purpose of the above code is, can that code be
> rewritten such that it does not depend on the details of how names are
> assigned to disks and partitions? Would disk_get_part() be useful here?

I did try, but couldn't figure out a way. I'll keep looking but likewise
let me know if you find a way.

  Luis