Commit 33b235a6 authored by Namjae Jeon's avatar Namjae Jeon Committed by Steve French
Browse files

ksmbd: fix race condition between tree conn lookup and disconnect 



if thread A in smb2_write is using work-tcon, other thread B use
smb2_tree_disconnect free the tcon, then thread A will use free'd tcon.

                            Time
                             +
 Thread A                    | Thread A
 smb2_write                  | smb2_tree_disconnect
                             |
                             |
                             |   kfree(tree_conn)
                             |
  // UAF!                    |
  work->tcon->share_conf     |
                             +

This patch add state, reference count and lock for tree conn to fix race
condition issue.

Reported-by: default avatarluosili <rootlab@huawei.com>
Signed-off-by: default avatarNamjae Jeon <linkinjeon@kernel.org>
Signed-off-by: default avatarSteve French <stfrench@microsoft.com>
parent 75ac9a3d

fs/smb/server/mgmt/tree_connect.c

+39 −3
Original line number Diff line number Diff line
@@ -73,7 +73,10 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess, 


	tree_conn->user = sess->user;

	tree_conn->share_conf = sc;

	tree_conn->t_state = TREE_NEW;

	status.tree_conn = tree_conn;

	atomic_set(&tree_conn->refcount, 1);

	init_waitqueue_head(&tree_conn->refcount_q);



	ret = xa_err(xa_store(&sess->tree_conns, tree_conn->id, tree_conn,

			      GFP_KERNEL));
@@ -93,14 +96,33 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess, 
	return status;

}



void ksmbd_tree_connect_put(struct ksmbd_tree_connect *tcon)

{

	/*

	 * Checking waitqueue to releasing tree connect on

	 * tree disconnect. waitqueue_active is safe because it

	 * uses atomic operation for condition.

	 */

	if (!atomic_dec_return(&tcon->refcount) &&

	    waitqueue_active(&tcon->refcount_q))

		wake_up(&tcon->refcount_q);

}



int ksmbd_tree_conn_disconnect(struct ksmbd_session *sess,

			       struct ksmbd_tree_connect *tree_conn)

{

	int ret;



	write_lock(&sess->tree_conns_lock);

	xa_erase(&sess->tree_conns, tree_conn->id);

	write_unlock(&sess->tree_conns_lock);



	if (!atomic_dec_and_test(&tree_conn->refcount))

		wait_event(tree_conn->refcount_q,

			   atomic_read(&tree_conn->refcount) == 0);



	ret = ksmbd_ipc_tree_disconnect_request(sess->id, tree_conn->id);

	ksmbd_release_tree_conn_id(sess, tree_conn->id);

	xa_erase(&sess->tree_conns, tree_conn->id);

	ksmbd_share_config_put(tree_conn->share_conf);

	kfree(tree_conn);

	return ret;
@@ -111,11 +133,15 @@ struct ksmbd_tree_connect *ksmbd_tree_conn_lookup(struct ksmbd_session *sess, 
{

	struct ksmbd_tree_connect *tcon;



	read_lock(&sess->tree_conns_lock);

	tcon = xa_load(&sess->tree_conns, id);

	if (tcon) {

		if (test_bit(TREE_CONN_EXPIRE, &tcon->status))

		if (tcon->t_state != TREE_CONNECTED)

			tcon = NULL;

		else if (!atomic_inc_not_zero(&tcon->refcount))

			tcon = NULL;

	}

	read_unlock(&sess->tree_conns_lock);



	return tcon;

}
@@ -129,8 +155,18 @@ int ksmbd_tree_conn_session_logoff(struct ksmbd_session *sess) 
	if (!sess)

		return -EINVAL;



	xa_for_each(&sess->tree_conns, id, tc)

	xa_for_each(&sess->tree_conns, id, tc) {

		write_lock(&sess->tree_conns_lock);

		if (tc->t_state == TREE_DISCONNECTED) {

			write_unlock(&sess->tree_conns_lock);

			ret = -ENOENT;

			continue;

		}

		tc->t_state = TREE_DISCONNECTED;

		write_unlock(&sess->tree_conns_lock);



		ret |= ksmbd_tree_conn_disconnect(sess, tc);

	}

	xa_destroy(&sess->tree_conns);

	return ret;

}

fs/smb/server/mgmt/tree_connect.h

+9 −2
Original line number Diff line number Diff line
@@ -14,7 +14,11 @@ struct ksmbd_share_config; 
struct ksmbd_user;

struct ksmbd_conn;



#define TREE_CONN_EXPIRE		1

enum {

	TREE_NEW = 0,

	TREE_CONNECTED,

	TREE_DISCONNECTED

};



struct ksmbd_tree_connect {

	int				id;
@@ -27,7 +31,9 @@ struct ksmbd_tree_connect { 


	int				maximal_access;

	bool				posix_extensions;

	unsigned long			status;

	atomic_t			refcount;

	wait_queue_head_t		refcount_q;

	unsigned int			t_state;

};



struct ksmbd_tree_conn_status {
@@ -46,6 +52,7 @@ struct ksmbd_session; 
struct ksmbd_tree_conn_status

ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess,

			const char *share_name);

void ksmbd_tree_connect_put(struct ksmbd_tree_connect *tcon);



int ksmbd_tree_conn_disconnect(struct ksmbd_session *sess,

			       struct ksmbd_tree_connect *tree_conn);

fs/smb/server/mgmt/user_session.c

+1 −0
Original line number Diff line number Diff line
@@ -355,6 +355,7 @@ static struct ksmbd_session *__session_create(int protocol) 
	xa_init(&sess->ksmbd_chann_list);

	xa_init(&sess->rpc_handle_list);

	sess->sequence_number = 1;

	rwlock_init(&sess->tree_conns_lock);



	ret = __init_smb2_session(sess);

	if (ret)

fs/smb/server/mgmt/user_session.h

+1 −0
Original line number Diff line number Diff line
@@ -60,6 +60,7 @@ struct ksmbd_session { 


	struct ksmbd_file_table		file_table;

	unsigned long			last_active;

	rwlock_t			tree_conns_lock;

};



static inline int test_session_flag(struct ksmbd_session *sess, int bit)

fs/smb/server/server.c

+2 −0
Original line number Diff line number Diff line
@@ -241,6 +241,8 @@ static void __handle_ksmbd_work(struct ksmbd_work *work, 
	} while (is_chained == true);



send:

	if (work->tcon)

		ksmbd_tree_connect_put(work->tcon);

	smb3_preauth_hash_rsp(work);

	if (work->sess && work->sess->enc && work->encrypted &&

	    conn->ops->encrypt_resp) {