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

[Sergey Senozhatsky <senozhatsky@chromium.org>]

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.