| Message ID | af471807b329692a387ff26e62e1bbca01e1684f.1530783326.git.sbrivio@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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
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?
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 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,
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,
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
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) {
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(-)