[-next,v2] nbd: get config_lock before sock_shutdown

Commit Message

Li Lingfeng Dec. 5, 2023, 12:56 p.m. UTC

From: Zhong Jinghua <[e-mail]>

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/[e-mail]/
Signed-off-by: Zhong Jinghua <[e-mail]>
Signed-off-by: Li Lingfeng <[e-mail]>
---
v1->v2:
  Make comment more detailed.

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

Patch

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))