lkml.org 
[lkml]   [2019]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.1 162/405] block: fix use-after-free on gendisk
    Date
    [ Upstream commit 2c88e3c7ec32d7a40cc7c9b4a487cf90e4671bdd ]

    commit 2da78092dda "block: Fix dev_t minor allocation lifetime"
    specifically moved blk_free_devt(dev->devt) call to part_release()
    to avoid reallocating device number before the device is fully
    shutdown.

    However, it can cause use-after-free on gendisk in get_gendisk().
    We use md device as example to show the race scenes:

    Process1 Worker Process2
    md_free
    blkdev_open
    del_gendisk
    add delete_partition_work_fn() to wq
    __blkdev_get
    get_gendisk
    put_disk
    disk_release
    kfree(disk)
    find part from ext_devt_idr
    get_disk_and_module(disk)
    cause use after free

    delete_partition_work_fn
    put_device(part)
    part_release
    remove part from ext_devt_idr

    Before <devt, hd_struct pointer> is removed from ext_devt_idr by
    delete_partition_work_fn(), we can find the devt and then access
    gendisk by hd_struct pointer. But, if we access the gendisk after
    it have been freed, it can cause in use-after-freeon gendisk in
    get_gendisk().

    We fix this by adding a new helper blk_invalidate_devt() in
    delete_partition() and del_gendisk(). It replaces hd_struct
    pointer in idr with value 'NULL', and deletes the entry from
    idr in part_release() as we do now.

    Thanks to Jan Kara for providing the solution and more clear comments
    for the code.

    Fixes: 2da78092dda1 ("block: Fix dev_t minor allocation lifetime")
    Cc: Al Viro <viro@zeniv.linux.org.uk>
    Reviewed-by: Bart Van Assche <bvanassche@acm.org>
    Reviewed-by: Keith Busch <keith.busch@intel.com>
    Reviewed-by: Jan Kara <jack@suse.cz>
    Suggested-by: Jan Kara <jack@suse.cz>
    Signed-off-by: Yufen Yu <yuyufen@huawei.com>
    Signed-off-by: Jens Axboe <axboe@kernel.dk>
    Signed-off-by: Sasha Levin <sashal@kernel.org>
    ---
    block/genhd.c | 19 +++++++++++++++++++
    block/partition-generic.c | 7 +++++++
    include/linux/genhd.h | 1 +
    3 files changed, 27 insertions(+)

    diff --git a/block/genhd.c b/block/genhd.c
    index 703267865f14d..d8dff0b21f7d1 100644
    --- a/block/genhd.c
    +++ b/block/genhd.c
    @@ -531,6 +531,18 @@ void blk_free_devt(dev_t devt)
    }
    }

    +/**
    + * We invalidate devt by assigning NULL pointer for devt in idr.
    + */
    +void blk_invalidate_devt(dev_t devt)
    +{
    + if (MAJOR(devt) == BLOCK_EXT_MAJOR) {
    + spin_lock_bh(&ext_devt_lock);
    + idr_replace(&ext_devt_idr, NULL, blk_mangle_minor(MINOR(devt)));
    + spin_unlock_bh(&ext_devt_lock);
    + }
    +}
    +
    static char *bdevt_str(dev_t devt, char *buf)
    {
    if (MAJOR(devt) <= 0xff && MINOR(devt) <= 0xff) {
    @@ -793,6 +805,13 @@ void del_gendisk(struct gendisk *disk)

    if (!(disk->flags & GENHD_FL_HIDDEN))
    blk_unregister_region(disk_devt(disk), disk->minors);
    + /*
    + * Remove gendisk pointer from idr so that it cannot be looked up
    + * while RCU period before freeing gendisk is running to prevent
    + * use-after-free issues. Note that the device number stays
    + * "in-use" until we really free the gendisk.
    + */
    + blk_invalidate_devt(disk_devt(disk));

    kobject_put(disk->part0.holder_dir);
    kobject_put(disk->slave_dir);
    diff --git a/block/partition-generic.c b/block/partition-generic.c
    index 8e596a8dff321..aee643ce13d15 100644
    --- a/block/partition-generic.c
    +++ b/block/partition-generic.c
    @@ -285,6 +285,13 @@ void delete_partition(struct gendisk *disk, int partno)
    kobject_put(part->holder_dir);
    device_del(part_to_dev(part));

    + /*
    + * Remove gendisk pointer from idr so that it cannot be looked up
    + * while RCU period before freeing gendisk is running to prevent
    + * use-after-free issues. Note that the device number stays
    + * "in-use" until we really free the gendisk.
    + */
    + blk_invalidate_devt(part_devt(part));
    hd_struct_kill(part);
    }

    diff --git a/include/linux/genhd.h b/include/linux/genhd.h
    index 06c0fd594097d..69db1affedb0b 100644
    --- a/include/linux/genhd.h
    +++ b/include/linux/genhd.h
    @@ -610,6 +610,7 @@ struct unixware_disklabel {

    extern int blk_alloc_devt(struct hd_struct *part, dev_t *devt);
    extern void blk_free_devt(dev_t devt);
    +extern void blk_invalidate_devt(dev_t devt);
    extern dev_t blk_lookup_devt(const char *name, int partno);
    extern char *disk_name (struct gendisk *hd, int partno, char *buf);

    --
    2.20.1


    \
     
     \ /
      Last update: 2019-05-30 05:12    [W:2.749 / U:0.376 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site