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: luosili <rootlab@huawei.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>

diff --git a/fs/smb/server/mgmt/tree_connect.c b/fs/smb/server/mgmt/tree_connect.c
index 408cddf2f094a87a0b5ffce7855b08acd886a68d..d2c81a8a11dda10ef710bc97e1c059073fddb700 100644 (file)
--- a/fs/smb/server/mgmt/tree_connect.c
+++ b/fs/smb/server/mgmt/tree_connect.c
@@ -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 @@ out_error:
        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;
 }

diff --git a/fs/smb/server/mgmt/tree_connect.h b/fs/smb/server/mgmt/tree_connect.h
index 562d647ad9fad46e49e6746f396bce223a73c742..6377a70b811c8993575d96889a8c2ef5f2fada2c 100644 (file)
--- a/fs/smb/server/mgmt/tree_connect.h
+++ b/fs/smb/server/mgmt/tree_connect.h
@@ -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);

diff --git a/fs/smb/server/mgmt/user_session.c b/fs/smb/server/mgmt/user_session.c
index b8be14a96cf66f19cc875d9d779af158ee4e12f4..15f68ee0508946843983b7d773dc6837f12b3bf0 100644 (file)
--- a/fs/smb/server/mgmt/user_session.c
+++ b/fs/smb/server/mgmt/user_session.c
@@ -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)

diff --git a/fs/smb/server/mgmt/user_session.h b/fs/smb/server/mgmt/user_session.h
index f99d475b28db45f96ae559c7368220a5dcd1a69e..63cb08fffde84c4a5f03b936577fe02288885b6e 100644 (file)
--- a/fs/smb/server/mgmt/user_session.h
+++ b/fs/smb/server/mgmt/user_session.h
@@ -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)

diff --git a/fs/smb/server/server.c b/fs/smb/server/server.c
index 32347fec33c4e60e0ada2a366befaad2c85d3ce0..3079e607c5fe6db71e3cc70a76bcb26d90f813c3 100644 (file)
--- a/fs/smb/server/server.c
+++ b/fs/smb/server/server.c
@@ -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) {

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index fd6f05786ac2a61cc4863c27d4b62bed01bbf83e..898860adf9290d51cafcffb02ea4f12f20ed7805 100644 (file)
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -1993,6 +1993,9 @@ int smb2_tree_connect(struct ksmbd_work *work)
        if (conn->posix_ext_supported)
                status.tree_conn->posix_extensions = true;
 
+       write_lock(&sess->tree_conns_lock);
+       status.tree_conn->t_state = TREE_CONNECTED;
+       write_unlock(&sess->tree_conns_lock);
        rsp->StructureSize = cpu_to_le16(16);
 out_err1:
        rsp->Capabilities = 0;
@@ -2122,27 +2125,50 @@ int smb2_tree_disconnect(struct ksmbd_work *work)
 
        ksmbd_debug(SMB, "request\n");
 
+       if (!tcon) {
+               ksmbd_debug(SMB, "Invalid tid %d\n", req->hdr.Id.SyncId.TreeId);
+
+               rsp->hdr.Status = STATUS_NETWORK_NAME_DELETED;
+               err = -ENOENT;
+               goto err_out;
+       }
+
+       ksmbd_close_tree_conn_fds(work);
+
+       write_lock(&sess->tree_conns_lock);
+       if (tcon->t_state == TREE_DISCONNECTED) {
+               write_unlock(&sess->tree_conns_lock);
+               rsp->hdr.Status = STATUS_NETWORK_NAME_DELETED;
+               err = -ENOENT;
+               goto err_out;
+       }
+
+       WARN_ON_ONCE(atomic_dec_and_test(&tcon->refcount));
+       tcon->t_state = TREE_DISCONNECTED;
+       write_unlock(&sess->tree_conns_lock);
+
+       err = ksmbd_tree_conn_disconnect(sess, tcon);
+       if (err) {
+               rsp->hdr.Status = STATUS_NETWORK_NAME_DELETED;
+               goto err_out;
+       }
+
+       work->tcon = NULL;
+
        rsp->StructureSize = cpu_to_le16(4);
        err = ksmbd_iov_pin_rsp(work, rsp,
                                sizeof(struct smb2_tree_disconnect_rsp));
        if (err) {
                rsp->hdr.Status = STATUS_INSUFFICIENT_RESOURCES;
-               smb2_set_err_rsp(work);
-               return err;
+               goto err_out;
        }
 
-       if (!tcon || test_and_set_bit(TREE_CONN_EXPIRE, &tcon->status)) {
-               ksmbd_debug(SMB, "Invalid tid %d\n", req->hdr.Id.SyncId.TreeId);
+       return 0;
 
-               rsp->hdr.Status = STATUS_NETWORK_NAME_DELETED;
-               smb2_set_err_rsp(work);
-               return -ENOENT;
-       }
+err_out:
+       smb2_set_err_rsp(work);
+       return err;
 
-       ksmbd_close_tree_conn_fds(work);
-       ksmbd_tree_conn_disconnect(sess, tcon);
-       work->tcon = NULL;
-       return 0;
 }
 
 /**