Skip to content

Commit

Permalink
ksmbd: fix race condition between tree conn lookup and disconnect
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Signed-off-by: Namjae Jeon <[email protected]>
  • Loading branch information
@namjaejeon
namjaejeon committed Oct 5, 2023
1 parent 019d0b4 commit 9c5dca6
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 17 deletions.
42 changes: 39 additions & 3 deletions mgmt/tree_connect.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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;
Expand All @@ -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;
}
Expand All @@ -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;
}
11 changes: 9 additions & 2 deletions mgmt/tree_connect.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand All @@ -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);
Expand Down
1 change: 1 addition & 0 deletions mgmt/user_session.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,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);

switch (protocol) {
#ifdef CONFIG_SMB_INSECURE_SERVER
Expand Down
1 change: 1 addition & 0 deletions mgmt/user_session.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,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)
Expand Down
2 changes: 2 additions & 0 deletions server.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
50 changes: 38 additions & 12 deletions smb2pdu.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2010,6 +2010,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;
Expand Down Expand Up @@ -2139,27 +2142,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;
}

/**
Expand Down

0 comments on commit 9c5dca6

Please sign in to comment.