[PATCH] ksmbd: fix slab-out-of-bounds read in smb2_handle_negotiate

[PATCH] ksmbd: fix slab-out-of-bounds read in smb2_handle_negotiate

[HexRabbit]

Check request_buf length first to avoid out-of-bounds read by
req->DialectCount.

[ 3350.990282] BUG: KASAN: slab-out-of-bounds in smb2_handle_negotiate+0x35d7/0x3e60
[ 3350.990282] Read of size 2 at addr ffff88810ad61346 by task kworker/5:0/276
[ 3351.000406] Workqueue: ksmbd-io handle_ksmbd_work
[ 3351.003499] Call Trace:
[ 3351.006473]  <TASK>
[ 3351.006473]  dump_stack_lvl+0x8d/0xe0
[ 3351.006473]  print_report+0xcc/0x620
[ 3351.006473]  kasan_report+0x92/0xc0
[ 3351.006473]  smb2_handle_negotiate+0x35d7/0x3e60
[ 3351.014760]  ksmbd_smb_negotiate_common+0x7a7/0xf00
[ 3351.014760]  handle_ksmbd_work+0x3f7/0x12d0
[ 3351.014760]  process_one_work+0xa85/0x1780

Signed-off-by: HexRabbit <h3xrabbit@gmail.com>
---
 fs/ksmbd/smb2pdu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index cb93fd231..972176bff 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -1057,16 +1057,16 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
 		return rc;
 	}
 
-	if (req->DialectCount == 0) {
-		pr_err("malformed packet\n");
+	smb2_buf_len = get_rfc1002_len(work->request_buf);
+	smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects);
+	if (smb2_neg_size > smb2_buf_len) {
 		rsp->hdr.Status = STATUS_INVALID_PARAMETER;
 		rc = -EINVAL;
 		goto err_out;
 	}
 
-	smb2_buf_len = get_rfc1002_len(work->request_buf);
-	smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects);
-	if (smb2_neg_size > smb2_buf_len) {
+	if (req->DialectCount == 0) {
+		pr_err("malformed packet\n");
 		rsp->hdr.Status = STATUS_INVALID_PARAMETER;
 		rc = -EINVAL;
 		goto err_out;
-- 
2.25.1

Re: [PATCH] ksmbd: fix slab-out-of-bounds read in smb2_handle_negotiate

[Namjae Jeon]

2023-05-17 18:59 GMT+09:00, HexRabbit <h3xrabbit@gmail.com>:
> Check request_buf length first to avoid out-of-bounds read by
> req->DialectCount.
>
> [ 3350.990282] BUG: KASAN: slab-out-of-bounds in
> smb2_handle_negotiate+0x35d7/0x3e60
> [ 3350.990282] Read of size 2 at addr ffff88810ad61346 by task
> kworker/5:0/276
> [ 3351.000406] Workqueue: ksmbd-io handle_ksmbd_work
> [ 3351.003499] Call Trace:
> [ 3351.006473]  <TASK>
> [ 3351.006473]  dump_stack_lvl+0x8d/0xe0
> [ 3351.006473]  print_report+0xcc/0x620
> [ 3351.006473]  kasan_report+0x92/0xc0
> [ 3351.006473]  smb2_handle_negotiate+0x35d7/0x3e60
> [ 3351.014760]  ksmbd_smb_negotiate_common+0x7a7/0xf00
> [ 3351.014760]  handle_ksmbd_work+0x3f7/0x12d0
> [ 3351.014760]  process_one_work+0xa85/0x1780
>
> Signed-off-by: HexRabbit <h3xrabbit@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Sergey will say, "Do we still have a requirement that there should be
a real name in SoB?"

Thanks for your patch!

Re: [PATCH] ksmbd: fix slab-out-of-bounds read in smb2_handle_negotiate

[Sergey Senozhatsky]

On (23/05/17 19:17), Namjae Jeon wrote:
> > Signed-off-by: HexRabbit <h3xrabbit@gmail.com>
> Acked-by: Namjae Jeon <linkinjeon@kernel.org>
> 
> Sergey will say, "Do we still have a requirement that there should be
> a real name in SoB?"

:)   Thanks!

Re: [PATCH] ksmbd: fix slab-out-of-bounds read in smb2_handle_negotiate

[Sergey Senozhatsky]

On (23/05/17 09:59), HexRabbit wrote:
> [ 3350.990282] BUG: KASAN: slab-out-of-bounds in smb2_handle_negotiate+0x35d7/0x3e60
> [ 3350.990282] Read of size 2 at addr ffff88810ad61346 by task kworker/5:0/276
> [ 3351.000406] Workqueue: ksmbd-io handle_ksmbd_work
> [ 3351.003499] Call Trace:
> [ 3351.006473]  <TASK>
> [ 3351.006473]  dump_stack_lvl+0x8d/0xe0
> [ 3351.006473]  print_report+0xcc/0x620
> [ 3351.006473]  kasan_report+0x92/0xc0
> [ 3351.006473]  smb2_handle_negotiate+0x35d7/0x3e60
> [ 3351.014760]  ksmbd_smb_negotiate_common+0x7a7/0xf00
> [ 3351.014760]  handle_ksmbd_work+0x3f7/0x12d0
> [ 3351.014760]  process_one_work+0xa85/0x1780

[..]

> -	if (req->DialectCount == 0) {
> -		pr_err("malformed packet\n");
> +	smb2_buf_len = get_rfc1002_len(work->request_buf);
> +	smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects);
> +	if (smb2_neg_size > smb2_buf_len) {
>  		rsp->hdr.Status = STATUS_INVALID_PARAMETER;
>  		rc = -EINVAL;
>  		goto err_out;
>  	}
>  
> -	smb2_buf_len = get_rfc1002_len(work->request_buf);
> -	smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects);
> -	if (smb2_neg_size > smb2_buf_len) {
> +	if (req->DialectCount == 0) {
> +		pr_err("malformed packet\n");
>  		rsp->hdr.Status = STATUS_INVALID_PARAMETER;
>  		rc = -EINVAL;
>  		goto err_out;

May I please ask where out-of-bounds access happens and how does
`smb2_neg_size > smb2_buf_len` fix it?

Re: [PATCH] ksmbd: fix slab-out-of-bounds read in smb2_handle_negotiate

[Sergey Senozhatsky]

On (23/05/17 20:05), Sergey Senozhatsky wrote:
> On (23/05/17 09:59), HexRabbit wrote:
> > [ 3350.990282] BUG: KASAN: slab-out-of-bounds in smb2_handle_negotiate+0x35d7/0x3e60
> > [ 3350.990282] Read of size 2 at addr ffff88810ad61346 by task kworker/5:0/276
> > [ 3351.000406] Workqueue: ksmbd-io handle_ksmbd_work
> > [ 3351.003499] Call Trace:
> > [ 3351.006473]  <TASK>
> > [ 3351.006473]  dump_stack_lvl+0x8d/0xe0
> > [ 3351.006473]  print_report+0xcc/0x620
> > [ 3351.006473]  kasan_report+0x92/0xc0
> > [ 3351.006473]  smb2_handle_negotiate+0x35d7/0x3e60
> > [ 3351.014760]  ksmbd_smb_negotiate_common+0x7a7/0xf00
> > [ 3351.014760]  handle_ksmbd_work+0x3f7/0x12d0
> > [ 3351.014760]  process_one_work+0xa85/0x1780
> 
> [..]
> 
> > -	if (req->DialectCount == 0) {
> > -		pr_err("malformed packet\n");
> > +	smb2_buf_len = get_rfc1002_len(work->request_buf);
> > +	smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects);
> > +	if (smb2_neg_size > smb2_buf_len) {
> >  		rsp->hdr.Status = STATUS_INVALID_PARAMETER;
> >  		rc = -EINVAL;
> >  		goto err_out;
> >  	}
> >  
> > -	smb2_buf_len = get_rfc1002_len(work->request_buf);
> > -	smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects);
> > -	if (smb2_neg_size > smb2_buf_len) {
> > +	if (req->DialectCount == 0) {
> > +		pr_err("malformed packet\n");
> >  		rsp->hdr.Status = STATUS_INVALID_PARAMETER;
> >  		rc = -EINVAL;
> >  		goto err_out;
> 
> May I please ask where out-of-bounds access happens and how does
> `smb2_neg_size > smb2_buf_len` fix it?

Correction: I meant to ask "how does moving `smb2_neg_size > smb2_buf_len`
up fixes it?".

We have this in the code at the moment

```
         if (req->DialectCount == 0) {
                 pr_err("malformed packet\n");
                 rsp->hdr.Status = STATUS_INVALID_PARAMETER;
                 rc = -EINVAL;
                 goto err_out;
         }

         smb2_buf_len = get_rfc1002_len(work->request_buf);
         smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects);
         if (smb2_neg_size > smb2_buf_len) {
                 rsp->hdr.Status = STATUS_INVALID_PARAMETER;
                 rc = -EINVAL;
                 goto err_out;
         }
```

But if we move `smb2_neg_size > smb2_buf_len` brunch up, then it cures
out-of-bounds access? Where is that out-of-bounds access? Looking at
the stack trace, smb2_handle_negotiate+0x35d7/0x3e60 should be somewhere
much-much later than these if-s.

Re: [PATCH] ksmbd: fix slab-out-of-bounds read in smb2_handle_negotiate

[Sergey Senozhatsky]

On (23/05/17 19:45), Hex Rabbit wrote:
>    The out-of-bounds access is triggered by `req->DialectCount` memory
>    access,
>    sender can construct a malformed packet that only has a single 
>    `smb2_negotiate_req.StructureSize` field after the smb2 header.

Oh, I see, is that what you did in your reproducer?

>    Referring to the payload I showed below, since the function is 
>    assuming that the entire `smb2_negotiate_req` structure is presented, 
>    it will read above the `StructureSize` field (2400) and trigger KASAN. 
>    So check against `smb2_buf_len` first will ensure entire structure 
>    is inside the buffer, hope this make sense!
>    ```
>    00000000: 0000 0042 fe53 4d42 4000 0000 0000 0000  ...B.SMB@.......
>    00000010: 0000 0000 0000 0000 0000 0000 0000 0000  ................
>    00000020: 0000 0000 0000 0000 0000 0000 0000 0000  ................
>    00000030: 0000 0000 0000 0000 0000 0000 0000 0000  ................
>    00000040: 0000 0000 2400                                              
>    ....$.
>    ```
>    sorry for not providing full symbolized stack trace first

Thanks for clarifications. Maybe would be nice to have some of these
lines in the commit message.

>    Call Trace:
>    dump_stack_lvl (lib/dump_stack.c:107)
>    print_report (mm/kasan/report.c:352 mm/kasan/report.c:462)
>    kasan_report (mm/kasan/report.c:574)
>    smb2_handle_negotiate (fs/ksmbd/smb2pdu.c:1060)

I'm still puzzled by smb2_handle_negotiate+0x35d7/0x3e60 in the original
stack trace. 0x35d7/0x3e60 certainly doesn't translate to "start of the
function" to me, but what do I know :)

Re: [PATCH] ksmbd: fix slab-out-of-bounds read in smb2_handle_negotiate

[Hex Rabbit]

>    Oh, I see, is that what you did in your reproducer?

Yes, that's how I reproduce it.

>    I'm still puzzled by smb2_handle_negotiate+0x35d7/0x3e60 in the original
>    stack trace. 0x35d7/0x3e60 certainly doesn't translate to "start of the
>    function" to me, but what do I know :)

As you can see in the assembly below, the call to asan_report_*
functions is placed
at the bottom of the function, that's why the stack trace looks like that.

```
; smb2_handle_negotiate+0x216
.text:FFFFFFFF81FDA4F6                 test    dl, dl
.text:FFFFFFFF81FDA4F8                 setnz   al
.text:FFFFFFFF81FDA4FB                 test    cl, al
                  ; KASAN check
.text:FFFFFFFF81FDA4FD                 jnz     loc_FFFFFFFF81FDD80E  ;
jump to report

; smb2_handle_negotiate+0x352e
.text:FFFFFFFF81FDD80E loc_FFFFFFFF81FDD80E:
.text:FFFFFFFF81FDD80E                 mov     esi, 2
.text:FFFFFFFF81FDD813                 call    __asan_report_load_n_noabort
.text:FFFFFFFF81FDD818                 jmp     loc_FFFFFFFF81FDA503
.text:FFFFFFFF81FDD81D loc_FFFFFFFF81FDD81D:
.text:FFFFFFFF81FDD81D                 mov     esi, 4
.text:FFFFFFFF81FDD822                 call    __asan_report_store_n_noabort
.text:FFFFFFFF81FDD827                 jmp     loc_FFFFFFFF81FDAFA7
.text:FFFFFFFF81FDD82C loc_FFFFFFFF81FDD82C:
.text:FFFFFFFF81FDD82C                 mov     rdi, rcx
.text:FFFFFFFF81FDD82F                 call    __asan_report_load2_noabort
.text:FFFFFFFF81FDD834                 jmp     loc_FFFFFFFF81FDA558
```