linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals
@ 2023-05-05 15:11 Namjae Jeon
  2023-05-05 15:11 ` [PATCH 2/6] ksmbd: fix wrong UserName check in session_user Namjae Jeon
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Namjae Jeon @ 2023-05-05 15:11 UTC (permalink / raw)
  To: linux-cifs
  Cc: smfrench, senozhatsky, tom, atteh.mailbox, Namjae Jeon, Pumpkin

From: Pumpkin <cc85nod@gmail.com>

If the length of CreateContext name is larger than the tag, it will access
the data following the tag and trigger KASAN global-out-of-bounds.

Currently all CreateContext names are defined as string, so we can use
strcmp instead of memcmp to avoid the out-of-bound access.

[    7.995411] ==================================================================
[    7.995866] BUG: KASAN: global-out-of-bounds in memcmp+0x83/0xa0
[    7.996248] Read of size 8 at addr ffffffff8258d940 by task kworker/0:0/7
...
[    7.998191] Call Trace:
[    7.998358]  <TASK>
[    7.998503]  dump_stack_lvl+0x33/0x50
[    7.998743]  print_report+0xcc/0x620
[    7.999458]  kasan_report+0xae/0xe0
[    7.999895]  kasan_check_range+0x35/0x1b0
[    8.000152]  memcmp+0x83/0xa0
[    8.000347]  smb2_find_context_vals+0xf7/0x1e0
[    8.000635]  smb2_open+0x1df2/0x43a0
[    8.006398]  handle_ksmbd_work+0x274/0x810
[    8.006666]  process_one_work+0x419/0x760
[    8.006922]  worker_thread+0x2a2/0x6f0
[    8.007429]  kthread+0x160/0x190
[    8.007946]  ret_from_fork+0x1f/0x30
[    8.008181]  </TASK>

Signed-off-by: Pumpkin <cc85nod@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/oplock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
index 2e54ded4d92c..5e09834016bb 100644
--- a/fs/ksmbd/oplock.c
+++ b/fs/ksmbd/oplock.c
@@ -1492,7 +1492,7 @@ struct create_context *smb2_find_context_vals(void *open_req, const char *tag)
 			return ERR_PTR(-EINVAL);
 
 		name = (char *)cc + name_off;
-		if (memcmp(name, tag, name_len) == 0)
+		if (!strcmp(name, tag))
 			return cc;
 
 		remain_len -= next;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/6] ksmbd: fix wrong UserName check in session_user
  2023-05-05 15:11 [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Namjae Jeon
@ 2023-05-05 15:11 ` Namjae Jeon
  2023-05-06  3:10   ` Sergey Senozhatsky
  2023-05-05 15:11 ` [PATCH 3/6] ksmbd: allocate one more byte for implied bcc[0] Namjae Jeon
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Namjae Jeon @ 2023-05-05 15:11 UTC (permalink / raw)
  To: linux-cifs
  Cc: smfrench, senozhatsky, tom, atteh.mailbox, Namjae Jeon, Pumpkin

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(-)

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,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/6] ksmbd: allocate one more byte for implied bcc[0]
  2023-05-05 15:11 [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Namjae Jeon
  2023-05-05 15:11 ` [PATCH 2/6] ksmbd: fix wrong UserName check in session_user Namjae Jeon
@ 2023-05-05 15:11 ` Namjae Jeon
  2023-05-05 15:11 ` [PATCH 4/6] ksmbd: smb2: Allow messages padded to 8byte boundary Namjae Jeon
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Namjae Jeon @ 2023-05-05 15:11 UTC (permalink / raw)
  To: linux-cifs
  Cc: smfrench, senozhatsky, tom, atteh.mailbox, Namjae Jeon, Pumpkin

From: Pumpkin <cc85nod@gmail.com>

ksmbd_smb2_check_message allows client to return one byte more, so we
need to allocate additional memory in ksmbd_conn_handler_loop to avoid
out-of-bound access.

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

diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
index 4ed379f9b1aa..4882a812ea86 100644
--- a/fs/ksmbd/connection.c
+++ b/fs/ksmbd/connection.c
@@ -351,7 +351,8 @@ int ksmbd_conn_handler_loop(void *p)
 			break;
 
 		/* 4 for rfc1002 length field */
-		size = pdu_size + 4;
+		/* 1 for implied bcc[0] */
+		size = pdu_size + 4 + 1;
 		conn->request_buf = kvmalloc(size, GFP_KERNEL);
 		if (!conn->request_buf)
 			break;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/6] ksmbd: smb2: Allow messages padded to 8byte boundary
  2023-05-05 15:11 [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Namjae Jeon
  2023-05-05 15:11 ` [PATCH 2/6] ksmbd: fix wrong UserName check in session_user Namjae Jeon
  2023-05-05 15:11 ` [PATCH 3/6] ksmbd: allocate one more byte for implied bcc[0] Namjae Jeon
@ 2023-05-05 15:11 ` Namjae Jeon
  2023-05-05 15:11 ` [PATCH 5/6] ksmbd: remove unused ksmbd_tree_conn_share function Namjae Jeon
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Namjae Jeon @ 2023-05-05 15:11 UTC (permalink / raw)
  To: linux-cifs
  Cc: smfrench, senozhatsky, tom, atteh.mailbox, Namjae Jeon, Gustav Johansson

From: Gustav Johansson <gustajo@axis.com>

clc length is now accepted to <= 8 less than length,
rather than < 8.

Solve issues on some of Axis's smb clients which send
messages where clc length is 8 bytes less than length.

The specific client was running kernel 4.19.217 with
smb dialect 3.0.2 on armv7l.

Signed-off-by: Gustav Johansson <gustajo@axis.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2misc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index fbdde426dd01..0ffe663b7590 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -416,8 +416,11 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
 
 		/*
 		 * Allow a message that padded to 8byte boundary.
+		 * Linux 4.19.217 with smb 3.0.2 are sometimes
+		 * sending messages where the cls_len is exactly
+		 * 8 bytes less than len.
 		 */
-		if (clc_len < len && (len - clc_len) < 8)
+		if (clc_len < len && (len - clc_len) <= 8)
 			goto validate_credit;
 
 		pr_err_ratelimited(
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/6] ksmbd: remove unused ksmbd_tree_conn_share function
  2023-05-05 15:11 [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Namjae Jeon
                   ` (2 preceding siblings ...)
  2023-05-05 15:11 ` [PATCH 4/6] ksmbd: smb2: Allow messages padded to 8byte boundary Namjae Jeon
@ 2023-05-05 15:11 ` Namjae Jeon
  2023-05-08  1:07   ` Sergey Senozhatsky
  2023-05-05 15:11 ` [PATCH 6/6] ksmbd: use kzalloc() instead of __GFP_ZERO Namjae Jeon
  2023-05-08  1:05 ` [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Sergey Senozhatsky
  5 siblings, 1 reply; 14+ messages in thread
From: Namjae Jeon @ 2023-05-05 15:11 UTC (permalink / raw)
  To: linux-cifs; +Cc: smfrench, senozhatsky, tom, atteh.mailbox, Namjae Jeon

Remove unused ksmbd_tree_conn_share function.

Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/mgmt/tree_connect.c | 11 -----------
 fs/ksmbd/mgmt/tree_connect.h |  3 ---
 2 files changed, 14 deletions(-)

diff --git a/fs/ksmbd/mgmt/tree_connect.c b/fs/ksmbd/mgmt/tree_connect.c
index f07a05f37651..408cddf2f094 100644
--- a/fs/ksmbd/mgmt/tree_connect.c
+++ b/fs/ksmbd/mgmt/tree_connect.c
@@ -120,17 +120,6 @@ struct ksmbd_tree_connect *ksmbd_tree_conn_lookup(struct ksmbd_session *sess,
 	return tcon;
 }
 
-struct ksmbd_share_config *ksmbd_tree_conn_share(struct ksmbd_session *sess,
-						 unsigned int id)
-{
-	struct ksmbd_tree_connect *tc;
-
-	tc = ksmbd_tree_conn_lookup(sess, id);
-	if (tc)
-		return tc->share_conf;
-	return NULL;
-}
-
 int ksmbd_tree_conn_session_logoff(struct ksmbd_session *sess)
 {
 	int ret = 0;
diff --git a/fs/ksmbd/mgmt/tree_connect.h b/fs/ksmbd/mgmt/tree_connect.h
index 700df36cf3e3..562d647ad9fa 100644
--- a/fs/ksmbd/mgmt/tree_connect.h
+++ b/fs/ksmbd/mgmt/tree_connect.h
@@ -53,9 +53,6 @@ int ksmbd_tree_conn_disconnect(struct ksmbd_session *sess,
 struct ksmbd_tree_connect *ksmbd_tree_conn_lookup(struct ksmbd_session *sess,
 						  unsigned int id);
 
-struct ksmbd_share_config *ksmbd_tree_conn_share(struct ksmbd_session *sess,
-						 unsigned int id);
-
 int ksmbd_tree_conn_session_logoff(struct ksmbd_session *sess);
 
 #endif /* __TREE_CONNECT_MANAGEMENT_H__ */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 6/6] ksmbd: use kzalloc() instead of __GFP_ZERO
  2023-05-05 15:11 [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Namjae Jeon
                   ` (3 preceding siblings ...)
  2023-05-05 15:11 ` [PATCH 5/6] ksmbd: remove unused ksmbd_tree_conn_share function Namjae Jeon
@ 2023-05-05 15:11 ` Namjae Jeon
  2023-05-08  1:06   ` Sergey Senozhatsky
  2023-05-08  1:05 ` [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Sergey Senozhatsky
  5 siblings, 1 reply; 14+ messages in thread
From: Namjae Jeon @ 2023-05-05 15:11 UTC (permalink / raw)
  To: linux-cifs
  Cc: smfrench, senozhatsky, tom, atteh.mailbox, Namjae Jeon, Dan Carpenter

Use kzalloc() instead of __GFP_ZERO.

Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index af0c2a9b8529..c6e4d38319df 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -347,8 +347,8 @@ static int smb1_check_user_session(struct ksmbd_work *work)
  */
 static int smb1_allocate_rsp_buf(struct ksmbd_work *work)
 {
-	work->response_buf = kmalloc(MAX_CIFS_SMALL_BUFFER_SIZE,
-			GFP_KERNEL | __GFP_ZERO);
+	work->response_buf = kzalloc(MAX_CIFS_SMALL_BUFFER_SIZE,
+			GFP_KERNEL);
 	work->response_sz = MAX_CIFS_SMALL_BUFFER_SIZE;
 
 	if (!work->response_buf) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/6] ksmbd: fix wrong UserName check in session_user
  2023-05-05 15:11 ` [PATCH 2/6] ksmbd: fix wrong UserName check in session_user Namjae Jeon
@ 2023-05-06  3:10   ` Sergey Senozhatsky
  2023-05-07  0:52     ` Namjae Jeon
  0 siblings, 1 reply; 14+ messages in thread
From: Sergey Senozhatsky @ 2023-05-06  3:10 UTC (permalink / raw)
  To: Namjae Jeon
  Cc: linux-cifs, smfrench, senozhatsky, tom, atteh.mailbox, Pumpkin

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?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/6] ksmbd: fix wrong UserName check in session_user
  2023-05-06  3:10   ` Sergey Senozhatsky
@ 2023-05-07  0:52     ` Namjae Jeon
  0 siblings, 0 replies; 14+ messages in thread
From: Namjae Jeon @ 2023-05-07  0:52 UTC (permalink / raw)
  To: Pumpkin; +Cc: linux-cifs, smfrench, tom, atteh.mailbox, Sergey Senozhatsky

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.
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals
  2023-05-05 15:11 [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Namjae Jeon
                   ` (4 preceding siblings ...)
  2023-05-05 15:11 ` [PATCH 6/6] ksmbd: use kzalloc() instead of __GFP_ZERO Namjae Jeon
@ 2023-05-08  1:05 ` Sergey Senozhatsky
  2023-05-08 12:58   ` Namjae Jeon
  5 siblings, 1 reply; 14+ messages in thread
From: Sergey Senozhatsky @ 2023-05-08  1:05 UTC (permalink / raw)
  To: Namjae Jeon
  Cc: linux-cifs, smfrench, senozhatsky, tom, atteh.mailbox, Pumpkin

On (23/05/06 00:11), Namjae Jeon wrote:
> From: Pumpkin <cc85nod@gmail.com>
> 
> If the length of CreateContext name is larger than the tag, it will access
> the data following the tag and trigger KASAN global-out-of-bounds.
> 
> Currently all CreateContext names are defined as string, so we can use
> strcmp instead of memcmp to avoid the out-of-bound access.

[..]

> +++ b/fs/ksmbd/oplock.c
> @@ -1492,7 +1492,7 @@ struct create_context *smb2_find_context_vals(void *open_req, const char *tag)
>  			return ERR_PTR(-EINVAL);
>  
>  		name = (char *)cc + name_off;
> -		if (memcmp(name, tag, name_len) == 0)
> +		if (!strcmp(name, tag))
>  			return cc;
>  
>  		remain_len -= next;

I'm slightly surprised that that huge `if` before memcmp() doesn't catch
it

		if ((next & 0x7) != 0 ||
		    next > remain_len ||
		    name_off != offsetof(struct create_context, Buffer) ||
		    name_len < 4 ||
		    name_off + name_len > cc_len ||
		    (value_off & 0x7) != 0 ||
		    (value_off && (value_off < name_off + name_len)) ||
		    ((u64)value_off + value_len > cc_len))
			return ERR_PTR(-EINVAL);

Is that because we should check `name_len` instead of `name_off + name_len`?
IOW

		if (name_len != cc_len)
			return ERR_PTR();

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 6/6] ksmbd: use kzalloc() instead of __GFP_ZERO
  2023-05-05 15:11 ` [PATCH 6/6] ksmbd: use kzalloc() instead of __GFP_ZERO Namjae Jeon
@ 2023-05-08  1:06   ` Sergey Senozhatsky
  0 siblings, 0 replies; 14+ messages in thread
From: Sergey Senozhatsky @ 2023-05-08  1:06 UTC (permalink / raw)
  To: Namjae Jeon
  Cc: linux-cifs, smfrench, senozhatsky, tom, atteh.mailbox, Dan Carpenter

On (23/05/06 00:11), Namjae Jeon wrote:
> Use kzalloc() instead of __GFP_ZERO.
> 
> Reported-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/6] ksmbd: remove unused ksmbd_tree_conn_share function
  2023-05-05 15:11 ` [PATCH 5/6] ksmbd: remove unused ksmbd_tree_conn_share function Namjae Jeon
@ 2023-05-08  1:07   ` Sergey Senozhatsky
  0 siblings, 0 replies; 14+ messages in thread
From: Sergey Senozhatsky @ 2023-05-08  1:07 UTC (permalink / raw)
  To: Namjae Jeon; +Cc: linux-cifs, smfrench, senozhatsky, tom, atteh.mailbox

On (23/05/06 00:11), Namjae Jeon wrote:
> 
> Remove unused ksmbd_tree_conn_share function.
> 
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals
  2023-05-08  1:05 ` [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Sergey Senozhatsky
@ 2023-05-08 12:58   ` Namjae Jeon
  2023-05-09  3:05     ` Sergey Senozhatsky
  0 siblings, 1 reply; 14+ messages in thread
From: Namjae Jeon @ 2023-05-08 12:58 UTC (permalink / raw)
  To: Pumpkin; +Cc: linux-cifs, smfrench, tom, atteh.mailbox, Sergey Senozhatsky

2023-05-08 10:05 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>:
> On (23/05/06 00:11), Namjae Jeon wrote:
>> From: Pumpkin <cc85nod@gmail.com>
>>
>> If the length of CreateContext name is larger than the tag, it will
>> access
>> the data following the tag and trigger KASAN global-out-of-bounds.
>>
>> Currently all CreateContext names are defined as string, so we can use
>> strcmp instead of memcmp to avoid the out-of-bound access.
Hi Chih-Yen,

Please reply to Sergey's review comment. If needed, please send v2
patch after updating it.

Thanks.
>
> [..]
>
>> +++ b/fs/ksmbd/oplock.c
>> @@ -1492,7 +1492,7 @@ struct create_context *smb2_find_context_vals(void
>> *open_req, const char *tag)
>>  			return ERR_PTR(-EINVAL);
>>
>>  		name = (char *)cc + name_off;
>> -		if (memcmp(name, tag, name_len) == 0)
>> +		if (!strcmp(name, tag))
>>  			return cc;
>>
>>  		remain_len -= next;
>
> I'm slightly surprised that that huge `if` before memcmp() doesn't catch
> it
>
> 		if ((next & 0x7) != 0 ||
> 		    next > remain_len ||
> 		    name_off != offsetof(struct create_context, Buffer) ||
> 		    name_len < 4 ||
> 		    name_off + name_len > cc_len ||
> 		    (value_off & 0x7) != 0 ||
> 		    (value_off && (value_off < name_off + name_len)) ||
> 		    ((u64)value_off + value_len > cc_len))
> 			return ERR_PTR(-EINVAL);
>
> Is that because we should check `name_len` instead of `name_off +
> name_len`?
> IOW
>
> 		if (name_len != cc_len)
> 			return ERR_PTR();
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals
  2023-05-08 12:58   ` Namjae Jeon
@ 2023-05-09  3:05     ` Sergey Senozhatsky
       [not found]       ` <CAAn9K_vRCOtYZXRBDKY4GzPA-TyrQ_Zh-qssu51Vr6sTwg5w4w@mail.gmail.com>
  0 siblings, 1 reply; 14+ messages in thread
From: Sergey Senozhatsky @ 2023-05-09  3:05 UTC (permalink / raw)
  To: Namjae Jeon
  Cc: Pumpkin, linux-cifs, smfrench, tom, atteh.mailbox, Sergey Senozhatsky

On (23/05/08 21:58), Namjae Jeon wrote:
> 2023-05-08 10:05 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>:
> > On (23/05/06 00:11), Namjae Jeon wrote:
> >> From: Pumpkin <cc85nod@gmail.com>
> >>
> >> If the length of CreateContext name is larger than the tag, it will
> >> access
> >> the data following the tag and trigger KASAN global-out-of-bounds.
> >>
> >> Currently all CreateContext names are defined as string, so we can use
> >> strcmp instead of memcmp to avoid the out-of-bound access.
> Hi Chih-Yen,
> 
> Please reply to Sergey's review comment. If needed, please send v2
> patch after updating it.

Chih-Yen replied privately, but let me move the discussion back to
public list.

> >> +++ b/fs/ksmbd/oplock.c
> >> @@ -1492,7 +1492,7 @@ struct create_context *smb2_find_context_vals(void
> >> *open_req, const char *tag)
> >>  			return ERR_PTR(-EINVAL);
> >>
> >>  		name = (char *)cc + name_off;
> >> -		if (memcmp(name, tag, name_len) == 0)
> >> +		if (!strcmp(name, tag))
> >>  			return cc;
> >>
> >>  		remain_len -= next;
> >
> > I'm slightly surprised that that huge `if` before memcmp() doesn't catch
> > it
> >
> > 		if ((next & 0x7) != 0 ||
> > 		    next > remain_len ||
> > 		    name_off != offsetof(struct create_context, Buffer) ||
> > 		    name_len < 4 ||
> > 		    name_off + name_len > cc_len ||
> > 		    (value_off & 0x7) != 0 ||
> > 		    (value_off && (value_off < name_off + name_len)) ||
> > 		    ((u64)value_off + value_len > cc_len))
> > 			return ERR_PTR(-EINVAL);

So the question is: why doesn't this `if` catch that problem?
I'd rather add one extra condition here, it doesn't make a lot
of sense to strcmp/memcmp if we know beforehand that two strings
have different sizes. So a simple "name len != context len" should
do the trick. No?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals
       [not found]       ` <CAAn9K_vRCOtYZXRBDKY4GzPA-TyrQ_Zh-qssu51Vr6sTwg5w4w@mail.gmail.com>
@ 2023-05-12 14:14         ` Namjae Jeon
  0 siblings, 0 replies; 14+ messages in thread
From: Namjae Jeon @ 2023-05-12 14:14 UTC (permalink / raw)
  To: 張智諺
  Cc: Sergey Senozhatsky, linux-cifs, smfrench, tom, atteh.mailbox

2023-05-10 20:04 GMT+09:00, 張智諺 <cc85nod@gmail.com>:
> Hi  Sergey Senozhatsky,
> Let me take an example to make sure what condition we should add.
>
> Before patching, if the tag is "ExtA" and the values of create_context
> fields are following:
> - next - 24
> - name_len - 8
> - name_off - 16
> In this situation, the large `if` condition will be passed:
> ```
> if ((next & 0x7) != 0 ||
>     ...
>     ((u64)value_off + value_len > cc_len))
>     return ERR_PTR(-EINVAL);
> ```
> But when we are doing `memcmp`, the ksmbd will out-of-bound access the
> memory of the tag.
>
> However, after applying your patch, which is "name len != context len", the
> large `if` condition
> is still passwd, and the ksmbd still triggers the oob-read bug.
>
> So if we don't want to change `memcmp` to `strcmp`, what we do in the large
> `if` condition is
> make sure that the name length of create_context is equal or less than the
> length of tag, but
> we only can get the length of tag by `strlen`.
>
> Is my analysis correct? And do you have any ideas?
Yes, we need to compare length also, And we should not use strlen() to
get tag length. create context tag doesn't include null terminator.
tag length is 4 or 16. We can add tag_len argument in
smb2_find_context_vals(). So we change caller like this.

context = smb2_find_context_vals(req,

SMB2_CREATE_TIMEWARP_REQUEST, 4);

and then, In the comparison part, length is also checked.

  if (name_len == tag_len && !memcmp(name, tag, name_len))
                return cc;

Thanks.
>
> Thanks.
>
>
> Sergey Senozhatsky <senozhatsky@chromium.org> 於 2023年5月9日 週二 下午12:05寫道:
>
>> On (23/05/08 21:58), Namjae Jeon wrote:
>> > 2023-05-08 10:05 GMT+09:00, Sergey Senozhatsky
>> > <senozhatsky@chromium.org
>> >:
>> > > On (23/05/06 00:11), Namjae Jeon wrote:
>> > >> From: Pumpkin <cc85nod@gmail.com>
>> > >>
>> > >> If the length of CreateContext name is larger than the tag, it will
>> > >> access
>> > >> the data following the tag and trigger KASAN global-out-of-bounds.
>> > >>
>> > >> Currently all CreateContext names are defined as string, so we can
>> > >> use
>> > >> strcmp instead of memcmp to avoid the out-of-bound access.
>> > Hi Chih-Yen,
>> >
>> > Please reply to Sergey's review comment. If needed, please send v2
>> > patch after updating it.
>>
>> Chih-Yen replied privately, but let me move the discussion back to
>> public list.
>>
>> > >> +++ b/fs/ksmbd/oplock.c
>> > >> @@ -1492,7 +1492,7 @@ struct create_context
>> *smb2_find_context_vals(void
>> > >> *open_req, const char *tag)
>> > >>                    return ERR_PTR(-EINVAL);
>> > >>
>> > >>            name = (char *)cc + name_off;
>> > >> -          if (memcmp(name, tag, name_len) == 0)
>> > >> +          if (!strcmp(name, tag))
>> > >>                    return cc;
>> > >>
>> > >>            remain_len -= next;
>> > >
>> > > I'm slightly surprised that that huge `if` before memcmp() doesn't
>> catch
>> > > it
>> > >
>> > >             if ((next & 0x7) != 0 ||
>> > >                 next > remain_len ||
>> > >                 name_off != offsetof(struct create_context, Buffer)
>> > > ||
>> > >                 name_len < 4 ||
>> > >                 name_off + name_len > cc_len ||
>> > >                 (value_off & 0x7) != 0 ||
>> > >                 (value_off && (value_off < name_off + name_len)) ||
>> > >                 ((u64)value_off + value_len > cc_len))
>> > >                     return ERR_PTR(-EINVAL);
>>
>> So the question is: why doesn't this `if` catch that problem?
>> I'd rather add one extra condition here, it doesn't make a lot
>> of sense to strcmp/memcmp if we know beforehand that two strings
>> have different sizes. So a simple "name len != context len" should
>> do the trick. No?
>>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2023-05-12 14:14 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-05 15:11 [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Namjae Jeon
2023-05-05 15:11 ` [PATCH 2/6] ksmbd: fix wrong UserName check in session_user Namjae Jeon
2023-05-06  3:10   ` Sergey Senozhatsky
2023-05-07  0:52     ` Namjae Jeon
2023-05-05 15:11 ` [PATCH 3/6] ksmbd: allocate one more byte for implied bcc[0] Namjae Jeon
2023-05-05 15:11 ` [PATCH 4/6] ksmbd: smb2: Allow messages padded to 8byte boundary Namjae Jeon
2023-05-05 15:11 ` [PATCH 5/6] ksmbd: remove unused ksmbd_tree_conn_share function Namjae Jeon
2023-05-08  1:07   ` Sergey Senozhatsky
2023-05-05 15:11 ` [PATCH 6/6] ksmbd: use kzalloc() instead of __GFP_ZERO Namjae Jeon
2023-05-08  1:06   ` Sergey Senozhatsky
2023-05-08  1:05 ` [PATCH 1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals Sergey Senozhatsky
2023-05-08 12:58   ` Namjae Jeon
2023-05-09  3:05     ` Sergey Senozhatsky
     [not found]       ` <CAAn9K_vRCOtYZXRBDKY4GzPA-TyrQ_Zh-qssu51Vr6sTwg5w4w@mail.gmail.com>
2023-05-12 14:14         ` Namjae Jeon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).