Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
From:   Li Lingfeng <lilingfeng@huaweicloud.com>
To:     josef@toxicpanda.com
Cc:     linux-kernel@vger.kernel.org, hch@lst.de,
        linux-block@vger.kernel.org, nbd@other.debian.org, axboe@kernel.dk,
        chaitanya.kulkarni@wdc.com, yukuai1@huaweicloud.com,
        houtao1@huawei.com, yi.zhang@huawei.com, yangerkun@huawei.com,
        lilingfeng@huaweicloud.com, lilingfeng3@huawei.com
Subject: [PATCH -next v2] nbd: get config_lock before sock_shutdown
Date:   Tue,  5 Dec 2023 20:56:41 +0800
Message-Id: <20231205125641.1913393-1-lilingfeng@huaweicloud.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Zhong Jinghua <zhongjinghua@huawei.com>

Config->socks in sock_shutdown may trigger a UAF problem.
The reason is that sock_shutdown does not hold the config_lock,
so that nbd_ioctl can release config->socks at this time.

T0: NBD_DO_IT
T1: NBD_SET_SOCK

T0						T1

nbd_ioctl
  mutex_lock(&nbd->config_lock)
  // get lock
  __nbd_ioctl
	nbd_start_device_ioctl
	  nbd_start_device
	  mutex_unlock(&nbd->config_lock)
	  // relase lock
	  wait_event_interruptible
	  (kill, enter sock_shutdown)
	  sock_shutdown
					nbd_ioctl
					  mutex_lock(&nbd->config_lock)
					  // get lock
					  __nbd_ioctl
					    nbd_add_socket
					      krealloc
						kfree(p)
					        //config->socks is NULL
	    nbd_sock *nsock = config->socks // error

Fix it by moving config_lock up before sock_shutdown.

Link: https://lore.kernel.org/all/ab998dda-80ba-7d8b-0cae-36665826deb5@huaweicloud.com/
Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com>
Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
---
v1->v2:
  Make comment more detailed.

 drivers/block/nbd.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 855fdf5c3b4e..7a044b4726b4 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -92,6 +92,11 @@ struct nbd_config {
 	unsigned long runtime_flags;
 	u64 dead_conn_timeout;
 
+	/*
+	 * Anyone who tries to get config->socks needs to be
+	 * protected by config_lock since it may be released
+	 * by krealloc in nbd_add_socket.
+	 */
 	struct nbd_sock **socks;
 	int num_connections;
 	atomic_t live_connections;
@@ -876,6 +881,10 @@ static void recv_work(struct work_struct *work)
 	nbd_mark_nsock_dead(nbd, nsock, 1);
 	mutex_unlock(&nsock->tx_lock);
 
+	/*
+	 * recv_work will not get config_lock here if recv_workq is flushed
+	 * in ioctl since nbd_open is holding config_refs.
+	 */
 	nbd_config_put(nbd);
 	atomic_dec(&config->recv_threads);
 	wake_up(&config->recv_wq);
@@ -1417,13 +1426,21 @@ static int nbd_start_device_ioctl(struct nbd_device *nbd)
 	mutex_unlock(&nbd->config_lock);
 	ret = wait_event_interruptible(config->recv_wq,
 					 atomic_read(&config->recv_threads) == 0);
+
+	/*
+	 * Get config_lock before sock_shutdown to prevent UAF since nbd_add_socket
+	 * may release config->socks concurrently.
+	 *
+	 * config_lock can be got before flush_workqueue since recv_work will not
+	 * get it in the current scenario.
+	 */
+	mutex_lock(&nbd->config_lock);
 	if (ret) {
 		sock_shutdown(nbd);
 		nbd_clear_que(nbd);
 	}
 
 	flush_workqueue(nbd->recv_workq);
-	mutex_lock(&nbd->config_lock);
 	nbd_bdev_reset(nbd);
 	/* user requested, ignore socket errors */
 	if (test_bit(NBD_RT_DISCONNECT_REQUESTED, &config->runtime_flags))
-- 
2.39.2