lkml.org 
[lkml]   [2020]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC 2/3] blktrace: fix debugfs use after free
    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

    \
     
     \ /
      Last update: 2020-04-06 17:15    [W:3.078 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site