[v2] nbd: fix uaf in nbd_open

Commit Message

Li Lingfeng Nov. 7, 2023, 10:34 a.m. UTC

From: Li Lingfeng <[e-mail]>

Commit 4af5f2e03013 ("nbd: use blk_mq_alloc_disk and
blk_cleanup_disk") cleans up disk by blk_cleanup_disk() and it won't set
disk->private_data as NULL as before. UAF may be triggered in nbd_open()
if someone tries to open nbd device right after nbd_put() since nbd has
been free in nbd_dev_remove().

Fix this by implementing ->free_disk and free private data in it.

Fixes: 4af5f2e03013 ("nbd: use blk_mq_alloc_disk and blk_cleanup_disk")
Signed-off-by: Li Lingfeng <[e-mail]>
---
  v1->v2: Implemente ->free_disk and free nbd in it instead of setting
          disk->private_data as NULL before free nbd.

 drivers/block/nbd.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Josef Bacik Nov. 7, 2023, 3:05 p.m. UTC | #1

On Tue, Nov 07, 2023 at 06:34:35PM +0800, Li Lingfeng wrote:
> From: Li Lingfeng <[e-mail]>
> 
> Commit 4af5f2e03013 ("nbd: use blk_mq_alloc_disk and
> blk_cleanup_disk") cleans up disk by blk_cleanup_disk() and it won't set
> disk->private_data as NULL as before. UAF may be triggered in nbd_open()
> if someone tries to open nbd device right after nbd_put() since nbd has
> been free in nbd_dev_remove().
> 
> Fix this by implementing ->free_disk and free private data in it.
> 
> Fixes: 4af5f2e03013 ("nbd: use blk_mq_alloc_disk and blk_cleanup_disk")
> Signed-off-by: Li Lingfeng <[e-mail]>

Reviewed-by: Josef Bacik <[e-mail]>

Thanks,

Josef

Jens Axboe Nov. 7, 2023, 3:15 p.m. UTC | #2

On Tue, 07 Nov 2023 18:34:35 +0800, Li Lingfeng wrote:
> Commit 4af5f2e03013 ("nbd: use blk_mq_alloc_disk and
> blk_cleanup_disk") cleans up disk by blk_cleanup_disk() and it won't set
> disk->private_data as NULL as before. UAF may be triggered in nbd_open()
> if someone tries to open nbd device right after nbd_put() since nbd has
> been free in nbd_dev_remove().
> 
> Fix this by implementing ->free_disk and free private data in it.
> 
> [...]

Applied, thanks!

[1/1] nbd: fix uaf in nbd_open
      commit: 327462725b0f759f093788dfbcb2f1fd132f956b

Best regards,

Christoph Hellwig Nov. 8, 2023, 7:19 a.m. UTC | #3

I don't think this is actually enough on it's own.  You'll also
need to move al the teardown logic that is currently in
nbd_dev_remove.  And with this you should be able to remove the
NULL check in nbd_open, and propably the nbd_index_mutex critical
section.  Although that'll need a very careful audit.

Patch

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 800f131222fc..855fdf5c3b4e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -250,7 +250,6 @@  static void nbd_dev_remove(struct nbd_device *nbd)
 	struct gendisk *disk = nbd->disk;
 
 	del_gendisk(disk);
-	put_disk(disk);
 	blk_mq_free_tag_set(&nbd->tag_set);
 
 	/*
@@ -261,7 +260,7 @@  static void nbd_dev_remove(struct nbd_device *nbd)
 	idr_remove(&nbd_index_idr, nbd->index);
 	mutex_unlock(&nbd_index_mutex);
 	destroy_workqueue(nbd->recv_workq);
-	kfree(nbd);
+	put_disk(disk);
 }
 
 static void nbd_dev_remove_work(struct work_struct *work)
@@ -1608,6 +1607,13 @@  static void nbd_release(struct gendisk *disk)
 	nbd_put(nbd);
 }
 
+static void nbd_free_disk(struct gendisk *disk)
+{
+	struct nbd_device *nbd = disk->private_data;
+
+	kfree(nbd);
+}
+
 static const struct block_device_operations nbd_fops =
 {
 	.owner =	THIS_MODULE,
@@ -1615,6 +1621,7 @@  static const struct block_device_operations nbd_fops =
 	.release =	nbd_release,
 	.ioctl =	nbd_ioctl,
 	.compat_ioctl =	nbd_ioctl,
+	.free_disk =	nbd_free_disk,
 };
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)