* [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()
@ 2022-10-20 13:20 Yu Kuai
2022-10-20 13:20 ` [PATCH -nect RFC v2 1/2] block: add helpers for bd_holder_dir refcount management Yu Kuai
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Yu Kuai @ 2022-10-20 13:20 UTC (permalink / raw)
To: axboe, hch, willy, kch, martin.petersen, johannes.thumshirn,
yukuai3, ming.lei
Cc: linux-block, linux-kernel, yukuai1, yi.zhang
Changes in v2:
- in order to know when bd_holder_dir will be freed, instead of changing
kobject_put(), add a new field in block_device.
Yu Kuai (2):
block: add helpers for bd_holder_dir refcount management
block: fix uaf for bd_holder_dir
block/blk.h | 3 +++
block/genhd.c | 33 ++++++++++++++++++++++++++++++++-
block/holder.c | 19 +++++++++++++------
block/partitions/core.c | 5 ++++-
include/linux/blk_types.h | 1 +
5 files changed, 53 insertions(+), 8 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH -nect RFC v2 1/2] block: add helpers for bd_holder_dir refcount management
2022-10-20 13:20 [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder() Yu Kuai
@ 2022-10-20 13:20 ` Yu Kuai
2022-10-20 13:20 ` [PATCH -nect RFC v2 2/2] block: fix uaf for bd_holder_dir Yu Kuai
2022-10-20 16:47 ` [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder() Christoph Hellwig
2 siblings, 0 replies; 8+ messages in thread
From: Yu Kuai @ 2022-10-20 13:20 UTC (permalink / raw)
To: axboe, hch, willy, kch, martin.petersen, johannes.thumshirn,
yukuai3, ming.lei
Cc: linux-block, linux-kernel, yukuai1, yi.zhang
Currently the lifecycle of bd_holder_dir is problematic, it can be freed
in del_gendisk() while it can still be accessed before disk_release().
And there in no way to know if the kobject if freed by kobject_put() for
now, thus add a new field in bloce_device to manage refcount of
bd_holder_dir seperately.
Prepare to fix the problem in following patch.
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
block/blk.h | 3 +++
block/genhd.c | 27 +++++++++++++++++++++++++++
include/linux/blk_types.h | 1 +
3 files changed, 31 insertions(+)
diff --git a/block/blk.h b/block/blk.h
index d6ea0d1a6db0..fef868b4169c 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -458,6 +458,9 @@ extern const struct address_space_operations def_blk_aops;
int disk_register_independent_access_ranges(struct gendisk *disk);
void disk_unregister_independent_access_ranges(struct gendisk *disk);
+bool bd_try_get_holder_dir(struct block_device *bdev);
+void bd_put_holder_dir(struct block_device *bdev);
+
#ifdef CONFIG_FAIL_MAKE_REQUEST
bool should_fail_request(struct block_device *part, unsigned int bytes);
#else /* CONFIG_FAIL_MAKE_REQUEST */
diff --git a/block/genhd.c b/block/genhd.c
index 17b33c62423d..53f9c8b2690a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -375,6 +375,33 @@ int disk_scan_partitions(struct gendisk *disk, fmode_t mode)
return 0;
}
+bool bd_try_get_holder_dir(struct block_device *bdev)
+{
+ mutex_lock(&bdev->bd_disk->open_mutex);
+ if (!bdev->holder_dir_ref) {
+ mutex_unlock(&bdev->bd_disk->open_mutex);
+ return false;
+ }
+
+ bdev->holder_dir_ref++;
+ mutex_unlock(&bdev->bd_disk->open_mutex);
+ return true;
+}
+
+void bd_put_holder_dir(struct block_device *bdev)
+{
+ mutex_lock(&bdev->bd_disk->open_mutex);
+ if (WARN_ON(bdev->holder_dir_ref <= 0))
+ goto out;
+
+ if (!(--bdev->holder_dir_ref)) {
+ kobject_put(bdev->bd_holder_dir);
+ bdev->bd_holder_dir = NULL;
+ }
+out:
+ mutex_unlock(&bdev->bd_disk->open_mutex);
+}
+
/**
* device_add_disk - add disk information to kernel list
* @parent: parent device for the disk
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index e0b098089ef2..550c8e15af49 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -53,6 +53,7 @@ struct block_device {
int bd_holders;
bool bd_write_holder;
struct kobject *bd_holder_dir;
+ int holder_dir_ref;
u8 bd_partno;
spinlock_t bd_size_lock; /* for bd_inode->i_size updates */
struct gendisk * bd_disk;
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH -nect RFC v2 2/2] block: fix uaf for bd_holder_dir
2022-10-20 13:20 [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder() Yu Kuai
2022-10-20 13:20 ` [PATCH -nect RFC v2 1/2] block: add helpers for bd_holder_dir refcount management Yu Kuai
@ 2022-10-20 13:20 ` Yu Kuai
2022-10-20 16:47 ` [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder() Christoph Hellwig
2 siblings, 0 replies; 8+ messages in thread
From: Yu Kuai @ 2022-10-20 13:20 UTC (permalink / raw)
To: axboe, hch, willy, kch, martin.petersen, johannes.thumshirn,
yukuai3, ming.lei
Cc: linux-block, linux-kernel, yukuai1, yi.zhang
Lifecycle of bd_holder_dir is problematic:
t1: t2:
// get bdev of lower disk
blkdev_get_by_dev
// remove lower disk
del_gendisk
// initial reference is released, and
// bd_holder_dir can be freed
kobject_put
// uaf is triggered
bd_link_disk_holder
Fix the problem by protecting bd_holder_dir by open_mutex.
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
block/genhd.c | 6 +++++-
block/holder.c | 19 +++++++++++++------
block/partitions/core.c | 5 ++++-
3 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 53f9c8b2690a..b3d04e79e854 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -499,6 +499,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
ret = -ENOMEM;
goto out_del_integrity;
}
+ disk->part0->holder_dir_ref = 1;
+
disk->slave_dir = kobject_create_and_add("slaves", &ddev->kobj);
if (!disk->slave_dir) {
ret = -ENOMEM;
@@ -557,6 +559,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
kobject_put(disk->slave_dir);
out_put_holder_dir:
kobject_put(disk->part0->bd_holder_dir);
+ disk->part0->bd_holder_dir = NULL;
+ disk->part0->holder_dir_ref = 0;
out_del_integrity:
blk_integrity_del(disk);
out_del_block_link:
@@ -649,7 +653,7 @@ void del_gendisk(struct gendisk *disk)
blk_unregister_queue(disk);
- kobject_put(disk->part0->bd_holder_dir);
+ bd_put_holder_dir(disk->part0);
kobject_put(disk->slave_dir);
part_stat_set_all(disk->part0, 0);
diff --git a/block/holder.c b/block/holder.c
index 5283bc804cc1..2e28b472ba1a 100644
--- a/block/holder.c
+++ b/block/holder.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <linux/blkdev.h>
#include <linux/slab.h>
+#include "blk.h"
struct bd_holder_disk {
struct list_head list;
@@ -85,9 +86,19 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
goto out_unlock;
}
+ /*
+ * del_gendisk drops the initial reference to bd_holder_dir, so we need
+ * to keep our own here to allow for cleanup past that point.
+ */
+ if (!bd_try_get_holder_dir(bdev)) {
+ ret = -ENODEV;
+ goto out_unlock;
+ }
+
holder = kzalloc(sizeof(*holder), GFP_KERNEL);
if (!holder) {
ret = -ENOMEM;
+ bd_put_holder_dir(bdev);
goto out_unlock;
}
@@ -98,16 +109,12 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk)
ret = __link_disk_holder(bdev, disk);
if (ret) {
kfree(holder);
+ bd_put_holder_dir(bdev);
goto out_unlock;
}
}
list_add(&holder->list, &disk->slave_bdevs);
- /*
- * del_gendisk drops the initial reference to bd_holder_dir, so we need
- * to keep our own here to allow for cleanup past that point.
- */
- kobject_get(bdev->bd_holder_dir);
out_unlock:
mutex_unlock(&disk->open_mutex);
@@ -141,7 +148,7 @@ void bd_unlink_disk_holder(struct block_device *bdev, struct gendisk *disk)
if (!WARN_ON_ONCE(holder == NULL) && !--holder->refcnt) {
if (disk->slave_dir)
__unlink_disk_holder(bdev, disk);
- kobject_put(bdev->bd_holder_dir);
+ bd_put_holder_dir(bdev);
list_del_init(&holder->list);
kfree(holder);
}
diff --git a/block/partitions/core.c b/block/partitions/core.c
index b8112f52d388..39a14f7c308e 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -279,7 +279,7 @@ 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);
+ bd_put_holder_dir(part);
device_del(&part->bd_device);
/*
@@ -390,6 +390,7 @@ static struct block_device *add_partition(struct gendisk *disk, int partno,
bdev->bd_holder_dir = kobject_create_and_add("holders", &pdev->kobj);
if (!bdev->bd_holder_dir)
goto out_del;
+ bdev->holder_dir_ref = 1;
dev_set_uevent_suppress(pdev, 0);
if (flags & ADDPART_FLAG_WHOLEDISK) {
@@ -411,6 +412,8 @@ static struct block_device *add_partition(struct gendisk *disk, int partno,
out_del:
kobject_put(bdev->bd_holder_dir);
+ bdev->bd_holder_dir = NULL;
+ bdev->holder_dir_ref = 0;
device_del(pdev);
out_put:
put_device(pdev);
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()
2022-10-20 13:20 [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder() Yu Kuai
2022-10-20 13:20 ` [PATCH -nect RFC v2 1/2] block: add helpers for bd_holder_dir refcount management Yu Kuai
2022-10-20 13:20 ` [PATCH -nect RFC v2 2/2] block: fix uaf for bd_holder_dir Yu Kuai
@ 2022-10-20 16:47 ` Christoph Hellwig
2022-10-21 3:15 ` Yu Kuai
2 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2022-10-20 16:47 UTC (permalink / raw)
To: Yu Kuai
Cc: axboe, hch, willy, kch, martin.petersen, johannes.thumshirn,
ming.lei, linux-block, linux-kernel, yukuai1, yi.zhang
As mentioned before I don't think we should make this even more
crufty in the block layer. See the series I just sent to move it int
dm.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()
2022-10-20 16:47 ` [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder() Christoph Hellwig
@ 2022-10-21 3:15 ` Yu Kuai
2022-10-26 11:16 ` Yu Kuai
2022-10-30 15:30 ` Christoph Hellwig
0 siblings, 2 replies; 8+ messages in thread
From: Yu Kuai @ 2022-10-21 3:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, willy, kch, martin.petersen, johannes.thumshirn, ming.lei,
linux-block, linux-kernel, yukuai1, yi.zhang, yukuai (C)
Hi,
在 2022/10/21 0:47, Christoph Hellwig 写道:
> As mentioned before I don't think we should make this even more
> crufty in the block layer. See the series I just sent to move it int
> dm.
It seems we had some misunderstanding, the problem I tried to fix here
should not just related to dm, but all the caller of
bd_link_disk_holder().
Thanks,
Kuai
>
> .
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()
2022-10-21 3:15 ` Yu Kuai
@ 2022-10-26 11:16 ` Yu Kuai
2022-10-30 15:30 ` Christoph Hellwig
1 sibling, 0 replies; 8+ messages in thread
From: Yu Kuai @ 2022-10-26 11:16 UTC (permalink / raw)
To: Yu Kuai, Christoph Hellwig
Cc: axboe, willy, kch, martin.petersen, johannes.thumshirn, ming.lei,
linux-block, linux-kernel, yi.zhang, yukuai (C)
Hi, Christoph
在 2022/10/21 11:15, Yu Kuai 写道:
> Hi,
>
> 在 2022/10/21 0:47, Christoph Hellwig 写道:
>> As mentioned before I don't think we should make this even more
>> crufty in the block layer. See the series I just sent to move it int
>> dm.
>
> It seems we had some misunderstanding, the problem I tried to fix here
> should not just related to dm, but all the caller of
> bd_link_disk_holder().
Any suggestions about how to fix this problem?
Thanks,
Kuai
>
> Thanks,
> Kuai
>>
>> .
>>
>
> .
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()
2022-10-21 3:15 ` Yu Kuai
2022-10-26 11:16 ` Yu Kuai
@ 2022-10-30 15:30 ` Christoph Hellwig
2022-10-31 1:08 ` Yu Kuai
1 sibling, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2022-10-30 15:30 UTC (permalink / raw)
To: Yu Kuai
Cc: Christoph Hellwig, axboe, willy, kch, martin.petersen,
johannes.thumshirn, ming.lei, linux-block, linux-kernel,
yi.zhang, yukuai (C)
On Fri, Oct 21, 2022 at 11:15:34AM +0800, Yu Kuai wrote:
> Hi,
>
> 在 2022/10/21 0:47, Christoph Hellwig 写道:
>> As mentioned before I don't think we should make this even more
>> crufty in the block layer. See the series I just sent to move it int
>> dm.
>
> It seems we had some misunderstanding, the problem I tried to fix here
> should not just related to dm, but all the caller of
> bd_link_disk_holder().
As far as I can tell the problem was just that patch 1 in my series blows
away the bd_holder_dir pointer in part0 on del_gendisk. Each holder
actually holds a reference to the kobject, so the memory for it is
still valid, it's just that the pointer got cleared. I'll send a v2
in a bit.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder()
2022-10-30 15:30 ` Christoph Hellwig
@ 2022-10-31 1:08 ` Yu Kuai
0 siblings, 0 replies; 8+ messages in thread
From: Yu Kuai @ 2022-10-31 1:08 UTC (permalink / raw)
To: Christoph Hellwig, Yu Kuai
Cc: axboe, willy, kch, martin.petersen, johannes.thumshirn, ming.lei,
linux-block, linux-kernel, yi.zhang, yukuai (C)
Hi, Christoph
在 2022/10/30 23:30, Christoph Hellwig 写道:
> On Fri, Oct 21, 2022 at 11:15:34AM +0800, Yu Kuai wrote:
>> Hi,
>>
>> 在 2022/10/21 0:47, Christoph Hellwig 写道:
>>> As mentioned before I don't think we should make this even more
>>> crufty in the block layer. See the series I just sent to move it int
>>> dm.
>>
>> It seems we had some misunderstanding, the problem I tried to fix here
>> should not just related to dm, but all the caller of
>> bd_link_disk_holder().
>
> As far as I can tell the problem was just that patch 1 in my series blows
> away the bd_holder_dir pointer in part0 on del_gendisk. Each holder
> actually holds a reference to the kobject, so the memory for it is
> still valid, it's just that the pointer got cleared. I'll send a v2
> in a bit.
This is not the real case. In bd_link_disk_hoder(), bd_hodler_dir is
accessed first by add_symlink(), and then reference is grabed later.
The reference should be grabed before bd_holder_dir is accessed, like
what I try to do in patch 2.
Thanks,
Kuai
>
> .
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-10-31 1:08 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-20 13:20 [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder() Yu Kuai
2022-10-20 13:20 ` [PATCH -nect RFC v2 1/2] block: add helpers for bd_holder_dir refcount management Yu Kuai
2022-10-20 13:20 ` [PATCH -nect RFC v2 2/2] block: fix uaf for bd_holder_dir Yu Kuai
2022-10-20 16:47 ` [PATCH -nect RFC v2 0/2] block: fix uaf in bd_link_disk_holder() Christoph Hellwig
2022-10-21 3:15 ` Yu Kuai
2022-10-26 11:16 ` Yu Kuai
2022-10-30 15:30 ` Christoph Hellwig
2022-10-31 1:08 ` Yu Kuai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).