| Message ID | 20171017124717.25955-1-aaptel@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | CIFS: do not send invalid input buffer on QUERY_INFO requests | expand |
Just tried out this patch, and the CIFS module does indeed produce well-formed GetInfo requests now (according to Wireshark's definition at least, didn't check myself). However, the server will still reject the request with a STATUS_NOT_SUPPORTED error, suggesting that Steve's interpretation was the right one: this is likely a server-side bug. Cheers, Hadrien Le 17/10/2017 à 14:47, Aurelien Aptel a écrit : > query_info() doesn't use the InputBuffer field of the QUERY_INFO > request, therefore according to [MS-SMB2] it must: > > a) set the InputBufferOffset to 0 > b) send a zero-length InputBuffer > > Doing a) is trivial but b) is a bit more tricky. > > The packet is allocated according to it's StructureSize, which takes > into account an extra 1 byte buffer which we don't need > here. StructureSize fields must have constant values no matter the > actual length of the whole packet so we can't just edit that constant. > > Both the NetBIOS-over-TCP message length ("rfc1002 length") L and the > iovec length L' have to be updated. Since L' is computed from L we > just update L by decrementing it by one. > > Signed-off-by: Aurelien Aptel <aaptel@suse.com> > --- > fs/cifs/smb2pdu.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 6f0e6343c15e..b927e131f997 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -2191,9 +2191,13 @@ query_info(const unsigned int xid, struct cifs_tcon *tcon, > req->PersistentFileId = persistent_fid; > req->VolatileFileId = volatile_fid; > req->AdditionalInformation = cpu_to_le32(additional_info); > - /* 4 for rfc1002 length field and 1 for Buffer */ > - req->InputBufferOffset = > - cpu_to_le16(sizeof(struct smb2_query_info_req) - 1 - 4); > + > + /* > + * We do not use the input buffer (do not send extra byte) > + */ > + req->InputBufferOffset = 0; > + inc_rfc1001_len(req, -1); > + > req->OutputBufferLength = cpu_to_le32(output_len); > > iov[0].iov_base = (char *)req; [ 1808.856080] device enp0s25 entered promiscuous mode [ 1815.757524] fs/cifs/cifsfs.c: Devname: //nfsserv22/expsi flags: 0 [ 1815.757565] fs/cifs/connect.c: Domain name set [ 1815.757571] fs/cifs/connect.c: Username: grasland [ 1815.757575] fs/cifs/connect.c: file mode: 0x1ed dir mode: 0x1ed [ 1815.757578] fs/cifs/connect.c: CIFS VFS: in cifs_mount as Xid: 6 with uid: 0 [ 1815.757580] fs/cifs/connect.c: UNC: \\nfsserv22\expsi [ 1815.757594] fs/cifs/connect.c: Socket created [ 1815.757596] fs/cifs/connect.c: sndbuf 16384 rcvbuf 87380 rcvtimeo 0x6d6 [ 1815.758284] fs/cifs/fscache.c: cifs_fscache_get_client_cookie: (0xffff883a2bbf8000/0xffff883b522ef960) [ 1815.758287] fs/cifs/connect.c: Demultiplex PID: 15841 [ 1815.758292] fs/cifs/connect.c: CIFS VFS: in cifs_get_smb_ses as Xid: 7 with uid: 0 [ 1815.758293] fs/cifs/connect.c: Existing smb sess not found [ 1815.758299] fs/cifs/smb2pdu.c: Negotiate protocol [ 1815.758307] fs/cifs/transport.c: Sending smb: smb_len=102 [ 1815.761290] fs/cifs/connect.c: RFC1002 header 0xd3 [ 1815.761297] fs/cifs/smb2misc.c: smb2_check_message length: 0xd7, smb_buf_length: 0xd3 [ 1815.761300] fs/cifs/smb2misc.c: SMB2 data length 83 offset 128 [ 1815.761301] fs/cifs/smb2misc.c: SMB2 len 215 [ 1815.761341] fs/cifs/transport.c: cifs_sync_mid_result: cmd=0 mid=0 state=4 [ 1815.761344] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release [ 1815.761349] fs/cifs/smb2pdu.c: mode 0x1 [ 1815.761350] fs/cifs/smb2pdu.c: negotiated smb2.0 dialect [ 1815.761358] fs/cifs/asn1.c: OID len = 7 oid = 0x1 0x2 0x348 0xbb92 [ 1815.761360] fs/cifs/asn1.c: OID len = 7 oid = 0x1 0x2 0x348 0x1bb92 [ 1815.761362] fs/cifs/asn1.c: OID len = 10 oid = 0x1 0x3 0x6 0x1 [ 1815.761366] fs/cifs/connect.c: Security Mode: 0x1 Capabilities: 0x300001 TimeAdjust: 0 [ 1815.761367] fs/cifs/smb2pdu.c: Session Setup [ 1815.761368] fs/cifs/smb2pdu.c: sess setup type 4 [ 1815.761372] fs/cifs/transport.c: Sending smb: smb_len=120 [ 1815.762083] fs/cifs/connect.c: RFC1002 header 0xee [ 1815.762089] fs/cifs/smb2misc.c: smb2_check_message length: 0xf2, smb_buf_length: 0xee [ 1815.762091] fs/cifs/smb2misc.c: SMB2 data length 166 offset 72 [ 1815.762092] fs/cifs/smb2misc.c: SMB2 len 242 [ 1815.762128] fs/cifs/transport.c: cifs_sync_mid_result: cmd=1 mid=1 state=4 [ 1815.762134] fs/cifs/smb2maperror.c: Mapping SMB2 status code 0xc0000016 to POSIX err -5 [ 1815.762135] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release [ 1815.762139] fs/cifs/smb2pdu.c: rawntlmssp session setup challenge phase [ 1815.762187] fs/cifs/transport.c: Sending smb: smb_len=348 [ 1815.782014] fs/cifs/connect.c: RFC1002 header 0x48 [ 1815.782020] fs/cifs/smb2misc.c: smb2_check_message length: 0x4c, smb_buf_length: 0x48 [ 1815.782022] fs/cifs/smb2misc.c: SMB2 data length 0 offset 72 [ 1815.782023] fs/cifs/smb2misc.c: SMB2 len 77 [ 1815.782025] fs/cifs/smb2misc.c: Calculated size 77 length 76 mismatch mid 2 [ 1815.782064] fs/cifs/transport.c: cifs_sync_mid_result: cmd=1 mid=2 state=4 [ 1815.782067] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release [ 1815.782071] fs/cifs/smb2pdu.c: SMB2/3 session established successfully [ 1815.782075] fs/cifs/connect.c: CIFS VFS: leaving cifs_get_smb_ses (xid = 7) rc = 0 [ 1815.782079] fs/cifs/connect.c: CIFS VFS: in cifs_get_tcon as Xid: 8 with uid: 0 [ 1815.782080] fs/cifs/smb2pdu.c: TCON [ 1815.782084] fs/cifs/transport.c: Sending smb: smb_len=108 [ 1815.782739] fs/cifs/connect.c: RFC1002 header 0x50 [ 1815.782744] fs/cifs/smb2misc.c: smb2_check_message length: 0x54, smb_buf_length: 0x50 [ 1815.782746] fs/cifs/smb2misc.c: SMB2 len 84 [ 1815.782781] fs/cifs/transport.c: cifs_sync_mid_result: cmd=3 mid=3 state=4 [ 1815.782784] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release [ 1815.782787] fs/cifs/smb2pdu.c: connection to disk share [ 1815.782790] fs/cifs/connect.c: CIFS VFS: leaving cifs_get_tcon (xid = 8) rc = 0 [ 1815.782792] fs/cifs/connect.c: Tcon rc = 0 [ 1815.782797] fs/cifs/fscache.c: cifs_fscache_get_super_cookie: (0xffff883b522ef960/0xffff883b522efaf0) [ 1815.782799] fs/cifs/smb2pdu.c: create/open [ 1815.782803] fs/cifs/transport.c: Sending smb: smb_len=128 [ 1815.783498] fs/cifs/connect.c: RFC1002 header 0x98 [ 1815.783503] fs/cifs/smb2misc.c: smb2_check_message length: 0x9c, smb_buf_length: 0x98 [ 1815.783505] fs/cifs/smb2misc.c: SMB2 data length 0 offset 0 [ 1815.783507] fs/cifs/smb2misc.c: SMB2 len 157 [ 1815.783509] fs/cifs/smb2misc.c: Calculated size 157 length 156 mismatch mid 4 [ 1815.783543] fs/cifs/transport.c: cifs_sync_mid_result: cmd=5 mid=4 state=4 [ 1815.783547] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release [ 1815.783551] fs/cifs/smb2pdu.c: Query FSInfo level 5 [ 1815.783555] fs/cifs/transport.c: Sending smb: smb_len=105 [ 1815.784202] fs/cifs/connect.c: RFC1002 header 0x5c [ 1815.784208] fs/cifs/smb2misc.c: smb2_check_message length: 0x60, smb_buf_length: 0x5c [ 1815.784210] fs/cifs/smb2misc.c: SMB2 data length 20 offset 72 [ 1815.784211] fs/cifs/smb2misc.c: SMB2 len 96 [ 1815.784225] fs/cifs/transport.c: cifs_sync_mid_result: cmd=16 mid=5 state=4 [ 1815.784228] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release [ 1815.784232] fs/cifs/smb2pdu.c: Query FSInfo level 4 [ 1815.784236] fs/cifs/transport.c: Sending smb: smb_len=105 [ 1815.784815] fs/cifs/connect.c: RFC1002 header 0x50 [ 1815.784821] fs/cifs/smb2misc.c: smb2_check_message length: 0x54, smb_buf_length: 0x50 [ 1815.784823] fs/cifs/smb2misc.c: SMB2 data length 8 offset 72 [ 1815.784824] fs/cifs/smb2misc.c: SMB2 len 84 [ 1815.784862] fs/cifs/transport.c: cifs_sync_mid_result: cmd=16 mid=6 state=4 [ 1815.784865] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release [ 1815.784868] fs/cifs/smb2pdu.c: Close [ 1815.784872] fs/cifs/transport.c: Sending smb: smb_len=88 [ 1815.785815] fs/cifs/connect.c: RFC1002 header 0x7c [ 1815.785821] fs/cifs/smb2misc.c: smb2_check_message length: 0x80, smb_buf_length: 0x7c [ 1815.785823] fs/cifs/smb2misc.c: SMB2 len 128 [ 1815.785860] fs/cifs/transport.c: cifs_sync_mid_result: cmd=6 mid=7 state=4 [ 1815.785863] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release [ 1815.785869] fs/cifs/connect.c: build_unc_path_to_root: full_path=\\nfsserv22\expsi [ 1815.785871] fs/cifs/smb2pdu.c: TCON [ 1815.785875] fs/cifs/transport.c: Sending smb: smb_len=116 [ 1815.786587] fs/cifs/connect.c: RFC1002 header 0x50 [ 1815.786593] fs/cifs/smb2misc.c: smb2_check_message length: 0x54, smb_buf_length: 0x50 [ 1815.786595] fs/cifs/smb2misc.c: SMB2 len 84 [ 1815.786632] fs/cifs/transport.c: cifs_sync_mid_result: cmd=3 mid=8 state=4 [ 1815.786635] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release [ 1815.786639] fs/cifs/connect.c: Tcon rc = 0 ipc_tid = 65 [ 1815.786642] fs/cifs/smb2ops.c: smb2_get_dfs_refer path <\nfsserv22\expsi> [ 1815.786646] fs/cifs/smb2pdu.c: SMB2 IOCTL [ 1815.786648] fs/cifs/smb2pdu.c: replacing tid 0x40 with IPC tid 0x41 [ 1815.786651] fs/cifs/transport.c: Sending smb: smb_len=156 [ 1815.787382] fs/cifs/connect.c: RFC1002 header 0x49 [ 1815.787388] fs/cifs/smb2misc.c: smb2_check_message length: 0x4d, smb_buf_length: 0x49 [ 1815.787390] fs/cifs/smb2misc.c: SMB2 data length 0 offset 0 [ 1815.787391] fs/cifs/smb2misc.c: SMB2 len 77 [ 1815.787427] fs/cifs/transport.c: cifs_sync_mid_result: cmd=11 mid=9 state=4 [ 1815.787434] fs/cifs/smb2maperror.c: Mapping SMB2 status code 0xc0000225 to POSIX err -2 [ 1815.787435] fs/cifs/misc.c: Null buffer passed to cifs_small_buf_release [ 1815.787439] CIFS VFS: ioctl error in smb2_get_dfs_refer rc=-2 [ 1815.787447] fs/cifs/smb2pdu.c: create/open [ 1815.788648] fs/cifs/smb2misc.c: SMB2 data length 0 offset 0 [ 1815.788651] fs/cifs/smb2misc.c: Calculated size 157 length 156 mismatch mid 10 [ 1815.788669] fs/cifs/smb2pdu.c: Close [ 1815.789451] fs/cifs/smb2pdu.c: create/open [ 1815.790282] fs/cifs/smb2misc.c: SMB2 data length 0 offset 0 [ 1815.790285] fs/cifs/smb2misc.c: Calculated size 157 length 156 mismatch mid 12 [ 1815.790324] fs/cifs/smb2pdu.c: Close [ 1815.791316] fs/cifs/connect.c: CIFS VFS: leaving cifs_mount (xid = 6) rc = 0 [ 1815.791394] fs/cifs/inode.c: CIFS VFS: in cifs_root_iget as Xid: 9 with uid: 0 [ 1815.791395] fs/cifs/inode.c: Getting info on [ 1815.791401] fs/cifs/smb2pdu.c: create/open [ 1815.792086] fs/cifs/smb2misc.c: SMB2 data length 0 offset 0 [ 1815.792089] fs/cifs/smb2misc.c: Calculated size 157 length 156 mismatch mid 14 [ 1815.792106] fs/cifs/smb2pdu.c: Query Info [ 1815.792723] fs/cifs/smb2maperror.c: Mapping SMB2 status code 0xc00000bb to POSIX err -95 [ 1815.792727] fs/cifs/smb2pdu.c: Close [ 1815.794352] fs/cifs/smb2pdu.c: create/open [ 1815.795290] fs/cifs/smb2misc.c: Calculated size 157 length 156 mismatch mid 17 [ 1815.795321] fs/cifs/smb2pdu.c: Query Info [ 1815.795974] fs/cifs/smb2maperror.c: Mapping SMB2 status code 0xc00000bb to POSIX err -95 [ 1815.795977] fs/cifs/smb2pdu.c: Close [ 1815.797136] CIFS VFS: cifs_read_super: get root inode failed [ 1815.816161] fs/cifs/connect.c: cifs_put_tcon: tc_count=1 [ 1815.816165] fs/cifs/connect.c: CIFS VFS: in cifs_put_tcon as Xid: 10 with uid: 0 [ 1815.816167] fs/cifs/smb2pdu.c: Tree Disconnect [ 1815.816776] fs/cifs/fscache.c: cifs_fscache_release_super_cookie: (0xffff883b522efaf0) [ 1815.816782] fs/cifs/connect.c: cifs_put_smb_ses: ses_count=1 [ 1815.816784] fs/cifs/connect.c: CIFS VFS: in cifs_put_smb_ses as Xid: 11 with uid: 0 [ 1815.816786] fs/cifs/smb2pdu.c: disconnect session ffff883b9abf1400 [ 1815.817500] fs/cifs/fscache.c: cifs_fscache_release_client_cookie: (0xffff883a2bbf8000/0xffff883b522ef960) [ 1820.409323] device enp0s25 left promiscuous mode
Hadrien Grasland <grasland@lal.in2p3.fr> writes: > Just tried out this patch, and the CIFS module does indeed produce > well-formed GetInfo requests now (according to Wireshark's definition at > least, didn't check myself). > > However, the server will still reject the request with a > STATUS_NOT_SUPPORTED error, suggesting that Steve's interpretation was > the right one: this is likely a server-side bug. Yes. As Steve suggested as a workaround, we could try to use a less detailed information level if FullInfo fails. Is it worth doing for a single old NetApp server? As for the offset and extra byte in QUERY_INFO I guess having the length set to 0 makes it ok... I've sent a couple of patches to Wireshark to fix the packet parsing. It now accepts the empty filename in CREATE (no more "[unknown]"), which enables the proper parsing of the unknown field.
rebased cifs-2.6.git for-next and pushed this patch On Tue, Oct 17, 2017 at 7:47 AM, Aurelien Aptel <aaptel@suse.com> wrote: > query_info() doesn't use the InputBuffer field of the QUERY_INFO > request, therefore according to [MS-SMB2] it must: > > a) set the InputBufferOffset to 0 > b) send a zero-length InputBuffer > > Doing a) is trivial but b) is a bit more tricky. > > The packet is allocated according to it's StructureSize, which takes > into account an extra 1 byte buffer which we don't need > here. StructureSize fields must have constant values no matter the > actual length of the whole packet so we can't just edit that constant. > > Both the NetBIOS-over-TCP message length ("rfc1002 length") L and the > iovec length L' have to be updated. Since L' is computed from L we > just update L by decrementing it by one. > > Signed-off-by: Aurelien Aptel <aaptel@suse.com> > --- > fs/cifs/smb2pdu.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 6f0e6343c15e..b927e131f997 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -2191,9 +2191,13 @@ query_info(const unsigned int xid, struct cifs_tcon *tcon, > req->PersistentFileId = persistent_fid; > req->VolatileFileId = volatile_fid; > req->AdditionalInformation = cpu_to_le32(additional_info); > - /* 4 for rfc1002 length field and 1 for Buffer */ > - req->InputBufferOffset = > - cpu_to_le16(sizeof(struct smb2_query_info_req) - 1 - 4); > + > + /* > + * We do not use the input buffer (do not send extra byte) > + */ > + req->InputBufferOffset = 0; > + inc_rfc1001_len(req, -1); > + > req->OutputBufferLength = cpu_to_le32(output_len); > > iov[0].iov_base = (char *)req; > -- > 2.12.3 >
Le 18/10/2017 à 16:50, Aurélien Aptel a écrit : > Hadrien Grasland <grasland@lal.in2p3.fr> writes: >> Just tried out this patch, and the CIFS module does indeed produce >> well-formed GetInfo requests now (according to Wireshark's definition at >> least, didn't check myself). >> >> However, the server will still reject the request with a >> STATUS_NOT_SUPPORTED error, suggesting that Steve's interpretation was >> the right one: this is likely a server-side bug. > Yes. As Steve suggested as a workaround, we could try to use a less > detailed information level if FullInfo fails. Is it worth doing for a > single old NetApp server? If it's only me that's having the issue, I would say don't bother. The same system issue that prevents my sysadmin colleagues from updating to a NetApp software version where the bug is fixed also prevents them from disabling SMBv1 support, and sticking with SMBv1 for now is fine by me. > As for the offset and extra byte in QUERY_INFO I guess having the length > set to 0 makes it ok... I've sent a couple of patches to Wireshark to > fix the packet parsing. It now accepts the empty filename in CREATE (no > more "[unknown]"), which enables the proper parsing of the unknown > field. Thanks for looking into this! Hadrien -- 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
2017-10-17 5:47 GMT-07:00 Aurelien Aptel <aaptel@suse.com>: > query_info() doesn't use the InputBuffer field of the QUERY_INFO > request, therefore according to [MS-SMB2] it must: > > a) set the InputBufferOffset to 0 > b) send a zero-length InputBuffer > > Doing a) is trivial but b) is a bit more tricky. > > The packet is allocated according to it's StructureSize, which takes > into account an extra 1 byte buffer which we don't need > here. StructureSize fields must have constant values no matter the > actual length of the whole packet so we can't just edit that constant. > > Both the NetBIOS-over-TCP message length ("rfc1002 length") L and the > iovec length L' have to be updated. Since L' is computed from L we > just update L by decrementing it by one. > > Signed-off-by: Aurelien Aptel <aaptel@suse.com> > --- > fs/cifs/smb2pdu.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 6f0e6343c15e..b927e131f997 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -2191,9 +2191,13 @@ query_info(const unsigned int xid, struct cifs_tcon *tcon, > req->PersistentFileId = persistent_fid; > req->VolatileFileId = volatile_fid; > req->AdditionalInformation = cpu_to_le32(additional_info); > - /* 4 for rfc1002 length field and 1 for Buffer */ > - req->InputBufferOffset = > - cpu_to_le16(sizeof(struct smb2_query_info_req) - 1 - 4); > + > + /* > + * We do not use the input buffer (do not send extra byte) > + */ > + req->InputBufferOffset = 0; > + inc_rfc1001_len(req, -1); > + I was looking at the code and noticed that build_qfs_info_req() uses the same pattern of initializing InputBufferOffset field. Do we need to fix it in the same way? -- Best regards, Pavel Shilovsky -- 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 6f0e6343c15e..b927e131f997 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2191,9 +2191,13 @@ query_info(const unsigned int xid, struct cifs_tcon *tcon, req->PersistentFileId = persistent_fid; req->VolatileFileId = volatile_fid; req->AdditionalInformation = cpu_to_le32(additional_info); - /* 4 for rfc1002 length field and 1 for Buffer */ - req->InputBufferOffset = - cpu_to_le16(sizeof(struct smb2_query_info_req) - 1 - 4); + + /* + * We do not use the input buffer (do not send extra byte) + */ + req->InputBufferOffset = 0; + inc_rfc1001_len(req, -1); + req->OutputBufferLength = cpu_to_le32(output_len); iov[0].iov_base = (char *)req;
query_info() doesn't use the InputBuffer field of the QUERY_INFO request, therefore according to [MS-SMB2] it must: a) set the InputBufferOffset to 0 b) send a zero-length InputBuffer Doing a) is trivial but b) is a bit more tricky. The packet is allocated according to it's StructureSize, which takes into account an extra 1 byte buffer which we don't need here. StructureSize fields must have constant values no matter the actual length of the whole packet so we can't just edit that constant. Both the NetBIOS-over-TCP message length ("rfc1002 length") L and the iovec length L' have to be updated. Since L' is computed from L we just update L by decrementing it by one. Signed-off-by: Aurelien Aptel <aaptel@suse.com> --- fs/cifs/smb2pdu.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)