linux-kernel - [PATCH v2] ubi: block: Fix cleanup handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230523-ubiblock-remove-v2-1-7671fc60ba49@axis.com>
Date:   Wed, 7 Jun 2023 11:25:06 +0200
From:   Vincent Whitchurch <vincent.whitchurch@...s.com>
To:     Richard Weinberger <richard@....at>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Vignesh Raghavendra <vigneshr@...com>
CC:     <hch@...radead.org>, <linux-mtd@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <kernel@...s.com>,
        Vincent Whitchurch <vincent.whitchurch@...s.com>
Subject: [PATCH v2] ubi: block: Fix cleanup handling

ubiblock's remove handling has a couple of problems:

 - It uses the gendisk after put_disk(), resulting in a use-after-free.

 - There is a circular locking dependency between disk->open_mutex (used
   from del_gendisk() and blkdev_open()) and dev->dev_mutex (used from
   ubiblock_open() and ubiblock_remove()).

Fix these by implementing ->free_disk() and moving the final cleanup
there.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@...s.com>
---
Changes in v2:
- Combine and rework patches to implement and use ->free_disk().
- Link to v1: https://lore.kernel.org/r/20230523-ubiblock-remove-v1-0-240bed75849b@axis.com
---
 drivers/mtd/ubi/block.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 3711d7f74600..570e660673ad 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -293,11 +293,23 @@ static int ubiblock_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	return 0;
 }
 
+static void ubiblock_free_disk(struct gendisk *disk)
+{
+	struct ubiblock *dev = disk->private_data;
+
+	mutex_lock(&devices_mutex);
+	idr_remove(&ubiblock_minor_idr, disk->first_minor);
+	mutex_unlock(&devices_mutex);
+
+	kfree(dev);
+}
+
 static const struct block_device_operations ubiblock_ops = {
 	.owner = THIS_MODULE,
 	.open = ubiblock_open,
 	.release = ubiblock_release,
 	.getgeo	= ubiblock_getgeo,
+	.free_disk = ubiblock_free_disk,
 };
 
 static blk_status_t ubiblock_queue_rq(struct blk_mq_hw_ctx *hctx,
@@ -452,9 +464,8 @@ static void ubiblock_cleanup(struct ubiblock *dev)
 	del_gendisk(dev->gd);
 	/* Finally destroy the blk queue */
 	dev_info(disk_to_dev(dev->gd), "released");
-	put_disk(dev->gd);
 	blk_mq_free_tag_set(&dev->tag_set);
-	idr_remove(&ubiblock_minor_idr, dev->gd->first_minor);
+	put_disk(dev->gd);
 }
 
 int ubiblock_remove(struct ubi_volume_info *vi)
@@ -478,11 +489,11 @@ int ubiblock_remove(struct ubi_volume_info *vi)
 
 	/* Remove from device list */
 	list_del(&dev->list);
-	ubiblock_cleanup(dev);
 	mutex_unlock(&dev->dev_mutex);
 	mutex_unlock(&devices_mutex);
 
-	kfree(dev);
+	ubiblock_cleanup(dev);
+
 	return 0;
 
 out_unlock_dev:
@@ -623,17 +634,19 @@ static void ubiblock_remove_all(void)
 {
 	struct ubiblock *next;
 	struct ubiblock *dev;
+	LIST_HEAD(list);
 
 	mutex_lock(&devices_mutex);
-	list_for_each_entry_safe(dev, next, &ubiblock_devices, list) {
+	list_splice_init(&ubiblock_devices, &list);
+	mutex_unlock(&devices_mutex);
+
+	list_for_each_entry_safe(dev, next, &list, list) {
 		/* The module is being forcefully removed */
 		WARN_ON(dev->desc);
 		/* Remove from device list */
 		list_del(&dev->list);
 		ubiblock_cleanup(dev);
-		kfree(dev);
 	}
-	mutex_unlock(&devices_mutex);
 }
 
 int __init ubiblock_init(void)

---
base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
change-id: 20230523-ubiblock-remove-eab61cf683f0

Best regards,
-- 
Vincent Whitchurch <vincent.whitchurch@...s.com>