CIFS: do not send invalid input buffer on QUERY_INFO requests
diff mbox

Message ID 20171017124717.25955-1-aaptel@suse.com
State New
Headers show

Commit Message

Aurelien Aptel Oct. 17, 2017, 12:47 p.m. UTC
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(-)

Comments

Hadrien Grasland Oct. 17, 2017, 1:41 p.m. UTC | #1
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;
Aurelien Aptel Oct. 18, 2017, 2:50 p.m. UTC | #2
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.
Steve French Oct. 18, 2017, 4:53 p.m. UTC | #3
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
>
Hadrien Grasland Oct. 18, 2017, 8:20 p.m. UTC | #4
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
Pavel Shilovsky Nov. 21, 2017, 1:10 a.m. UTC | #5
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

Patch
diff mbox

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;