slab-out-of-bounds in ksmbd_ipc_login_request+0x78/0xc0

[mmakassikis]

Hello,

On a kernel compiled with CONFIG_KASAN=y and CONFIG_STACKTRACE=y, I see the following report in dmesg:

 ==================================================================
 BUG: KASAN: slab-out-of-bounds in ksmbd_ipc_login_request+0x78/0xc0 [ksmbd]
 Read of size 47 at addr ffffff8065bd2200 by task kworker/1:1/93

 CPU: 1 PID: 93 Comm: kworker/1:1 Not tainted 5.4.60-02905-gd748fef41fcc-dirty #5
 Workqueue: ksmbd-io handle_ksmbd_work [ksmbd]
 Call trace:
  dump_backtrace+0x0/0x1c0
  show_stack+0x14/0x20
  dump_stack+0xdc/0x12c
  print_address_description.isra.12+0x64/0x388
  __kasan_report+0x140/0x190
  kasan_report+0xc/0x18
  check_memory_region+0x17c/0x1e8
  memcpy+0x34/0x68
  ksmbd_ipc_login_request+0x78/0xc0 [ksmbd]
  ksmbd_alloc_user+0x10/0x190 [ksmbd]
  smb2_sess_setup+0x478/0xee8 [ksmbd]
  handle_ksmbd_work+0x2a4/0x710 [ksmbd]
  process_one_work+0x398/0x5e8
  worker_thread+0x70/0x5b0
  kthread+0x1d4/0x1e0
  ret_from_fork+0x10/0x1c

 Allocated by task 93:
  save_stack+0x24/0xb0
  __kasan_kmalloc.isra.11+0xc4/0xe0
  kasan_kmalloc+0xc/0x18
  __kmalloc+0x1a8/0x318
  smb_strndup_from_utf16+0x138/0x338 [ksmbd]
  smb2_sess_setup+0x430/0xee8 [ksmbd]
  handle_ksmbd_work+0x2a4/0x710 [ksmbd]
  process_one_work+0x398/0x5e8
  worker_thread+0x70/0x5b0
  kthread+0x1d4/0x1e0
  ret_from_fork+0x10/0x1c

 Freed by task 2159:
  save_stack+0x24/0xb0
  __kasan_slab_free+0x10c/0x188
  kasan_slab_free+0x10/0x18
  kfree+0xf4/0x300
  single_release+0x50/0x60
  close_pdeo+0x134/0x1e0
  proc_reg_release+0xbc/0xd8
  __fput+0x124/0x310
  ____fput+0xc/0x18
  task_work_run+0xd4/0x108
  do_notify_resume+0x170/0x568
  work_pending+0x8/0x10

 The buggy address belongs to the object at ffffff8065bd2200
  which belongs to the cache kmalloc-128 of size 128
 The buggy address is located 0 bytes inside of
  128-byte region [ffffff8065bd2200, ffffff8065bd2280)
 The buggy address belongs to the page:
 page:ffffffff0176f480 refcount:1 mapcount:0 mapping:ffffff806d003c80 index:0x0
 flags: 0x200(slab)
 raw: 0000000000000200 ffffffff016f0b40 0000000900000009 ffffff806d003c80
 raw: 0000000000000000 0000000000100010 00000001ffffffff 0000000000000000
 page dumped because: kasan: bad access detected

 Memory state around the buggy address:
  ffffff8065bd2100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  ffffff8065bd2180: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 >ffffff8065bd2200: 05 fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
                    ^
  ffffff8065bd2280: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
  ffffff8065bd2300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ==================================================================
 Disabling lock debugging due to kernel taint

The offending code in ksmbd_ipc_login_request() seems to be the following:

511
512         msg->type = KSMBD_EVENT_LOGIN_REQUEST;
513         req = KSMBD_IPC_MSG_PAYLOAD(msg);
514         req->handle = ksmbd_acquire_id(ida);
515         memcpy(req->account, account, sizeof(req->account) - 1);
516

The memcpy() call copies 'sizeof(req->account) - 1' bytes to the req->account. In doing so, it reads past the end of the 'account' buffer.

With the following patch, I no longer see the KASAN warning:

diff --git a/transport_ipc.c b/transport_ipc.c
index ee5d30c..7a93cf4 100644
--- a/transport_ipc.c
+++ b/transport_ipc.c
@@ -504,6 +504,7 @@ struct ksmbd_login_response *ksmbd_ipc_login_request(const
char *account)
        struct ksmbd_ipc_msg *msg;
        struct ksmbd_login_request *req;
        struct ksmbd_login_response *resp;
+       size_t account_sz;

        msg = ipc_msg_alloc(sizeof(struct ksmbd_login_request));
        if (!msg)
@@ -512,7 +513,14 @@ struct ksmbd_login_response
*ksmbd_ipc_login_request(const char *account)
        msg->type = KSMBD_EVENT_LOGIN_REQUEST;
        req = KSMBD_IPC_MSG_PAYLOAD(msg);
        req->handle = ksmbd_acquire_id(ida);
-       memcpy(req->account, account, sizeof(req->account) - 1);
+
+       account_sz = strlen(account);
+       if (account_sz < sizeof(req->account)) {
+               memcpy(req->account, account, account_sz);
+               req->account[account_sz] = '\0';
+       } else
+               return NULL;
+

        resp = ipc_msg_send_request(msg, req->handle);
        ipc_msg_handle_free(req->handle);

If my understanding is correct, `account' is built with the smb_strndup_from_utf16() function that nul-terminates the buffer, so it's ok to use strlen() to determine the buffer's size.

Thoughts ?

[namjaejeon]

Thanks for your report and patch. I prefer to add more simple code.

+ if ((strlen(account) >= KSMBD_REQ_MAX_ACCOUNT_NAME_SZ)
+             return NULL;
+ strcpy(req->acount, account);

How about this ?

[sergey-senozhatsky]

On (20/11/05 08:49), mmakassikis wrote:

The offending code in ksmbd_ipc_login_request() seems to be the following:

511 512 msg->type = KSMBD_EVENT_LOGIN_REQUEST; 513 req = KSMBD_IPC_MSG_PAYLOAD(msg); 514 req->handle = ksmbd_acquire_id(ida); 515 memcpy(req->account, account, sizeof(req->account) - 1); 516

memcpy() is definitely wrong there. Would be better to use a proper string copy function instead - strcpy().

-ss

[sergey-senozhatsky]

On (20/11/05 16:14), Namjae Jeon wrote:

Thanks for your report and patch. I prefer to add more simple code.

• if ((strlen(account) >= KSMBD_REQ_MAX_ACCOUNT_NAME_SZ)
• return NULL;

• strcpy(req->acount, account);

  How about this ?

This leaks the allocated req memory. The check should be done before msg = ipc_msg_alloc(), so that we won't even bother to allocate req for such case.

-ss

[namjaejeon]

This leaks the allocated req memory. The check should be done before msg = ipc_msg_alloc(), so that we won't even bother to allocate req for such case.

Right. size check should be moved to above msg = ipc_msg_alloc().

[namjaejeon]

@mmakassikis Could you please check the following change and send the patch to the mailing list(linux-cifsd-devel@lists.sourceforge.net)

+      if ((strlen(account) >= KSMBD_REQ_MAX_ACCOUNT_NAME_SZ)
+             return NULL;
         msg = ipc_msg_alloc(sizeof(struct ksmbd_login_request));
    .....
-       memcpy(req->account, account, sizeof(req->account) - 1);
+      strcpy(req->acount, account);

[mmakassikis]

I've sent a patch on the mailing list which uses strlcpy/strscpy instead of strcpy as you suggested. I've also changed other memcpy calls in the same file that dealt with strings to also use strlcpy/strscpy.

[namjaejeon]

@mmakassikis I pushed your patch to git tree. It will be merged into cifsd-team tree soon. Thanks!