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

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

[Zhang Xiaoxu]

v7->v8: update the commit message and smb2_ioctl just ensure the
	DialectCount in the payload.
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         | 94 ++++++++++++++++++---------------------
 fs/ksmbd/smb2pdu.c        | 13 ++++--
 fs/smbfs_common/smb2pdu.h |  3 +-
 3 files changed, 54 insertions(+), 56 deletions(-)

-- 
2.31.1

[PATCH v8 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 v8 2/3] ksmbd: Fix wrong return value and message length check in smb2_ioctl()

[Zhang Xiaoxu]

Commit c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock
break, and move its struct to smbfs_common") use the defination
of 'struct validate_negotiate_info_req' in smbfs_common, the
array length of 'Dialects' changed from 1 to 4, but the protocol
does not require the client to send all 4. This lead the request
which satisfied with protocol and server to fail.

So just ensure the request payload has the 'DialectCount' in
smb2_ioctl(), then fsctl_validate_negotiate_info() will use it
to validate the payload length and each dialect.

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

Fixes: f7db8fd03a4b ("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..22dc2facac8a 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)) {
+			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 v8 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 v8 2/3] ksmbd: Fix wrong return value and message length check in smb2_ioctl()

[Namjae Jeon]

2022-09-26 12:36 GMT+09:00, Zhang Xiaoxu <zhangxiaoxu5@huawei.com>:
> Commit c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock
> break, and move its struct to smbfs_common") use the defination
> of 'struct validate_negotiate_info_req' in smbfs_common, the
> array length of 'Dialects' changed from 1 to 4, but the protocol
> does not require the client to send all 4. This lead the request
> which satisfied with protocol and server to fail.
>
> So just ensure the request payload has the 'DialectCount' in
> smb2_ioctl(), then fsctl_validate_negotiate_info() will use it
> to validate the payload length and each dialect.
>
> Also when the {in, out}_buf_len is less than the required, should
> goto out to initialize the status in the response header.
>
> Fixes: f7db8fd03a4b ("ksmbd: add validation in smb2_ioctl")
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks!

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

[Steve French]

merged into cifs-2.6.git for-next  (waiting on additional
review/testing of patch 3 in the series before merging that)

On Sun, Sep 25, 2022 at 9:35 PM Zhang Xiaoxu <zhangxiaoxu5@huawei.com> 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>
> 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
>


-- 
Thanks,

Steve

Re: [PATCH v8 0/3] Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler

[Paulo Alcantara]

Zhang Xiaoxu <zhangxiaoxu5@huawei.com> writes:

> v7->v8: update the commit message and smb2_ioctl just ensure the
> 	DialectCount in the payload.
> 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         | 94 ++++++++++++++++++---------------------
>  fs/ksmbd/smb2pdu.c        | 13 ++++--
>  fs/smbfs_common/smb2pdu.h |  3 +-
>  3 files changed, 54 insertions(+), 56 deletions(-)

Acked-by: Paulo Alcantara (SUSE) <pc@cjr.nz>