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);
}
