Re: [PATCH RFC] block: fix use after free for bd_holder_dir/slave_dir

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

 



Hi,

在 2022/10/12 23:49, Greg KH 写道:
On Wed, Oct 12, 2022 at 08:58:38PM +0800, Yu Kuai wrote:
From: Yu Kuai <yukuai3@xxxxxxxxxx>

Our test report a following uaf:

==================================================================
BUG: KASAN: use-after-free in sysfs_remove_link+0x23/0x60
Read of size 8 at addr ffff888117398db0 by task dmsetup/1097

CPU: 12 PID: 1097 Comm: dmsetup Not tainted 6.0.0-next-20221007-00011-gf46c8956
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-4
Call Trace:
  <TASK>
  ? dump_stack_lvl+0x73/0x9f
  ? print_report+0x249/0x746
  ? __virt_addr_valid+0xd4/0x200
  ? sysfs_remove_link+0x23/0x60
  ? kasan_report+0xc0/0x120
  ? sysfs_remove_link+0x23/0x60
  ? __asan_load8+0x74/0x110
  ? sysfs_remove_link+0x23/0x60
  ? __unlink_disk_holder.isra.0+0x2f/0x80
  ? bd_unlink_disk_holder+0xd2/0x1c0
  ? dm_put_table_device+0xf1/0x250
  ? dm_put_device+0x14f/0x230
  ? linear_dtr+0x34/0x50
  ? dm_table_destroy+0x7b/0x280
  ? table_load+0x34a/0x710
  ? list_devices+0x4c0/0x4c0
  ? kvmalloc_node+0x7d/0x160
  ? __kmalloc_node+0x185/0x2b0
  ? ctl_ioctl+0x388/0x7b0
  ? list_devices+0x4c0/0x4c0
  ? free_params+0x50/0x50
  ? do_vfs_ioctl+0x931/0x10d0
  ? tick_program_event+0x65/0xd0
  ? __kasan_check_read+0x1d/0x30
  ? __fget_light+0xc2/0x370
  ? dm_ctl_ioctl+0x12/0x20
  ? __x64_sys_ioctl+0xd5/0x150
  ? do_syscall_64+0x35/0x80
  ? entry_SYSCALL_64_after_hwframe+0x63/0xcd
  </TASK>

Allocated by task 1097:
  kasan_save_stack+0x26/0x60
  kasan_set_track+0x29/0x40
  kasan_save_alloc_info+0x1f/0x40
  __kasan_kmalloc+0xcb/0xe0
  kmalloc_trace+0x7e/0x150
  kobject_create_and_add+0x3d/0xc0
  device_add_disk+0x429/0x7e0
  dm_setup_md_queue+0x15b/0x240
  table_load+0x469/0x710
  ctl_ioctl+0x388/0x7b0
  dm_ctl_ioctl+0x12/0x20
  __x64_sys_ioctl+0xd5/0x150
  do_syscall_64+0x35/0x80
  entry_SYSCALL_64_after_hwframe+0x63/0xcd

Freed by task 1097:
  kasan_save_stack+0x26/0x60
  kasan_set_track+0x29/0x40
  kasan_save_free_info+0x32/0x60
  __kasan_slab_free+0x172/0x2c0
  __kmem_cache_free+0x11c/0x560
  kfree+0xd3/0x240
  dynamic_kobj_release+0x1e/0x60
  kobject_put+0x192/0x410
  device_add_disk+0x535/0x7e0
  dm_setup_md_queue+0x15b/0x240
  table_load+0x469/0x710
  ctl_ioctl+0x388/0x7b0
  dm_ctl_ioctl+0x12/0x20
  __x64_sys_ioctl+0xd5/0x150
  do_syscall_64+0x35/0x80
  entry_SYSCALL_64_after_hwframe+0x63/0xcd

This problem is very easy to reporduce by injecting failure while creating
dm, and root cause is that lifetime of bd_holder_dir/slave_dir is
problematic:

1) device alloc
alloc_disk_node
  kzalloc_node			-> gendisk
  disk->part0 = bdev_alloc	-> part0
  device_initialize
   kobject_init()		-> part0->bd_device

2) device create
device_add_disk
  device_add
   kobject_add			-> part0->bd_device
  kobject_create_and_add		-> part0->bd_holder_dir
  kobject_create_and_add		-> gendisk->slave_dir

3) device remove
del_gendisk
  kobject_put			-> part0->bd_holder_dir
  kobject_put			-> gendisk->slave_dir
  device_del
   kobject_del			-> part0->bd_device

4) device free
disk_release
  iput
   bdev_free_inode
    kfree			-> gendisk
    kmem_cache_free		-> part0

bd_holder_dir and slave_dir is allocated in step 2), and freed in steup
3), while they are still accessible before step 4).

This patch delay the kobject destruction to disk_release/part_release to
fix the problem.

Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
---
  block/genhd.c           | 7 ++++---
  block/partitions/core.c | 3 ++-
  drivers/base/core.c     | 1 -
  3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 514395361d7c..7ebd085d3a3f 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -615,9 +615,6 @@ void del_gendisk(struct gendisk *disk)
blk_unregister_queue(disk); - kobject_put(disk->part0->bd_holder_dir);
-	kobject_put(disk->slave_dir);
-
  	part_stat_set_all(disk->part0, 0);
  	disk->part0->bd_stamp = 0;
  	if (!sysfs_deprecated)
@@ -1139,6 +1136,10 @@ static void disk_release(struct device *dev)
  	might_sleep();
  	WARN_ON_ONCE(disk_live(disk));
+ kobject_put(disk->part0->bd_holder_dir);
+	kobject_put(disk->slave_dir);
+	kobject_del(&dev->kobj);
+
  	/*
  	 * To undo the all initialization from blk_mq_init_allocated_queue in
  	 * case of a probe failure where add_disk is never called we have to
diff --git a/block/partitions/core.c b/block/partitions/core.c
index b8112f52d388..b86a66198cc8 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -250,6 +250,8 @@ static const struct attribute_group *part_attr_groups[] = {
static void part_release(struct device *dev)
  {
+	kobject_put(dev_to_bdev(dev)->bd_holder_dir);
+	kobject_del(&dev->kobj);
  	put_disk(dev_to_bdev(dev)->bd_disk);
  	iput(dev_to_bdev(dev)->bd_inode);
  }
@@ -279,7 +281,6 @@ static void delete_partition(struct block_device *part)
  	__invalidate_device(part, true);
xa_erase(&part->bd_disk->part_tbl, part->bd_partno);
-	kobject_put(part->bd_holder_dir);
  	device_del(&part->bd_device);
/*
diff --git a/drivers/base/core.c b/drivers/base/core.c
index d02501933467..d2ff3d2a8710 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3712,7 +3712,6 @@ void device_del(struct device *dev)
  					     BUS_NOTIFY_REMOVED_DEVICE, dev);
  	kobject_uevent(&dev->kobj, KOBJ_REMOVE);
  	glue_dir = get_glue_dir(dev);
-	kobject_del(&dev->kobj);

Wait, no, you can't do this without having to change a bunch of other
code too.  This feels really wrong, how did you test that this actually
works for all devices in the systems (not just block devices)?

Yes, you're right, I just tested some common block devices, and I don't
have a good idea to fix this yet...

Thanks,
Kuai

so NAK on this change.

greg k-h
.





[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