On Sat, Apr 04, 2020 at 08:39:47PM -0700, Bart Van Assche wrote:
> On 2020-04-01 17:00, Luis Chamberlain wrote:
> > korg#205713 then was used to create CVE-2019-19770 and claims that
> > the bug is in a use-after-free in the debugfs core code. The
> > implications of this being a generic UAF on debugfs would be
> > much more severe, as it would imply parent dentries can sometimes
> > not be possitive, which is something claim is not possible.
> ^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> positive? is there perhaps a word missing here?
Sorry yeah, this was supposed to say:
it would imply parent dentries can sometimes not be positive, which
is just not possible.
> > It turns out that the issue actually is a mis-use of debugfs for
> > the multiqueue case, and the fragile nature of how we free the
> > directory used to keep track of blktrace debugfs files. Omar's
> > commit assumed the parent directory would be kept with
> > debugfs_lookup() but this is not the case, only the dentry is
> > kept around. We also special-case a solution for multiqueue
> > given that for multiqueue code we always instantiate the debugfs
> > directory for the request queue. We were leaving it only to chance,
> > if someone happens to use blktrace, on single queue block devices
> > for the respective debugfs directory be created.
>
> Since the legacy block layer is gone, the above explanation may have to
> be rephrased.
Will do.
> > We can fix the UAF by simply using a debugfs directory which is
> > always created for singlequeue and multiqueue block devices. This
> > simplifies the code considerably, with the only penalty now being
> > that we're always creating the request queue directory debugfs
> > directory for the block device on singlequeue block devices.
>
> Same comment here - the legacy block layer is gone. I think that today
> all block drivers are either request-based and multiqueue or so-called
> make_request drivers. See also the output of git grep -nHw
> blk_alloc_queue for examples of the latter category.
Will adjust.
> > This patch then also contends the severity of CVE-2019-19770 as
> > this issue is only possible using root to shoot yourself in the
> > foot by also misuing blktrace.
> ^^^^^^^
> misusing?
>
> > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> > index b3f2ba483992..bda9378eab90 100644
> > --- a/block/blk-mq-debugfs.c
> > +++ b/block/blk-mq-debugfs.c
> > @@ -823,9 +823,6 @@ void blk_mq_debugfs_register(struct request_queue *q)
> > struct blk_mq_hw_ctx *hctx;
> > int i;
> >
> > - q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
> > - blk_debugfs_root);
> > -
> > debugfs_create_files(q->debugfs_dir, q, blk_mq_debugfs_queue_attrs);
> >
> > /*
>
> [ ... ]
>
> > static void blk_mq_debugfs_register_ctx(struct blk_mq_hw_ctx *hctx,
> > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > index fca9b158f4a0..20f20b0fa0b9 100644
> > --- a/block/blk-sysfs.c
> > +++ b/block/blk-sysfs.c
> > @@ -895,6 +895,7 @@ static void __blk_release_queue(struct work_struct *work)
> >
> > blk_trace_shutdown(q);
> >
> > + blk_q_debugfs_unregister(q);
> > if (queue_is_mq(q))
> > blk_mq_debugfs_unregister(q);
>
> Does this patch change the behavior of the block layer from only
> registering a debugfs directory for request-based block devices to
> registering a debugfs directory for request-based and make_request based
> block devices? Is that behavior change an intended behavior change?
Yes, specifically this was already done, however for request-based block
devices this was done upon init, and for make_request based devices this
was only instantiated *iff* blktrace was used at least once. It is
actually a bit difficult to see the later, given the rq->debugfs_dir was
not used per se for make_request based block devices, but instead
the debugfs_create_dir(buts->name, blk_debugfs_root) call was made
directly, which happens to end up being the same directory as
debugfs_create_dir(kobject_name(q->kobj.parent), blk_debugfs_root)
called on block/blk-mq-debugfs.c.
This changes the block layer so that the rq->debugfs_dir is always created
now if debugfs is enabled.
Note that blktrace already depends on debugfs. What was missing in this
patch too was this hunk:
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -569,8 +569,10 @@ struct request_queue {
struct list_head tag_set_list;
struct bio_set bio_split;
-#ifdef CONFIG_BLK_DEBUG_FS
+#ifdef CONFIG_DEBUG_FS
struct dentry *debugfs_dir;
+#endif
+#ifdef CONFIG_BLK_DEBUG_FS
struct dentry
*sched_debugfs_dir;
struct dentry
*rqos_debugfs_dir;
#endif
