[PATCH 1/3] ksmbd: fix uninitialized locks in __session_create() error path

[PATCH 1/3] ksmbd: fix uninitialized locks in __session_create() error path

[Dmitriy Okunev]

In __session_create(), the ksmbd_init_file_table() call is
made before the tree_conns_lock, rpc_lock and chann_lock rw_semaphores
are initialized with init_rwsem().  If ksmbd_init_file_table() fails
(which can happen on -ENOMEM), the error path calls
ksmbd_session_destroy(), which in turn calls
ksmbd_tree_conn_session_logoff(), ksmbd_session_rpc_clear_list() and
free_channel_list(). All three functions call down_write() with
uninitialized rw_semaphore.

INFO: trying to register non-static key.
  ...
  down_write+0x7d/0x1c0
  ksmbd_tree_conn_session_logoff+0xa2/0x1f0
  ksmbd_session_destroy+0xaf/0x380
  ksmbd_smb2_session_create+0xdb/0x110
  ...

Fix it by moving the three init_rwsem() calls above
ksmbd_init_file_table(), so that all locks are valid on every error
path.

Found by Linux Verification Center (linuxtesting.org).

Fixes: a1f46c99d9ea ("ksmbd: fix use-after-free in ksmbd_session_rpc_open")
Signed-off-by: Dmitriy Okunev <dokunevdmitriy@gmail.com>
---
 fs/smb/server/mgmt/user_session.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/smb/server/mgmt/user_session.c b/fs/smb/server/mgmt/user_session.c
index 44dc3f800cd4..fc28088772ac 100644
--- a/fs/smb/server/mgmt/user_session.c
+++ b/fs/smb/server/mgmt/user_session.c
@@ -844,6 +844,10 @@ static struct ksmbd_session *__session_create(int protocol)
 
 	ida_init(&sess->tree_conn_ida);
 
+	init_rwsem(&sess->tree_conns_lock);
+	init_rwsem(&sess->rpc_lock);
+	init_rwsem(&sess->chann_lock);
+
 	if (ksmbd_init_file_table(&sess->file_table))
 		goto error;
 
@@ -855,9 +859,6 @@ static struct ksmbd_session *__session_create(int protocol)
 	xa_init(&sess->rpc_handle_list);
 	sess->sequence_number = 1;
 	atomic_set(&sess->refcnt, 2);
-	init_rwsem(&sess->tree_conns_lock);
-	init_rwsem(&sess->rpc_lock);
-	init_rwsem(&sess->chann_lock);
 
 	ret = __init_smb2_session(sess);
 	if (ret)
-- 
2.53.0

[PATCH 2/3] ksmbd: fix uninitialized xarrays in __session_create() error path

[Dmitriy Okunev]

In __session_create(), the tree_conns, ksmbd_chann_list and
rpc_handle_list xarrays are initialized with xa_init() after the
ksmbd_init_file_table() call. If ksmbd_init_file_table() fails, the
error path calls ksmbd_session_destroy(), which in turn calls
ksmbd_tree_conn_session_logoff().  That function runs
xa_destroy(&sess->tree_conns), which tries to take the xarray's
internal spinlock - uninitialized because xa_init() has not run yet:

  INFO: trying to register non-static key.
  ...
  _raw_spin_lock_irqsave+0x3a/0x60
  xa_destroy+0xbe/0x360
  ksmbd_tree_conn_session_logoff+0x174/0x1f0
  ksmbd_session_destroy+0xaf/0x390
  ksmbd_smb2_session_create+0x131/0x160
  smb2_sess_setup+0xde4/0x6fd0
  ...

Move the three xa_init() calls above ksmbd_init_file_table(), the same
way the rwsem initialization was moved, so that all xarrays are
valid on every error path.

Found by Linux Verification Center (linuxtesting.org).

Fixes: 02b68b2065c9 ("cifsd: use xarray instead of linked list for tree connect list")
Signed-off-by: Dmitriy Okunev <dokunevdmitriy@gmail.com>
---
 fs/smb/server/mgmt/user_session.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/smb/server/mgmt/user_session.c b/fs/smb/server/mgmt/user_session.c
index fc28088772ac..386c8629e134 100644
--- a/fs/smb/server/mgmt/user_session.c
+++ b/fs/smb/server/mgmt/user_session.c
@@ -844,6 +844,9 @@ static struct ksmbd_session *__session_create(int protocol)
 
 	ida_init(&sess->tree_conn_ida);
 
+	xa_init(&sess->tree_conns);
+	xa_init(&sess->ksmbd_chann_list);
+	xa_init(&sess->rpc_handle_list);
 	init_rwsem(&sess->tree_conns_lock);
 	init_rwsem(&sess->rpc_lock);
 	init_rwsem(&sess->chann_lock);
@@ -854,9 +857,6 @@ static struct ksmbd_session *__session_create(int protocol)
 	sess->last_active = jiffies;
 	sess->state = SMB2_SESSION_IN_PROGRESS;
 	set_session_flag(sess, protocol);
-	xa_init(&sess->tree_conns);
-	xa_init(&sess->ksmbd_chann_list);
-	xa_init(&sess->rpc_handle_list);
 	sess->sequence_number = 1;
 	atomic_set(&sess->refcnt, 2);
 
-- 
2.53.0

[PATCH 3/3] ksmbd: do not release session id 0 on init failure

[Dmitriy Okunev]

When __session_create() fails before __init_smb2_session() assigns an
id, sess->id remains 0 because the session was allocated with kzalloc.
ksmbd_session_destroy() then calls ksmbd_release_id(&session_ida,
sess->id), and since ida_alloc_min() has not yet been called,
ida_free(ida, 0) triggers:

  ida_free called for id=0 which is not allocated.
  WARNING: lib/idr.c:594 at ida_free+0x18c/0x250
  ...
  ksmbd_release_id+0xd/0x10
  ksmbd_session_destroy+0x2d5/0x380
  ksmbd_smb2_session_create+0x131/0x160
  ...

Add a check for sess->id > 0  before  the release,
since ids are allocated starting from 1
(ida_alloc_min(ida,1,...)).

Found by Linux Verification Center (linuxtesting.org).

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Signed-off-by: Dmitriy Okunev <dokunevdmitriy@gmail.com>
---
 fs/smb/server/mgmt/user_session.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/smb/server/mgmt/user_session.c b/fs/smb/server/mgmt/user_session.c
index 386c8629e134..d46dc7b2c562 100644
--- a/fs/smb/server/mgmt/user_session.c
+++ b/fs/smb/server/mgmt/user_session.c
@@ -409,7 +409,8 @@ void ksmbd_session_destroy(struct ksmbd_session *sess)
 	ksmbd_session_rpc_clear_list(sess);
 	free_channel_list(sess);
 	kfree_sensitive(sess->Preauth_HashValue);
-	ksmbd_release_id(&session_ida, sess->id);
+	if (sess->id > 0)
+		ksmbd_release_id(&session_ida, sess->id);
 	ida_destroy(&sess->tree_conn_ida);
 	kfree_sensitive(sess);
 }
-- 
2.53.0