[PATCH v7 0/3] Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler

[PATCH v7 0/3] Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler

[Zhang Xiaoxu]

v6->v7: squash 2 and 3; use helper function to get the dialect count
v5->v6: use 'static' for 'smbx_neg_dialects'
v4->v5: reorder the patch; 
	add check in smb2_ioctl() to ensure no oob read to DialectCount
v3->v4: Fix the wrong sizeof validate_negotiate_info_req in ksmbd
v2->v3: refactor the dialects in struct validate_negotiate_info_req to
	variable array
v1->v2: fix some bug in ksmbd when handle FSCTL_VALIDATE_NEGOTIATE_INFO
	message

Zhang Xiaoxu (3):
  cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message
  ksmbd: Fix wrong return value and message length check in smb2_ioctl()
  cifs: Refactor dialects in validate_negotiate_info_req to variable
    array

 fs/cifs/smb2pdu.c         | 97 +++++++++++++++++++--------------------
 fs/ksmbd/smb2pdu.c        | 13 ++++--
 fs/smbfs_common/smb2pdu.h |  3 +-
 3 files changed, 57 insertions(+), 56 deletions(-)

-- 
2.31.1

[PATCH v7 1/3] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message

[Zhang Xiaoxu]

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>
Reviewed-by: Tom Talpey <tom@talpey.com>
---
 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 40da444c46b4..90ccac18f9f3 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1169,9 +1169,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,
-- 
2.31.1

[PATCH v7 2/3] ksmbd: Fix wrong return value and message length check in smb2_ioctl()

[Zhang Xiaoxu]

The structure size includes 4 dialect slots, but the protocol does not
require the client to send all 4. So this allows the negotiation to not
fail.

Also when the {in, out}_buf_len is less than the required, should goto
out to initialize the status in the response header.

Fixes: f7db8fd03a4bc ("ksmbd: add validation in smb2_ioctl")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
---
 fs/ksmbd/smb2pdu.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 15c487aa19ad..d8ef50530a73 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7638,11 +7638,16 @@ int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
-		if (in_buf_len < sizeof(struct validate_negotiate_info_req))
-			return -EINVAL;
+		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
+					  Dialects[1])) {
+			ret = -EINVAL;
+			goto out;
+		}
 
-		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
-			return -EINVAL;
+		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp)) {
+			ret = -EINVAL;
+			goto out;
+		}
 
 		ret = fsctl_validate_negotiate_info(conn,
 			(struct validate_negotiate_info_req *)&req->Buffer[0],
-- 
2.31.1

[PATCH v7 3/3] cifs: Refactor dialects in validate_negotiate_info_req to variable array

[Zhang Xiaoxu]

The length of the message FSCTL_VALIDATE_NEGOTIATE_INFO is
depends on the count of the dialects, redefine the dialects
to variable array.

Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
---
 fs/cifs/smb2pdu.c         | 94 ++++++++++++++++++---------------------
 fs/smbfs_common/smb2pdu.h |  3 +-
 2 files changed, 45 insertions(+), 52 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 90ccac18f9f3..d0340af845e9 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -847,6 +847,41 @@ add_posix_context(struct kvec *iov, unsigned int *num_iovec, umode_t mode)
 	return 0;
 }
 
+static int neg_dialect_cnt(struct TCP_Server_Info *server)
+{
+	if (!strcmp(server->vals->version_string,
+		    SMB3ANY_VERSION_STRING))
+		return 3;
+	else if (!strcmp(server->vals->version_string,
+			 SMBDEFAULT_VERSION_STRING))
+		return 4;
+
+	/* otherwise specific dialect was requested */
+	return 1;
+}
+
+static int
+build_neg_dialects(struct TCP_Server_Info *server, __le16 *dialects)
+{
+	if (!strcmp(server->vals->version_string,
+		    SMB3ANY_VERSION_STRING)) {
+		dialects[0] = cpu_to_le16(SMB30_PROT_ID);
+		dialects[1] = cpu_to_le16(SMB302_PROT_ID);
+		dialects[2] = cpu_to_le16(SMB311_PROT_ID);
+		return 3;
+	} else if (!strcmp(server->vals->version_string,
+			   SMBDEFAULT_VERSION_STRING)) {
+		dialects[0] = cpu_to_le16(SMB21_PROT_ID);
+		dialects[1] = cpu_to_le16(SMB30_PROT_ID);
+		dialects[2] = cpu_to_le16(SMB302_PROT_ID);
+		dialects[3] = cpu_to_le16(SMB311_PROT_ID);
+		return 4;
+	}
+
+	/* otherwise specific dialect was requested */
+	dialects[0] = cpu_to_le16(server->vals->protocol_id);
+	return 1;
+}
 
 /*
  *
@@ -897,27 +932,9 @@ SMB2_negotiate(const unsigned int xid,
 	memset(server->preauth_sha_hash, 0, SMB2_PREAUTH_HASH_SIZE);
 	memset(ses->preauth_sha_hash, 0, SMB2_PREAUTH_HASH_SIZE);
 
-	if (strcmp(server->vals->version_string,
-		   SMB3ANY_VERSION_STRING) == 0) {
-		req->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
-		req->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
-		req->Dialects[2] = cpu_to_le16(SMB311_PROT_ID);
-		req->DialectCount = cpu_to_le16(3);
-		total_len += 6;
-	} else if (strcmp(server->vals->version_string,
-		   SMBDEFAULT_VERSION_STRING) == 0) {
-		req->Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
-		req->Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
-		req->Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
-		req->Dialects[3] = cpu_to_le16(SMB311_PROT_ID);
-		req->DialectCount = cpu_to_le16(4);
-		total_len += 8;
-	} else {
-		/* otherwise send specific dialect */
-		req->Dialects[0] = cpu_to_le16(server->vals->protocol_id);
-		req->DialectCount = cpu_to_le16(1);
-		total_len += 2;
-	}
+	rc = build_neg_dialects(server, req->Dialects);
+	req->DialectCount = cpu_to_le16(rc);
+	total_len += rc * sizeof(req->Dialects[0]);
 
 	/* only one of SMB2 signing flags may be set in SMB2 request */
 	if (ses->sign)
@@ -1124,7 +1141,10 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
 		cifs_tcon_dbg(VFS, "Unexpected null user (anonymous) auth flag sent by server\n");
 
-	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_NOFS);
+	inbuflen = sizeof(*pneg_inbuf) +
+			sizeof(__le16) * neg_dialect_cnt(server);
+
+	pneg_inbuf = kmalloc(inbuflen, GFP_NOFS);
 	if (!pneg_inbuf)
 		return -ENOMEM;
 
@@ -1145,34 +1165,8 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 	else
 		pneg_inbuf->SecurityMode = 0;
 
-
-	if (strcmp(server->vals->version_string,
-		SMB3ANY_VERSION_STRING) == 0) {
-		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
-		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
-		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB311_PROT_ID);
-		pneg_inbuf->DialectCount = cpu_to_le16(3);
-		/* SMB 2.1 not included so subtract one dialect from len */
-		inbuflen = sizeof(*pneg_inbuf) -
-				(sizeof(pneg_inbuf->Dialects[0]));
-	} else if (strcmp(server->vals->version_string,
-		SMBDEFAULT_VERSION_STRING) == 0) {
-		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
-		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
-		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
-		pneg_inbuf->Dialects[3] = cpu_to_le16(SMB311_PROT_ID);
-		pneg_inbuf->DialectCount = cpu_to_le16(4);
-		/* structure is big enough for 4 dialects */
-		inbuflen = sizeof(*pneg_inbuf);
-	} else {
-		/* otherwise specific dialect was requested */
-		pneg_inbuf->Dialects[0] =
-			cpu_to_le16(server->vals->protocol_id);
-		pneg_inbuf->DialectCount = cpu_to_le16(1);
-		/* structure is big enough for 4 dialects, sending only 1 */
-		inbuflen = sizeof(*pneg_inbuf) -
-				sizeof(pneg_inbuf->Dialects[0]) * 3;
-	}
+	rc = build_neg_dialects(server, pneg_inbuf->Dialects);
+	pneg_inbuf->DialectCount = cpu_to_le16(rc);
 
 	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
 		FSCTL_VALIDATE_NEGOTIATE_INFO,
diff --git a/fs/smbfs_common/smb2pdu.h b/fs/smbfs_common/smb2pdu.h
index 2cab413fffee..4780c72e9b3a 100644
--- a/fs/smbfs_common/smb2pdu.h
+++ b/fs/smbfs_common/smb2pdu.h
@@ -1388,13 +1388,12 @@ struct reparse_symlink_data_buffer {
 } __packed;
 
 /* See MS-FSCC 2.1.2.6 and cifspdu.h for struct reparse_posix_data */
-
 struct validate_negotiate_info_req {
 	__le32 Capabilities;
 	__u8   Guid[SMB2_CLIENT_GUID_SIZE];
 	__le16 SecurityMode;
 	__le16 DialectCount;
-	__le16 Dialects[4]; /* BB expand this if autonegotiate > 4 dialects */
+	__le16 Dialects[];
 } __packed;
 
 struct validate_negotiate_info_rsp {
-- 
2.31.1

Re: [PATCH v7 2/3] ksmbd: Fix wrong return value and message length check in smb2_ioctl()

[Namjae Jeon]

2022-09-24 19:52 GMT+09:00, Zhang Xiaoxu <zhangxiaoxu5@huawei.com>:
> The structure size includes 4 dialect slots, but the protocol does not
> require the client to send all 4. So this allows the negotiation to not
> fail.
This comment is a bit vague.
You need to add the description to validate DialectCount before
calling fsctl_validate_negotiate_info(). Because there is the check
using DialectCount
in this function, so we have to validate it first.

>
> Also when the {in, out}_buf_len is less than the required, should goto
> out to initialize the status in the response header.
>
> Fixes: f7db8fd03a4bc ("ksmbd: add validation in smb2_ioctl")
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> ---
>  fs/ksmbd/smb2pdu.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 15c487aa19ad..d8ef50530a73 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7638,11 +7638,16 @@ int smb2_ioctl(struct ksmbd_work *work)
>  			goto out;
>  		}
>
> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req))
> -			return -EINVAL;
> +		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
> +					  Dialects[1])) {
Why did you change it from v6? v6 is the correct fix...

> +			ret = -EINVAL;
> +			goto out;
> +		}
>
> -		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
> -			return -EINVAL;
> +		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
>
>  		ret = fsctl_validate_negotiate_info(conn,
>  			(struct validate_negotiate_info_req *)&req->Buffer[0],
> --
> 2.31.1
>
>