ksmbd: fix use-after-free in ksmbd_tree_connect_put under concurrency

Under high concurrency, A tree-connection object (tcon) is freed on
a disconnect path while another path still holds a reference and later
executes *_put()/write on it.

Reported-by: Qianchang Zhao <pioooooooooip@gmail.com>
Reported-by: Zhitong Liu <liuzhitong1993@gmail.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 ecfc5750867124828c72d04624bc9d52a5d1d395..d3483d9c757c79b4d57450ed6f74fe5c73226100 100644 (file)
--- a/fs/smb/server/mgmt/tree_connect.c
+++ b/fs/smb/server/mgmt/tree_connect.c
@@ -78,7 +78,6 @@ ksmbd_tree_conn_connect(struct ksmbd_work *work, const char *share_name)
        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,
                              KSMBD_DEFAULT_GFP));
@@ -100,14 +99,8 @@ out_error:
 
 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);
+       if (atomic_dec_and_test(&tcon->refcount))
+               kfree(tcon);
 }
 
 int ksmbd_tree_conn_disconnect(struct ksmbd_session *sess,
@@ -119,14 +112,11 @@ int ksmbd_tree_conn_disconnect(struct ksmbd_session *sess,
        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);
        ksmbd_share_config_put(tree_conn->share_conf);
-       kfree(tree_conn);
+       if (atomic_dec_and_test(&tree_conn->refcount))
+               kfree(tree_conn);
        return ret;
 }
 

diff --git a/fs/smb/server/mgmt/tree_connect.h b/fs/smb/server/mgmt/tree_connect.h
index a42cdd0510411437fe074ee3359a648496a36b13..f0023d86716f2525572734eb049640dceefa6ae9 100644 (file)
--- a/fs/smb/server/mgmt/tree_connect.h
+++ b/fs/smb/server/mgmt/tree_connect.h
@@ -33,7 +33,6 @@ struct ksmbd_tree_connect {
        int                             maximal_access;
        bool                            posix_extensions;
        atomic_t                        refcount;
-       wait_queue_head_t               refcount_q;
        unsigned int                    t_state;
 };
 

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index c2369415c6c4528b3eeba0297829d999f743010a..c3eedc70e6430afa56e79b50745fec130f435e8b 100644 (file)
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -2190,7 +2190,6 @@ int smb2_tree_disconnect(struct ksmbd_work *work)
                goto err_out;
        }
 
-       WARN_ON_ONCE(atomic_dec_and_test(&tcon->refcount));
        tcon->t_state = TREE_DISCONNECTED;
        write_unlock(&sess->tree_conns_lock);
 
@@ -2200,8 +2199,6 @@ int smb2_tree_disconnect(struct ksmbd_work *work)
                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));