diff mbox series

[2/6] ksmbd: fix wrong UserName check in session_user

Message ID 20230505151108.5911-2-linkinjeon@kernel.org (mailing list archive)
State New, archived
Headers show
Series [1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals | expand

Commit Message

Namjae Jeon May 5, 2023, 3:11 p.m. UTC 
From: Pumpkin <cc85nod@gmail.com>

The offset of UserName is related to the address of security
buffer. To ensure the validaty of UserName, we need to compare name_off
+ name_len with secbuf_len instead of auth_msg_len.

[   27.096243] ==================================================================
[   27.096890] BUG: KASAN: slab-out-of-bounds in smb_strndup_from_utf16+0x188/0x350
[   27.097609] Read of size 2 at addr ffff888005e3b542 by task kworker/0:0/7
...
[   27.099950] Call Trace:
[   27.100194]  <TASK>
[   27.100397]  dump_stack_lvl+0x33/0x50
[   27.100752]  print_report+0xcc/0x620
[   27.102305]  kasan_report+0xae/0xe0
[   27.103072]  kasan_check_range+0x35/0x1b0
[   27.103757]  smb_strndup_from_utf16+0x188/0x350
[   27.105474]  smb2_sess_setup+0xaf8/0x19c0
[   27.107935]  handle_ksmbd_work+0x274/0x810
[   27.108315]  process_one_work+0x419/0x760
[   27.108689]  worker_thread+0x2a2/0x6f0
[   27.109385]  kthread+0x160/0x190
[   27.110129]  ret_from_fork+0x1f/0x30
[   27.110454]  </TASK>

Signed-off-by: Pumpkin <cc85nod@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2pdu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Sergey Senozhatsky May 6, 2023, 3:10 a.m. UTC | #1 
On (23/05/06 00:11), Namjae Jeon wrote:
> The offset of UserName is related to the address of security
> buffer. To ensure the validaty of UserName, we need to compare name_off
> + name_len with secbuf_len instead of auth_msg_len.
> 
> [   27.096243] ==================================================================
> [   27.096890] BUG: KASAN: slab-out-of-bounds in smb_strndup_from_utf16+0x188/0x350
> [   27.097609] Read of size 2 at addr ffff888005e3b542 by task kworker/0:0/7
> ...
> [   27.099950] Call Trace:
> [   27.100194]  <TASK>
> [   27.100397]  dump_stack_lvl+0x33/0x50
> [   27.100752]  print_report+0xcc/0x620
> [   27.102305]  kasan_report+0xae/0xe0
> [   27.103072]  kasan_check_range+0x35/0x1b0
> [   27.103757]  smb_strndup_from_utf16+0x188/0x350
> [   27.105474]  smb2_sess_setup+0xaf8/0x19c0
> [   27.107935]  handle_ksmbd_work+0x274/0x810
> [   27.108315]  process_one_work+0x419/0x760
> [   27.108689]  worker_thread+0x2a2/0x6f0
> [   27.109385]  kthread+0x160/0x190
> [   27.110129]  ret_from_fork+0x1f/0x30
> [   27.110454]  </TASK>
> 
> Signed-off-by: Pumpkin <cc85nod@gmail.com>

Do we still have a requirement that there should be a real name in SoB?
Namjae Jeon May 7, 2023, 12:52 a.m. UTC | #2 
2023-05-06 12:10 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>:
> On (23/05/06 00:11), Namjae Jeon wrote:
>> The offset of UserName is related to the address of security
>> buffer. To ensure the validaty of UserName, we need to compare name_off
>> + name_len with secbuf_len instead of auth_msg_len.
>>
>> [   27.096243]
>> ==================================================================
>> [   27.096890] BUG: KASAN: slab-out-of-bounds in
>> smb_strndup_from_utf16+0x188/0x350
>> [   27.097609] Read of size 2 at addr ffff888005e3b542 by task
>> kworker/0:0/7
>> ...
>> [   27.099950] Call Trace:
>> [   27.100194]  <TASK>
>> [   27.100397]  dump_stack_lvl+0x33/0x50
>> [   27.100752]  print_report+0xcc/0x620
>> [   27.102305]  kasan_report+0xae/0xe0
>> [   27.103072]  kasan_check_range+0x35/0x1b0
>> [   27.103757]  smb_strndup_from_utf16+0x188/0x350
>> [   27.105474]  smb2_sess_setup+0xaf8/0x19c0
>> [   27.107935]  handle_ksmbd_work+0x274/0x810
>> [   27.108315]  process_one_work+0x419/0x760
>> [   27.108689]  worker_thread+0x2a2/0x6f0
>> [   27.109385]  kthread+0x160/0x190
>> [   27.110129]  ret_from_fork+0x1f/0x30
>> [   27.110454]  </TASK>
>>
>> Signed-off-by: Pumpkin <cc85nod@gmail.com>
>
> Do we still have a requirement that there should be a real name in SoB?
Hi 張智諺,

I will update SoB if you tell me your english real name.

Thanks.
>
diff mbox series

Patch

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index cb93fd231f4e..8de8afd473ae 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -1356,7 +1356,7 @@  static struct ksmbd_user *session_user(struct ksmbd_conn *conn,
 	struct authenticate_message *authblob;
 	struct ksmbd_user *user;
 	char *name;
-	unsigned int auth_msg_len, name_off, name_len, secbuf_len;
+	unsigned int name_off, name_len, secbuf_len;
 
 	secbuf_len = le16_to_cpu(req->SecurityBufferLength);
 	if (secbuf_len < sizeof(struct authenticate_message)) {
@@ -1366,9 +1366,8 @@  static struct ksmbd_user *session_user(struct ksmbd_conn *conn,
 	authblob = user_authblob(conn, req);
 	name_off = le32_to_cpu(authblob->UserName.BufferOffset);
 	name_len = le16_to_cpu(authblob->UserName.Length);
-	auth_msg_len = le16_to_cpu(req->SecurityBufferOffset) + secbuf_len;
 
-	if (auth_msg_len < (u64)name_off + name_len)
+	if (secbuf_len < (u64)name_off + name_len)
 		return NULL;
 
 	name = smb_strndup_from_utf16((const char *)authblob + name_off,