Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH RFC] block: fix use after free for bd_holder_dir/slave_dir
To:     Greg KH <gregkh@linuxfoundation.org>,
        Yu Kuai <yukuai1@huaweicloud.com>
Cc:     axboe@kernel.dk, willy@infradead.org, kch@nvidia.com,
        martin.petersen@oracle.com, johannes.thumshirn@wdc.com,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        yi.zhang@huawei.com, "yukuai (C)" <yukuai3@huawei.com>
References: <20221012125838.1608619-1-yukuai1@huaweicloud.com>
 <Y0biFaIxrs+faeao@kroah.com>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <f988d278-16e8-bcd0-7603-6079e6b9be84@huaweicloud.com>
Date:   Thu, 13 Oct 2022 09:09:19 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <Y0biFaIxrs+faeao@kroah.com>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@huawei.com>
>>
>> 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@huawei.com>
>> ---
>>   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
> .
>