diff mbox series

[v4,1/5] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message

Message ID 20220901142413.3351804-2-zhangxiaoxu5@huawei.com
State New
Headers show
Series Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler | expand

Commit Message

zhangxiaoxu (A) Sept. 1, 2022, 2:24 p.m. UTC
Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list")
extend the dialects from 3 to 4, but forget to decrease the extended
length when specific the dialect, then the message length is larger
than expected.

This maybe leak some info through network because not initialize the
message body.

After apply this patch, the VALIDATE_NEGOTIATE_INFO message length is
reduced from 28 bytes to 26 bytes.

Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Cc: <stable@vger.kernel.org>
---
 fs/cifs/smb2pdu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Tom Talpey Sept. 2, 2022, 12:52 p.m. UTC | #1
I guess this is good for stable:, but the code is obsoleted just
a few patches down. I do have one comment below.

On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
> Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list")
> extend the dialects from 3 to 4, but forget to decrease the extended
> length when specific the dialect, then the message length is larger
> than expected.
> 
> This maybe leak some info through network because not initialize the
> message body.
> 
> After apply this patch, the VALIDATE_NEGOTIATE_INFO message length is
> reduced from 28 bytes to 26 bytes.
> 
> Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list")
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> Cc: <stable@vger.kernel.org>
> ---
>   fs/cifs/smb2pdu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 128e44e57528..37f422eb3876 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1167,9 +1167,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>   		pneg_inbuf->Dialects[0] =
>   			cpu_to_le16(server->vals->protocol_id);
>   		pneg_inbuf->DialectCount = cpu_to_le16(1);
> -		/* structure is big enough for 3 dialects, sending only 1 */
> +		/* structure is big enough for 4 dialects, sending only 1 */
>   		inbuflen = sizeof(*pneg_inbuf) -
> -				sizeof(pneg_inbuf->Dialects[0]) * 2;
> +				sizeof(pneg_inbuf->Dialects[0]) * 3;

This can be coded much more defensively by expressing as

	inbuflen = offsetof(pneg_inbuf->Dialects[1])

No need to repeat the "big enough for 4", either. Just drop the comment.

Reviewed-by: Tom Talpey <tom@talpey.com>

>   	}
>   
>   	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
diff mbox series

Patch

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 128e44e57528..37f422eb3876 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1167,9 +1167,9 @@  int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 		pneg_inbuf->Dialects[0] =
 			cpu_to_le16(server->vals->protocol_id);
 		pneg_inbuf->DialectCount = cpu_to_le16(1);
-		/* structure is big enough for 3 dialects, sending only 1 */
+		/* structure is big enough for 4 dialects, sending only 1 */
 		inbuflen = sizeof(*pneg_inbuf) -
-				sizeof(pneg_inbuf->Dialects[0]) * 2;
+				sizeof(pneg_inbuf->Dialects[0]) * 3;
 	}
 
 	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,