[PATCH v4 0/5] Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler

[PATCH v4 0/5] Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler

[Zhang Xiaoxu]

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 (5):
  cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message
  ksmbd: Remove the wrong message length check of
    FSCTL_VALIDATE_NEGOTIATE_INFO
  ksmbd: Fix wrong return value in smb2_ioctl() when wrong out_buf_len
  cifs: Add neg dialects info to smb version values
  cifs: Refactor dialects in validate_negotiate_info_req to variable
    array

 fs/cifs/cifsglob.h        |  2 ++
 fs/cifs/smb2ops.c         | 35 ++++++++++++++++++++++
 fs/cifs/smb2pdu.c         | 61 +++++++--------------------------------
 fs/ksmbd/smb2pdu.c        | 11 ++++---
 fs/smbfs_common/smb2pdu.h |  3 +-
 5 files changed, 54 insertions(+), 58 deletions(-)

-- 
2.31.1

[PATCH v4 1/5] 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>
---
 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;
 	}
 
 	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
-- 
2.31.1

[PATCH v4 2/5] ksmbd: Remove the wrong message length check of FSCTL_VALIDATE_NEGOTIATE_INFO

[Zhang Xiaoxu]

The struct validate_negotiate_info_req change from variable-length array
to reguler array, but the message length check is unchanged.

The fsctl_validate_negotiate_info() already check the message length, so
remove it from smb2_ioctl().

Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and move its struct to smbfs_common")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Cc: <stable@vger.kernel.org>
---
 fs/ksmbd/smb2pdu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index c49f65146ab3..c9f400bbb814 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7640,9 +7640,6 @@ int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
-		if (in_buf_len < sizeof(struct validate_negotiate_info_req))
-			return -EINVAL;
-
 		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
 			return -EINVAL;
 
-- 
2.31.1

[PATCH v4 3/5] ksmbd: Fix wrong return value in smb2_ioctl() when wrong out_buf_len

[Zhang Xiaoxu]

When the out_buf_len is less than the size of struct
validate_negotiate_info_rsp, 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>
Cc: <stable@vger.kernel.org>
---
 fs/ksmbd/smb2pdu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index c9f400bbb814..7da0ec466887 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7640,8 +7640,10 @@ int smb2_ioctl(struct ksmbd_work *work)
 			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 v4 4/5] cifs: Add neg dialects info to smb version values

[Zhang Xiaoxu]

The dialects information when negotiate with server is
depends on the smb version, add it to the version values
and make code simple.

Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
---
 fs/cifs/cifsglob.h |  2 ++
 fs/cifs/smb2ops.c  | 35 ++++++++++++++++++++++++++++
 fs/cifs/smb2pdu.c  | 58 +++++++---------------------------------------
 3 files changed, 46 insertions(+), 49 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index ae7f571a7dba..376421b63738 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -553,6 +553,8 @@ struct smb_version_values {
 	__u16		signing_enabled;
 	__u16		signing_required;
 	size_t		create_lease_size;
+	int		neg_dialect_cnt;
+	__le16		*neg_dialects;
 };
 
 #define HEADER_SIZE(server) (server->vals->header_size)
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 421be43af425..3df330806490 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -5664,6 +5664,12 @@ struct smb_version_values smb21_values = {
 	.create_lease_size = sizeof(struct create_lease),
 };
 
+__le16 smb3any_neg_dialects[] = {
+	cpu_to_le16(SMB30_PROT_ID),
+	cpu_to_le16(SMB302_PROT_ID),
+	cpu_to_le16(SMB311_PROT_ID)
+};
+
 struct smb_version_values smb3any_values = {
 	.version_string = SMB3ANY_VERSION_STRING,
 	.protocol_id = SMB302_PROT_ID, /* doesn't matter, send protocol array */
@@ -5683,6 +5689,15 @@ struct smb_version_values smb3any_values = {
 	.signing_enabled = SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
 	.signing_required = SMB2_NEGOTIATE_SIGNING_REQUIRED,
 	.create_lease_size = sizeof(struct create_lease_v2),
+	.neg_dialect_cnt = ARRAY_SIZE(smb3any_neg_dialects),
+	.neg_dialects = smb3any_neg_dialects,
+};
+
+__le16 smbdefault_neg_dialects[] = {
+	cpu_to_le16(SMB21_PROT_ID),
+	cpu_to_le16(SMB30_PROT_ID),
+	cpu_to_le16(SMB302_PROT_ID),
+	cpu_to_le16(SMB311_PROT_ID)
 };
 
 struct smb_version_values smbdefault_values = {
@@ -5704,6 +5719,12 @@ struct smb_version_values smbdefault_values = {
 	.signing_enabled = SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
 	.signing_required = SMB2_NEGOTIATE_SIGNING_REQUIRED,
 	.create_lease_size = sizeof(struct create_lease_v2),
+	.neg_dialect_cnt = ARRAY_SIZE(smbdefault_neg_dialects),
+	.neg_dialects = smbdefault_neg_dialects,
+};
+
+__le16 smb30_neg_dialects[] = {
+	cpu_to_le16(SMB30_PROT_ID),
 };
 
 struct smb_version_values smb30_values = {
@@ -5725,6 +5746,12 @@ struct smb_version_values smb30_values = {
 	.signing_enabled = SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
 	.signing_required = SMB2_NEGOTIATE_SIGNING_REQUIRED,
 	.create_lease_size = sizeof(struct create_lease_v2),
+	.neg_dialect_cnt = ARRAY_SIZE(smb30_neg_dialects),
+	.neg_dialects = smb30_neg_dialects,
+};
+
+__le16 smb302_neg_dialects[] = {
+	cpu_to_le16(SMB302_PROT_ID),
 };
 
 struct smb_version_values smb302_values = {
@@ -5746,6 +5773,12 @@ struct smb_version_values smb302_values = {
 	.signing_enabled = SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
 	.signing_required = SMB2_NEGOTIATE_SIGNING_REQUIRED,
 	.create_lease_size = sizeof(struct create_lease_v2),
+	.neg_dialect_cnt = ARRAY_SIZE(smb302_neg_dialects),
+	.neg_dialects = smb302_neg_dialects,
+};
+
+__le16 smb311_neg_dialects[] = {
+	cpu_to_le16(SMB311_PROT_ID),
 };
 
 struct smb_version_values smb311_values = {
@@ -5767,4 +5800,6 @@ struct smb_version_values smb311_values = {
 	.signing_enabled = SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
 	.signing_required = SMB2_NEGOTIATE_SIGNING_REQUIRED,
 	.create_lease_size = sizeof(struct create_lease_v2),
+	.neg_dialect_cnt = ARRAY_SIZE(smb311_neg_dialects),
+	.neg_dialects = smb311_neg_dialects,
 };
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 37f422eb3876..1fbb8ccf1ff6 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -897,27 +897,10 @@ 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;
-	}
+	req->DialectCount = cpu_to_le16(server->vals->neg_dialect_cnt);
+	memcpy(req->Dialects, server->vals->neg_dialects,
+		sizeof(__le16) * server->vals->neg_dialect_cnt);
+	total_len += sizeof(__le16) * server->vals->neg_dialect_cnt;
 
 	/* only one of SMB2 signing flags may be set in SMB2 request */
 	if (ses->sign)
@@ -1143,34 +1126,11 @@ 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;
-	}
+	pneg_inbuf->DialectCount = cpu_to_le16(server->vals->neg_dialect_cnt);
+	memcpy(pneg_inbuf->Dialects, server->vals->neg_dialects,
+		server->vals->neg_dialect_cnt * sizeof(__le16));
+	inbuflen = offsetof(struct validate_negotiate_info_req, Dialects) +
+		sizeof(pneg_inbuf->Dialects[0]) * server->vals->neg_dialect_cnt;
 
 	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
 		FSCTL_VALIDATE_NEGOTIATE_INFO,
-- 
2.31.1

[PATCH v4 5/5] 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, the dialects count is
depending on the smb version, so the dialects should be
variable array.

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

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 1fbb8ccf1ff6..82cd21c26c60 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1105,7 +1105,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) * server->vals->neg_dialect_cnt;
+
+	pneg_inbuf = kmalloc(inbuflen, GFP_NOFS);
 	if (!pneg_inbuf)
 		return -ENOMEM;
 
@@ -1129,8 +1132,6 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 	pneg_inbuf->DialectCount = cpu_to_le16(server->vals->neg_dialect_cnt);
 	memcpy(pneg_inbuf->Dialects, server->vals->neg_dialects,
 		server->vals->neg_dialect_cnt * sizeof(__le16));
-	inbuflen = offsetof(struct validate_negotiate_info_req, Dialects) +
-		sizeof(pneg_inbuf->Dialects[0]) * server->vals->neg_dialect_cnt;
 
 	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
 		FSCTL_VALIDATE_NEGOTIATE_INFO,
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 7da0ec466887..52a524fd2f3b 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7392,7 +7392,7 @@ static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn,
 	int ret = 0;
 	int dialect;
 
-	if (in_buf_len < offsetof(struct validate_negotiate_info_req, Dialects) +
+	if (in_buf_len < sizeof(*neg_req) +
 			le16_to_cpu(neg_req->DialectCount) * sizeof(__le16))
 		return -EINVAL;
 
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 v4 1/5] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message

[Tom Talpey]

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,

Re: [PATCH v4 2/5] ksmbd: Remove the wrong message length check of FSCTL_VALIDATE_NEGOTIATE_INFO

[Tom Talpey]

On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
> The struct validate_negotiate_info_req change from variable-length array
> to reguler array, but the message length check is unchanged.
> 
> The fsctl_validate_negotiate_info() already check the message length, so
> remove it from smb2_ioctl().
> 
> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and move its struct to smbfs_common")
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> Cc: <stable@vger.kernel.org>
> ---
>   fs/ksmbd/smb2pdu.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index c49f65146ab3..c9f400bbb814 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7640,9 +7640,6 @@ int smb2_ioctl(struct ksmbd_work *work)
>   			goto out;
>   		}
>   
> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req))
> -			return -EINVAL;
> -

This actually fixes a different bug, which the comment needs to mention.
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. Which is good.

But there are two other issues now.

1) The code no longer checks that a complete validate negotiate header
is present before dereferencing neg_req->DialectCount. This code will
fetch the DialectCount potentially beyond the end of an invalid short
packet:

   fsctl_validate_negotiate_info():

         if (in_buf_len < offsetof(struct validate_negotiate_info_req, 
Dialects) +
                         le16_to_cpu(neg_req->DialectCount) * 
sizeof(__le16))
                 return -EINVAL;

         dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
                                              neg_req->DialectCount);

2) The DialectCount is an le16, which can be negative. A small invalid
negative value may pass this test and proceed to process the array,
which would be bad. This is an existing issue, but should be fixed
since you now need to correct the test anyway.

Tom.


>   		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
>   			return -EINVAL; >

Re: [PATCH v4 3/5] ksmbd: Fix wrong return value in smb2_ioctl() when wrong out_buf_len

[Tom Talpey]

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

On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
> When the out_buf_len is less than the size of struct
> validate_negotiate_info_rsp, 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>
> Cc: <stable@vger.kernel.org>
> ---
>   fs/ksmbd/smb2pdu.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index c9f400bbb814..7da0ec466887 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7640,8 +7640,10 @@ int smb2_ioctl(struct ksmbd_work *work)
>   			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],

Re: [PATCH v4 4/5] cifs: Add neg dialects info to smb version values

[Tom Talpey]

Is this really necessary? It's nice and all, but there aren't
any new SMB2/3 dialects coming down the pike. I'm ambivalent
about changing the code unless there's an actual issue, for
the purpose of maintaining stable, etc. Steve?

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

On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
> The dialects information when negotiate with server is
> depends on the smb version, add it to the version values
> and make code simple.
> 
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> ---
>   fs/cifs/cifsglob.h |  2 ++
>   fs/cifs/smb2ops.c  | 35 ++++++++++++++++++++++++++++
>   fs/cifs/smb2pdu.c  | 58 +++++++---------------------------------------
>   3 files changed, 46 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index ae7f571a7dba..376421b63738 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -553,6 +553,8 @@ struct smb_version_values {
>   	__u16		signing_enabled;
>   	__u16		signing_required;
>   	size_t		create_lease_size;
> +	int		neg_dialect_cnt;
> +	__le16		*neg_dialects;
>   };
>   
>   #define HEADER_SIZE(server) (server->vals->header_size)
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 421be43af425..3df330806490 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -5664,6 +5664,12 @@ struct smb_version_values smb21_values = {
>   	.create_lease_size = sizeof(struct create_lease),
>   };
>   
> +__le16 smb3any_neg_dialects[] = {
> +	cpu_to_le16(SMB30_PROT_ID),
> +	cpu_to_le16(SMB302_PROT_ID),
> +	cpu_to_le16(SMB311_PROT_ID)
> +};
> +
>   struct smb_version_values smb3any_values = {
>   	.version_string = SMB3ANY_VERSION_STRING,
>   	.protocol_id = SMB302_PROT_ID, /* doesn't matter, send protocol array */
> @@ -5683,6 +5689,15 @@ struct smb_version_values smb3any_values = {
>   	.signing_enabled = SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
>   	.signing_required = SMB2_NEGOTIATE_SIGNING_REQUIRED,
>   	.create_lease_size = sizeof(struct create_lease_v2),
> +	.neg_dialect_cnt = ARRAY_SIZE(smb3any_neg_dialects),
> +	.neg_dialects = smb3any_neg_dialects,
> +};
> +
> +__le16 smbdefault_neg_dialects[] = {
> +	cpu_to_le16(SMB21_PROT_ID),
> +	cpu_to_le16(SMB30_PROT_ID),
> +	cpu_to_le16(SMB302_PROT_ID),
> +	cpu_to_le16(SMB311_PROT_ID)
>   };
>   
>   struct smb_version_values smbdefault_values = {
> @@ -5704,6 +5719,12 @@ struct smb_version_values smbdefault_values = {
>   	.signing_enabled = SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
>   	.signing_required = SMB2_NEGOTIATE_SIGNING_REQUIRED,
>   	.create_lease_size = sizeof(struct create_lease_v2),
> +	.neg_dialect_cnt = ARRAY_SIZE(smbdefault_neg_dialects),
> +	.neg_dialects = smbdefault_neg_dialects,
> +};
> +
> +__le16 smb30_neg_dialects[] = {
> +	cpu_to_le16(SMB30_PROT_ID),
>   };
>   
>   struct smb_version_values smb30_values = {
> @@ -5725,6 +5746,12 @@ struct smb_version_values smb30_values = {
>   	.signing_enabled = SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
>   	.signing_required = SMB2_NEGOTIATE_SIGNING_REQUIRED,
>   	.create_lease_size = sizeof(struct create_lease_v2),
> +	.neg_dialect_cnt = ARRAY_SIZE(smb30_neg_dialects),
> +	.neg_dialects = smb30_neg_dialects,
> +};
> +
> +__le16 smb302_neg_dialects[] = {
> +	cpu_to_le16(SMB302_PROT_ID),
>   };
>   
>   struct smb_version_values smb302_values = {
> @@ -5746,6 +5773,12 @@ struct smb_version_values smb302_values = {
>   	.signing_enabled = SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
>   	.signing_required = SMB2_NEGOTIATE_SIGNING_REQUIRED,
>   	.create_lease_size = sizeof(struct create_lease_v2),
> +	.neg_dialect_cnt = ARRAY_SIZE(smb302_neg_dialects),
> +	.neg_dialects = smb302_neg_dialects,
> +};
> +
> +__le16 smb311_neg_dialects[] = {
> +	cpu_to_le16(SMB311_PROT_ID),
>   };
>   
>   struct smb_version_values smb311_values = {
> @@ -5767,4 +5800,6 @@ struct smb_version_values smb311_values = {
>   	.signing_enabled = SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
>   	.signing_required = SMB2_NEGOTIATE_SIGNING_REQUIRED,
>   	.create_lease_size = sizeof(struct create_lease_v2),
> +	.neg_dialect_cnt = ARRAY_SIZE(smb311_neg_dialects),
> +	.neg_dialects = smb311_neg_dialects,
>   };
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 37f422eb3876..1fbb8ccf1ff6 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -897,27 +897,10 @@ 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;
> -	}
> +	req->DialectCount = cpu_to_le16(server->vals->neg_dialect_cnt);
> +	memcpy(req->Dialects, server->vals->neg_dialects,
> +		sizeof(__le16) * server->vals->neg_dialect_cnt);
> +	total_len += sizeof(__le16) * server->vals->neg_dialect_cnt;
>   
>   	/* only one of SMB2 signing flags may be set in SMB2 request */
>   	if (ses->sign)
> @@ -1143,34 +1126,11 @@ 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;
> -	}
> +	pneg_inbuf->DialectCount = cpu_to_le16(server->vals->neg_dialect_cnt);
> +	memcpy(pneg_inbuf->Dialects, server->vals->neg_dialects,
> +		server->vals->neg_dialect_cnt * sizeof(__le16));
> +	inbuflen = offsetof(struct validate_negotiate_info_req, Dialects) +
> +		sizeof(pneg_inbuf->Dialects[0]) * server->vals->neg_dialect_cnt;
>   
>   	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
>   		FSCTL_VALIDATE_NEGOTIATE_INFO,

Re: [PATCH v4 5/5] cifs: Refactor dialects in validate_negotiate_info_req to variable array

[Tom Talpey]

On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
> The length of the message FSCTL_VALIDATE_NEGOTIATE_INFO is
> depends on the count of the dialects, the dialects count is
> depending on the smb version, so the dialects should be
> variable array.
> 
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> ---
>   fs/cifs/smb2pdu.c         | 7 ++++---
>   fs/ksmbd/smb2pdu.c        | 2 +-
>   fs/smbfs_common/smb2pdu.h | 3 +--
>   3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 1fbb8ccf1ff6..82cd21c26c60 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1105,7 +1105,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) * server->vals->neg_dialect_cnt;

Good!

> +
> +	pneg_inbuf = kmalloc(inbuflen, GFP_NOFS);
>   	if (!pneg_inbuf)
>   		return -ENOMEM;
>   
> @@ -1129,8 +1132,6 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>   	pneg_inbuf->DialectCount = cpu_to_le16(server->vals->neg_dialect_cnt);
>   	memcpy(pneg_inbuf->Dialects, server->vals->neg_dialects,
>   		server->vals->neg_dialect_cnt * sizeof(__le16));
> -	inbuflen = offsetof(struct validate_negotiate_info_req, Dialects) +
> -		sizeof(pneg_inbuf->Dialects[0]) * server->vals->neg_dialect_cnt;
>   
>   	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
>   		FSCTL_VALIDATE_NEGOTIATE_INFO,
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 7da0ec466887..52a524fd2f3b 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7392,7 +7392,7 @@ static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn,
>   	int ret = 0;
>   	int dialect;
>   
> -	if (in_buf_len < offsetof(struct validate_negotiate_info_req, Dialects) +
> +	if (in_buf_len < sizeof(*neg_req) +
>   			le16_to_cpu(neg_req->DialectCount) * sizeof(__le16))

This has the same issue as I mention in the patch 3 comment, it will
potentially dereference a non-present DialectCount field, and may
perform calculation on a negative value.

>   		return -EINVAL;
>   
> 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 {

Re: [PATCH v4 3/5] ksmbd: Fix wrong return value in smb2_ioctl() when wrong out_buf_len

[Namjae Jeon]

2022-09-02 22:29 GMT+09:00, Tom Talpey <tom@talpey.com>:
> Reviewed-by: Tom Talpey <tom@talpey.com>
>
> On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
>> When the out_buf_len is less than the size of struct
>> validate_negotiate_info_rsp, 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>
>> Cc: <stable@vger.kernel.org>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Zhang, Can you add cc me on next-spin ?

Re: [PATCH v4 2/5] ksmbd: Remove the wrong message length check of FSCTL_VALIDATE_NEGOTIATE_INFO

[Namjae Jeon]

2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
>> The struct validate_negotiate_info_req change from variable-length array
>> to reguler array, but the message length check is unchanged.
>>
>> The fsctl_validate_negotiate_info() already check the message length, so
>> remove it from smb2_ioctl().
>>
>> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
I think the fixes tag is wrong. Isn't the below correct?
Fixes: f7db8fd03a4bc ("ksmbd: add validation in smb2_ioctl")

>> move its struct to smbfs_common")
>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>>   fs/ksmbd/smb2pdu.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index c49f65146ab3..c9f400bbb814 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -7640,9 +7640,6 @@ int smb2_ioctl(struct ksmbd_work *work)
>>   			goto out;
>>   		}
>>
>> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req))
>> -			return -EINVAL;
>> -
>
> This actually fixes a different bug, which the comment needs to mention.
> 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. Which is good.
>
> But there are two other issues now.
>
> 1) The code no longer checks that a complete validate negotiate header
> is present before dereferencing neg_req->DialectCount. This code will
> fetch the DialectCount potentially beyond the end of an invalid short
> packet:
>
>    fsctl_validate_negotiate_info():
>
>          if (in_buf_len < offsetof(struct validate_negotiate_info_req,
> Dialects) +
>                          le16_to_cpu(neg_req->DialectCount) *
> sizeof(__le16))
>                  return -EINVAL;
>
>          dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
>                                               neg_req->DialectCount);
>
> 2) The DialectCount is an le16, which can be negative. A small invalid
> negative value may pass this test and proceed to process the array,
> which would be bad. This is an existing issue, but should be fixed
> since you now need to correct the test anyway.
>
> Tom.
>
>
>>   		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
>>   			return -EINVAL; >
>

Re: [PATCH v4 2/5] ksmbd: Remove the wrong message length check of FSCTL_VALIDATE_NEGOTIATE_INFO

[huaweicloud]

在 2022/9/2 22:47, Namjae Jeon 写道:
> 2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@talpey.com>:
>> On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
>>> The struct validate_negotiate_info_req change from variable-length array
>>> to reguler array, but the message length check is unchanged.
>>>
>>> The fsctl_validate_negotiate_info() already check the message length, so
>>> remove it from smb2_ioctl().
>>>
>>> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
> I think the fixes tag is wrong. Isn't the below correct?
> Fixes: f7db8fd03a4bc ("ksmbd: add validation in smb2_ioctl")
> 
>>> move its struct to smbfs_common")
>>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>>    fs/ksmbd/smb2pdu.c | 3 ---
>>>    1 file changed, 3 deletions(-)
>>>
>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>> index c49f65146ab3..c9f400bbb814 100644
>>> --- a/fs/ksmbd/smb2pdu.c
>>> +++ b/fs/ksmbd/smb2pdu.c
>>> @@ -7640,9 +7640,6 @@ int smb2_ioctl(struct ksmbd_work *work)
>>>    			goto out;
>>>    		}
>>>
>>> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req))
>>> -			return -EINVAL;
>>> -
>>
>> This actually fixes a different bug, which the comment needs to mention.
>> 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. Which is good.
>>
>> But there are two other issues now.
>>
>> 1) The code no longer checks that a complete validate negotiate header
>> is present before dereferencing neg_req->DialectCount. This code will
>> fetch the DialectCount potentially beyond the end of an invalid short
>> packet:
>>
>>     fsctl_validate_negotiate_info():
>>
>>           if (in_buf_len < offsetof(struct validate_negotiate_info_req,
>> Dialects) +
>>                           le16_to_cpu(neg_req->DialectCount) *
>> sizeof(__le16))
>>                   return -EINVAL;
>>
>>           dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
>>                                                neg_req->DialectCount);
>>
>> 2) The DialectCount is an le16, which can be negative. A small invalid
>> negative value may pass this test and proceed to process the array,
>> which would be bad. This is an existing issue, but should be fixed
>> since you now need to correct the test anyway.
>>
>> Tom.
>>
>>
>>>    		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
>>>    			return -EINVAL; >
Thanks Tom.

I will send next version.

In smb2_ioctl, validate the in_buf_len with offsetof(Dialects),
in fsctl_validate_negotiate_info, validate the DialectCount
>>

Re: [PATCH v4 2/5] ksmbd: Remove the wrong message length check of FSCTL_VALIDATE_NEGOTIATE_INFO

[zhangxiaoxu (A)]

在 2022/9/2 22:47, Namjae Jeon 写道:
> 2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@talpey.com>:
>> On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
>>> The struct validate_negotiate_info_req change from variable-length array
>>> to reguler array, but the message length check is unchanged.
>>>
>>> The fsctl_validate_negotiate_info() already check the message length, so
>>> remove it from smb2_ioctl().
>>>
>>> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
> I think the fixes tag is wrong. Isn't the below correct?
> Fixes: f7db8fd03a4bc ("ksmbd: add validation in smb2_ioctl")

Before commit c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
move its struct to smbfs_common"), the struct defined in fs/ksmbd/smb2pdu.h:

   struct validate_negotiate_info_req {
          __le32 Capabilities;
          __u8   Guid[SMB2_CLIENT_GUID_SIZE];
          __le16 SecurityMode;
          __le16 DialectCount;
          __le16 Dialects[1]; /* dialect (someday maybe list) client asked for */
   } __packed;

After this commit, the struct defined in fs/smbfs_common/smb2pdu.h:
   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 */
   } __packed;

The 'Dialects' array length from 1 change to 4.

Before commit c7803b05f74b, the message should contain at lease 1 dialects,
but after this commit, it changed to should contain at lease 4 dialects.

So the fixes tag should be c7803b05f74b.

> 
>>> move its struct to smbfs_common")
>>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>>    fs/ksmbd/smb2pdu.c | 3 ---
>>>    1 file changed, 3 deletions(-)
>>>
>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>> index c49f65146ab3..c9f400bbb814 100644
>>> --- a/fs/ksmbd/smb2pdu.c
>>> +++ b/fs/ksmbd/smb2pdu.c
>>> @@ -7640,9 +7640,6 @@ int smb2_ioctl(struct ksmbd_work *work)
>>>    			goto out;
>>>    		}
>>>
>>> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req))
>>> -			return -EINVAL;
>>> -
>>
>> This actually fixes a different bug, which the comment needs to mention.
>> 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. Which is good.
>>
>> But there are two other issues now.
>>
>> 1) The code no longer checks that a complete validate negotiate header
>> is present before dereferencing neg_req->DialectCount. This code will
>> fetch the DialectCount potentially beyond the end of an invalid short
>> packet:
>>
>>     fsctl_validate_negotiate_info():
>>
>>           if (in_buf_len < offsetof(struct validate_negotiate_info_req,
>> Dialects) +
>>                           le16_to_cpu(neg_req->DialectCount) *
>> sizeof(__le16))
>>                   return -EINVAL;
>>
>>           dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
>>                                                neg_req->DialectCount);
>>
>> 2) The DialectCount is an le16, which can be negative. A small invalid
>> negative value may pass this test and proceed to process the array,
>> which would be bad. This is an existing issue, but should be fixed
>> since you now need to correct the test anyway.
>>
>> Tom.
>>
>>
>>>    		if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
>>>    			return -EINVAL; >
>>

Re: [PATCH v4 2/5] ksmbd: Remove the wrong message length check of FSCTL_VALIDATE_NEGOTIATE_INFO

[Namjae Jeon]

2022-09-05 11:29 GMT+09:00, zhangxiaoxu (A) <zhangxiaoxu5@huawei.com>:
>
>
> 在 2022/9/2 22:47, Namjae Jeon 写道:
>> 2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@talpey.com>:
>>> On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
>>>> The struct validate_negotiate_info_req change from variable-length
>>>> array
>>>> to reguler array, but the message length check is unchanged.
>>>>
>>>> The fsctl_validate_negotiate_info() already check the message length,
>>>> so
>>>> remove it from smb2_ioctl().
>>>>
>>>> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break,
>>>> and
>> I think the fixes tag is wrong. Isn't the below correct?
>> Fixes: f7db8fd03a4bc ("ksmbd: add validation in smb2_ioctl")
>
> Before commit c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break,
> and
> move its struct to smbfs_common"), the struct defined in
> fs/ksmbd/smb2pdu.h:
>
>    struct validate_negotiate_info_req {
>           __le32 Capabilities;
>           __u8   Guid[SMB2_CLIENT_GUID_SIZE];
>           __le16 SecurityMode;
>           __le16 DialectCount;
>           __le16 Dialects[1]; /* dialect (someday maybe list) client asked
> for */
>    } __packed;
>
> After this commit, the struct defined in fs/smbfs_common/smb2pdu.h:
>    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 */
>    } __packed;
>
> The 'Dialects' array length from 1 change to 4.
Okay, Do you think that your patch is not needed without commit c7803b05f74b ?
I understood that the InputCount check doesn't take the DialectCount
into account and it's already a duplicate check, So you try to remove
that.

>
> Before commit c7803b05f74b, the message should contain at lease 1 dialects,
> but after this commit, it changed to should contain at lease 4 dialects.
>
> So the fixes tag should be c7803b05f74b.

Re: [PATCH v4 2/5] ksmbd: Remove the wrong message length check of FSCTL_VALIDATE_NEGOTIATE_INFO

[zhangxiaoxu (A)]

在 2022/9/2 21:28, Tom Talpey 写道:
> On 9/1/2022 10:24 AM, Zhang Xiaoxu wrote:
>> The struct validate_negotiate_info_req change from variable-length array
>> to reguler array, but the message length check is unchanged.
>>
>> The fsctl_validate_negotiate_info() already check the message length, so
>> remove it from smb2_ioctl().
>>
>> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and move its struct to smbfs_common")
>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>>   fs/ksmbd/smb2pdu.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index c49f65146ab3..c9f400bbb814 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -7640,9 +7640,6 @@ int smb2_ioctl(struct ksmbd_work *work)
>>               goto out;
>>           }
>> -        if (in_buf_len < sizeof(struct validate_negotiate_info_req))
>> -            return -EINVAL;
>> -
> 
> This actually fixes a different bug, which the comment needs to mention.
> 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. Which is good.
> 
> But there are two other issues now.
> 
> 1) The code no longer checks that a complete validate negotiate header
> is present before dereferencing neg_req->DialectCount. This code will
> fetch the DialectCount potentially beyond the end of an invalid short
> packet:
Yes, if no judgement the length before dereferencing 'DialectCount',
OOB read will occur.>
>    fsctl_validate_negotiate_info():
> 
>          if (in_buf_len < offsetof(struct validate_negotiate_info_req, Dialects) +
>                          le16_to_cpu(neg_req->DialectCount) * sizeof(__le16))
>                  return -EINVAL;
> 
>          dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
>                                               neg_req->DialectCount);
> 
> 2) The DialectCount is an le16, which can be negative. A small invalid
After precompile the smb2pdu.c:
   typedef unsigned short __u16;     # include/uapi/asm-generic/int-ll64.h
   typedef __u16 __le16;             # include/uapi/linux/types.h

   struct validate_negotiate_info_req {
    __le32 Capabilities;
    __u8 Guid[16];
    __le16 SecurityMode;
    __le16 DialectCount;
    __le16 Dialects[];
   } __attribute__((__packed__));

The DialectCount is unsigned, can't be negative.

Even the 'DialectCount' is big enough to USHRT_MAX(65535), according
integer promotions, it also can't be overflow to negative since the
sizeof(__le16) is only 2.

So, I think the expression is correct now.
> negative value may pass this test and proceed to process the array,
> which would be bad. This is an existing issue, but should be fixed
> since you now need to correct the test anyway.
> 
> Tom.
> 
> 
>>           if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
>>               return -EINVAL; >