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