[PATCH] cifs: Fix slab-out-of-bounds in send_set_info() on SMB2 ACE setting

[PATCH] cifs: Fix slab-out-of-bounds in send_set_info() on SMB2 ACE setting

[Stefano Brivio]

A "small" CIFS buffer is not big enough in general to hold a
setacl request for SMB2, and we end up overflowing the buffer in
send_set_info(). For instance:

 # mount.cifs //127.0.0.1/test /mnt/test -o username=test,password=test,nounix,cifsacl
 # touch /mnt/test/acltest
 # getcifsacl /mnt/test/acltest
 REVISION:0x1
 CONTROL:0x9004
 OWNER:S-1-5-21-2926364953-924364008-418108241-1000
 GROUP:S-1-22-2-1001
 ACL:S-1-5-21-2926364953-924364008-418108241-1000:ALLOWED/0x0/0x1e01ff
 ACL:S-1-22-2-1001:ALLOWED/0x0/R
 ACL:S-1-22-2-1001:ALLOWED/0x0/R
 ACL:S-1-5-21-2926364953-924364008-418108241-1000:ALLOWED/0x0/0x1e01ff
 ACL:S-1-1-0:ALLOWED/0x0/R
 # setcifsacl -a "ACL:S-1-22-2-1004:ALLOWED/0x0/R" /mnt/test/acltest

this setacl will cause the following KASAN splat:

[  330.777927] BUG: KASAN: slab-out-of-bounds in send_set_info+0x4dd/0xc20 [cifs]
[  330.779696] Write of size 696 at addr ffff88010d5e2860 by task setcifsacl/1012

[  330.781882] CPU: 1 PID: 1012 Comm: setcifsacl Not tainted 4.18.0-rc2+ #2
[  330.783140] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[  330.784395] Call Trace:
[  330.784789]  dump_stack+0xc2/0x16b
[  330.786777]  print_address_description+0x6a/0x270
[  330.787520]  kasan_report+0x258/0x380
[  330.788845]  memcpy+0x34/0x50
[  330.789369]  send_set_info+0x4dd/0xc20 [cifs]
[  330.799511]  SMB2_set_acl+0x76/0xa0 [cifs]
[  330.801395]  set_smb2_acl+0x7ac/0xf30 [cifs]
[  330.830888]  cifs_xattr_set+0x963/0xe40 [cifs]
[  330.840367]  __vfs_setxattr+0x84/0xb0
[  330.842060]  __vfs_setxattr_noperm+0xe6/0x370
[  330.843848]  vfs_setxattr+0xc2/0xd0
[  330.845519]  setxattr+0x258/0x320
[  330.859211]  path_setxattr+0x15b/0x1b0
[  330.864392]  __x64_sys_setxattr+0xc0/0x160
[  330.866133]  do_syscall_64+0x14e/0x4b0
[  330.876631]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  330.878503] RIP: 0033:0x7ff2e507db0a
[  330.880151] Code: 48 8b 0d 89 93 2c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 bc 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 56 93 2c 00 f7 d8 64 89 01 48
[  330.885358] RSP: 002b:00007ffdc4903c18 EFLAGS: 00000246 ORIG_RAX: 00000000000000bc
[  330.887733] RAX: ffffffffffffffda RBX: 000055d1170de140 RCX: 00007ff2e507db0a
[  330.890067] RDX: 000055d1170de7d0 RSI: 000055d115b39184 RDI: 00007ffdc4904818
[  330.892410] RBP: 0000000000000001 R08: 0000000000000000 R09: 000055d1170de7e4
[  330.894785] R10: 00000000000002b8 R11: 0000000000000246 R12: 0000000000000007
[  330.897148] R13: 000055d1170de0c0 R14: 0000000000000008 R15: 000055d1170de550

[  330.901057] Allocated by task 1012:
[  330.902888]  kasan_kmalloc+0xa0/0xd0
[  330.904714]  kmem_cache_alloc+0xc8/0x1d0
[  330.906615]  mempool_alloc+0x11e/0x380
[  330.908496]  cifs_small_buf_get+0x35/0x60 [cifs]
[  330.910510]  smb2_plain_req_init+0x4a/0xd60 [cifs]
[  330.912551]  send_set_info+0x198/0xc20 [cifs]
[  330.914535]  SMB2_set_acl+0x76/0xa0 [cifs]
[  330.916465]  set_smb2_acl+0x7ac/0xf30 [cifs]
[  330.918453]  cifs_xattr_set+0x963/0xe40 [cifs]
[  330.920426]  __vfs_setxattr+0x84/0xb0
[  330.922284]  __vfs_setxattr_noperm+0xe6/0x370
[  330.924213]  vfs_setxattr+0xc2/0xd0
[  330.926008]  setxattr+0x258/0x320
[  330.927762]  path_setxattr+0x15b/0x1b0
[  330.929592]  __x64_sys_setxattr+0xc0/0x160
[  330.931459]  do_syscall_64+0x14e/0x4b0
[  330.933314]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

[  330.936843] Freed by task 0:
[  330.938588] (stack is not available)

[  330.941886] The buggy address belongs to the object at ffff88010d5e2800
 which belongs to the cache cifs_small_rq of size 448
[  330.946362] The buggy address is located 96 bytes inside of
 448-byte region [ffff88010d5e2800, ffff88010d5e29c0)
[  330.950722] The buggy address belongs to the page:
[  330.952789] page:ffffea0004357880 count:1 mapcount:0 mapping:ffff880108fdca80 index:0x0 compound_mapcount: 0
[  330.955665] flags: 0x17ffffc0008100(slab|head)
[  330.957760] raw: 0017ffffc0008100 dead000000000100 dead000000000200 ffff880108fdca80
[  330.960356] raw: 0000000000000000 0000000080100010 00000001ffffffff 0000000000000000
[  330.963005] page dumped because: kasan: bad access detected

[  330.967039] Memory state around the buggy address:
[  330.969255]  ffff88010d5e2880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  330.971833]  ffff88010d5e2900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  330.974397] >ffff88010d5e2980: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc
[  330.976956]                                            ^
[  330.979226]  ffff88010d5e2a00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[  330.981755]  ffff88010d5e2a80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[  330.984225] ==================================================================

Fix this by allocating a regular CIFS buffer in
smb2_plain_req_init() if the request command is SMB2_SET_INFO.

Reported-by: Jianhong Yin <jiyin@redhat.com>
Fixes: 366ed846df60 ("cifs: Use smb 2 - 3 and cifsacl mount options setacl function")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 fs/cifs/smb2pdu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 7608c241b1ef..b2fa834da210 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -340,7 +340,10 @@ smb2_plain_req_init(__le16 smb2_command, struct cifs_tcon *tcon,
 		return rc;
 
 	/* BB eventually switch this to SMB2 specific small buf size */
-	*request_buf = cifs_small_buf_get();
+	if (smb2_command == SMB2_SET_INFO)
+		*request_buf = cifs_buf_get();
+	else
+		*request_buf = cifs_small_buf_get();
 	if (*request_buf == NULL) {
 		/* BB should we add a retry in here if not a writepage? */
 		return -ENOMEM;
@@ -3720,7 +3723,7 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
 
 	rc = cifs_send_recv(xid, ses, &rqst, &resp_buftype, flags,
 			    &rsp_iov);
-	cifs_small_buf_release(req);
+	cifs_buf_release(req);
 	rsp = (struct smb2_set_info_rsp *)rsp_iov.iov_base;
 
 	if (rc != 0) {
-- 
2.15.1

Re: [PATCH] cifs: Fix slab-out-of-bounds in send_set_info() on SMB2 ACE setting

[Aurélien Aptel]

Stefano Brivio <sbrivio@redhat.com> writes:
>  	/* BB eventually switch this to SMB2 specific small buf size */
> -	*request_buf = cifs_small_buf_get();
> +	if (smb2_command == SMB2_SET_INFO)
> +		*request_buf = cifs_buf_get();
> +	else
> +		*request_buf = cifs_small_buf_get();
>  	if (*request_buf == NULL) {
>  		/* BB should we add a retry in here if not a writepage? */
>  		return -ENOMEM;
> @@ -3720,7 +3723,7 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
>  
>  	rc = cifs_send_recv(xid, ses, &rqst, &resp_buftype, flags,
>  			    &rsp_iov);
> -	cifs_small_buf_release(req);
> +	cifs_buf_release(req);
>  	rsp = (struct smb2_set_info_rsp *)rsp_iov.iov_base;

Small and large bufs use different mempools, shouldn't the release func
match the get func?

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH] cifs: Fix slab-out-of-bounds in send_set_info() on SMB2 ACE setting

[Steve French]

On Thu, Jul 5, 2018 at 9:35 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Stefano Brivio <sbrivio@redhat.com> writes:
> >       /* BB eventually switch this to SMB2 specific small buf size */
> > -     *request_buf = cifs_small_buf_get();
> > +     if (smb2_command == SMB2_SET_INFO)
> > +             *request_buf = cifs_buf_get();
> > +     else
> > +             *request_buf = cifs_small_buf_get();
> >       if (*request_buf == NULL) {
> >               /* BB should we add a retry in here if not a writepage? */
> >               return -ENOMEM;
> > @@ -3720,7 +3723,7 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
> >
> >       rc = cifs_send_recv(xid, ses, &rqst, &resp_buftype, flags,
> >                           &rsp_iov);
> > -     cifs_small_buf_release(req);
> > +     cifs_buf_release(req);
> >       rsp = (struct smb2_set_info_rsp *)rsp_iov.iov_base;
>
> Small and large bufs use different mempools, shouldn't the release func
> match the get func?

Yes

Stefano,
Can you respin your patch?  I am hoping this patch addresses a bug I
have been seeing


-- 
Thanks,

Steve

Re: [PATCH] cifs: Fix slab-out-of-bounds in send_set_info() on SMB2 ACE setting

[Stefano Brivio]

On Thu, 05 Jul 2018 16:35:00 +0200
Aurélien Aptel <aaptel@suse.com> wrote:

> Stefano Brivio <sbrivio@redhat.com> writes:
> >  	/* BB eventually switch this to SMB2 specific small buf size */
> > -	*request_buf = cifs_small_buf_get();
> > +	if (smb2_command == SMB2_SET_INFO)
> > +		*request_buf = cifs_buf_get();
> > +	else
> > +		*request_buf = cifs_small_buf_get();
> >  	if (*request_buf == NULL) {
> >  		/* BB should we add a retry in here if not a writepage? */
> >  		return -ENOMEM;
> > @@ -3720,7 +3723,7 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
> >  
> >  	rc = cifs_send_recv(xid, ses, &rqst, &resp_buftype, flags,
> >  			    &rsp_iov);
> > -	cifs_small_buf_release(req);
> > +	cifs_buf_release(req);
> >  	rsp = (struct smb2_set_info_rsp *)rsp_iov.iov_base;  
> 
> Small and large bufs use different mempools, shouldn't the release func
> match the get func?

I think it does: for SMB2_SET_INFO we'll allocate with cifs_buf_get(),
which does:

	ret_buf = mempool_alloc(cifs_req_poolp, GFP_NOFS);

and release with cifs_buf_release():

	mempool_free(buf_to_free, cifs_req_poolp);

am I missing something?

-- 
Stefano

Re: [PATCH] cifs: Fix slab-out-of-bounds in send_set_info() on SMB2 ACE setting

[Stefano Brivio]

On Thu, 5 Jul 2018 09:55:49 -0500
Steve French <smfrench@gmail.com> wrote:

> On Thu, Jul 5, 2018 at 9:35 AM Aurélien Aptel <aaptel@suse.com> wrote:
> >
> > Stefano Brivio <sbrivio@redhat.com> writes:  
> > >       /* BB eventually switch this to SMB2 specific small buf size */
> > > -     *request_buf = cifs_small_buf_get();
> > > +     if (smb2_command == SMB2_SET_INFO)
> > > +             *request_buf = cifs_buf_get();
> > > +     else
> > > +             *request_buf = cifs_small_buf_get();
> > >       if (*request_buf == NULL) {
> > >               /* BB should we add a retry in here if not a writepage? */
> > >               return -ENOMEM;
> > > @@ -3720,7 +3723,7 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
> > >
> > >       rc = cifs_send_recv(xid, ses, &rqst, &resp_buftype, flags,
> > >                           &rsp_iov);
> > > -     cifs_small_buf_release(req);
> > > +     cifs_buf_release(req);
> > >       rsp = (struct smb2_set_info_rsp *)rsp_iov.iov_base;  
> >
> > Small and large bufs use different mempools, shouldn't the release func
> > match the get func?  
> 
> Yes
> 
> Stefano,
> Can you respin your patch?  I am hoping this patch addresses a bug I
> have been seeing

Steve,

I guess I'm missing something, but I fail to see the mismatch between
get and release, now for SMB2_SET_INFO we'll be using cifs_req_poolp in
both paths. What should I change?

-- 
Stefano

Re: [PATCH] cifs: Fix slab-out-of-bounds in send_set_info() on SMB2 ACE setting

[Aurélien Aptel]

Stefano Brivio <sbrivio@redhat.com> writes:
> I think it does: for SMB2_SET_INFO we'll allocate with cifs_buf_get(),
> which does:
>
> 	ret_buf = mempool_alloc(cifs_req_poolp, GFP_NOFS);
>
> and release with cifs_buf_release():
>
> 	mempool_free(buf_to_free, cifs_req_poolp);
>
> am I missing something?

I was just reading the diff and assumed it was all in the same function
nevermind, you're right.

Fix is correct but I don't like having packet specific code in the init
function.

We could also raise the small buf mempool slab size to take into account
the SETINFO with ACL so that it fits in a small buf.

It would be interesting to know the max size of a SETINFO for ACL for
that.

Meanwhile this is good enough as far as I'm concerned (if anyone has
other ideas feel free to comment).

Reviewed-by: Aurelien Aptel <aaptel@suse.com>

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH] cifs: Fix slab-out-of-bounds in send_set_info() on SMB2 ACE setting

[Aurélien Aptel]

Steve French <smfrench@gmail.com> writes:
> On Thu, Jul 5, 2018 at 9:35 AM Aurélien Aptel <aaptel@suse.com> wrote:
>>
>> Stefano Brivio <sbrivio@redhat.com> writes:
>> >       /* BB eventually switch this to SMB2 specific small buf size */
>> > -     *request_buf = cifs_small_buf_get();
>> > +     if (smb2_command == SMB2_SET_INFO)
>> > +             *request_buf = cifs_buf_get();
>> > +     else
>> > +             *request_buf = cifs_small_buf_get();
>> >       if (*request_buf == NULL) {
>> >               /* BB should we add a retry in here if not a writepage? */
>> >               return -ENOMEM;
>> > @@ -3720,7 +3723,7 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
>> >
>> >       rc = cifs_send_recv(xid, ses, &rqst, &resp_buftype, flags,
>> >                           &rsp_iov);
>> > -     cifs_small_buf_release(req);
>> > +     cifs_buf_release(req);
>> >       rsp = (struct smb2_set_info_rsp *)rsp_iov.iov_base;
>>
>> Small and large bufs use different mempools, shouldn't the release func
>> match the get func?
>
> Yes
>
> Stefano,
> Can you respin your patch?  I am hoping this patch addresses a bug I
> have been seeing

I've ran xfstests with the 2 patches on top (smb3 section only again) I
get good results less failures than v4.17 :)

Failures: generic/112 generic/123 generic/128 generic/210 generic/323 generic/355 generic/378 generic/469 generic/478 generic/484 generic/486
Failed 11 of 394 tests


Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH] cifs: Fix slab-out-of-bounds in send_set_info() on SMB2 ACE setting

[Steve French]

Reviewed the patch in more detail - and it looks correct am merging
this and the other fixes for cc:stable today into cifs-2.6.git
for-next

Thanks for spotting this.


On Thu, Jul 5, 2018 at 9:59 AM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Thu, 05 Jul 2018 16:35:00 +0200
> Aurélien Aptel <aaptel@suse.com> wrote:
>
> > Stefano Brivio <sbrivio@redhat.com> writes:
> > >     /* BB eventually switch this to SMB2 specific small buf size */
> > > -   *request_buf = cifs_small_buf_get();
> > > +   if (smb2_command == SMB2_SET_INFO)
> > > +           *request_buf = cifs_buf_get();
> > > +   else
> > > +           *request_buf = cifs_small_buf_get();
> > >     if (*request_buf == NULL) {
> > >             /* BB should we add a retry in here if not a writepage? */
> > >             return -ENOMEM;
> > > @@ -3720,7 +3723,7 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
> > >
> > >     rc = cifs_send_recv(xid, ses, &rqst, &resp_buftype, flags,
> > >                         &rsp_iov);
> > > -   cifs_small_buf_release(req);
> > > +   cifs_buf_release(req);
> > >     rsp = (struct smb2_set_info_rsp *)rsp_iov.iov_base;
> >
> > Small and large bufs use different mempools, shouldn't the release func
> > match the get func?
>
> I think it does: for SMB2_SET_INFO we'll allocate with cifs_buf_get(),
> which does:
>
>         ret_buf = mempool_alloc(cifs_req_poolp, GFP_NOFS);
>
> and release with cifs_buf_release():
>
>         mempool_free(buf_to_free, cifs_req_poolp);
>
> am I missing something?
>
> --
> Stefano
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve