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
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 <[email protected]>
---
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
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.
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
>
> .
>
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
>>
>> .
>>
>
> .
>
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.
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
>
> .
>