| Message ID | 20171017124717.25955-1-aaptel@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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(-)