Re: [PATCH 1/2] block: remove devt from ext_devt_idr when delete partition

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi


On 2019/3/30 6:42, Jan Kara wrote:
On Sat 23-03-19 20:51:45, yuyufen wrote:
This patch fix use-after-free on gendisk when open the disk partition.

Ping and Cc

Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: Bart Van Assche <bart.vanassche@xxxxxxx>
Cc: Jan Kara <jack@xxxxxxx>

Yufen
Thanks

On 2019/3/18 22:07, Yufen Yu wrote:
In part_release(), it will remove devt from ext_devt_idr and
get_gendisk cannot find it. But, if disk_release() works before
part_release, open device partition may cause use-after-free of
disk in get_gendisk(). We use md device as example, the race sence:

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 Woker thread removes part from ext_devt_idr, Process2 can find
the part and access the disk, resulting use-after-free.

We fix this by removing the devt from ext_devt_idr when delete partition.

Signed-off-by: Yufen Yu <yuyufen@xxxxxxxxxx>
Thanks for your analysis and the patch! I agree with our analysis of the
problem but I'm afraid the fix won't be so straightforward. The problem is
that once you remove device number from idr, it can get reallocated for
another device. So if nothing else, the idr_remove() call you've left in
part_release() could delete *newly created device* from idr which is not
what you want. And even if this is fixed, there are other possible issues
with reallocating device number before the device is fully shutdown -
commit 2da78092dda "block: Fix dev_t minor allocation lifetime"
specifically moved blk_free_devt(dev->devt) call to part_release() to avoid
such issues (sadly without explaining more details). Adding Keith to CC
just in case he remembers.

Anyway, you are right that it is fundamentally wrong to expose a structure
(hd_struct in our case) for lookup in idr when it is essentially scheduled
for RCU-delayed freeing. So what we can do is to replace hd_struct pointer
in idr with some placeholder value - NULL seems to be OK with idr - and
delete the entry from idr in part_release() as we do now. We can call this
new helper blk_invalidate_devt() or something like that.

								Honza


Thanks a lot for your detailed explanation:-). And replacing hd_struct pointer by
NULL in idr is a good idea. I will send v2 for this problem.

Yufen
Thanks

---
   block/partition-generic.c | 5 +++++
   1 file changed, 5 insertions(+)

diff --git a/block/partition-generic.c b/block/partition-generic.c
index 1ee3e1d1bc2a..30d1039d5e8d 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -288,6 +288,11 @@ void delete_partition(struct gendisk *disk, int partno)
   	kobject_put(part->holder_dir);
   	device_del(part_to_dev(part));
+	/*
+	 * We should ensuere to delete part from idr before kfree(disk),
+	 * avoiding use-after-free of disk.
+	 */
+	blk_free_devt(part_devt(part));
   	hd_struct_kill(part);
   }






[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux