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

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

[Zhang Xiaoxu]

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 (5):
  cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message
  ksmbd: Fix wrong return value in smb2_ioctl()
  ksmbd: Fix FSCTL_VALIDATE_NEGOTIATE_INFO message length check in
    smb2_ioctl()
  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        | 14 +++++----
 fs/smbfs_common/smb2pdu.h |  3 +-
 5 files changed, 58 insertions(+), 57 deletions(-)

-- 
2.31.1

[PATCH v6 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>
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 6352ab32c7e7..223056097b54 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 v6 2/5] ksmbd: Fix wrong return value in smb2_ioctl()

[Zhang Xiaoxu]

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>
Cc: <stable@vger.kernel.org>
---
 fs/ksmbd/smb2pdu.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index c49f65146ab3..b56d7688ccf1 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7640,11 +7640,15 @@ 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 < sizeof(struct validate_negotiate_info_req)) {
+			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 v6 3/5] ksmbd: Fix FSCTL_VALIDATE_NEGOTIATE_INFO 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.

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, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index b56d7688ccf1..09ae601e64f9 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
-		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
+		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
+					  Dialects)) {
 			ret = -EINVAL;
 			goto out;
 		}
-- 
2.31.1

[PATCH v6 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>
Acked-by: Tom Talpey <tom@talpey.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..e1407124c761 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),
 };
 
+static __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,
+};
+
+static __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,
+};
+
+static __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,
+};
+
+static __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,
+};
+
+static __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 223056097b54..482ed480fbc6 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)
@@ -1145,34 +1128,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 v6 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        | 5 ++---
 fs/smbfs_common/smb2pdu.h | 3 +--
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 482ed480fbc6..70a3fce85e7c 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1107,7 +1107,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;
 
@@ -1131,8 +1134,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 09ae601e64f9..aa86f31aa2cd 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;
 
@@ -7640,8 +7640,7 @@ int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
-		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
-					  Dialects)) {
+		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
 			ret = -EINVAL;
 			goto out;
 		}
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 v6 2/5] ksmbd: Fix wrong return value in smb2_ioctl()

[Tom Talpey]

On 9/13/2022 7:17 PM, Zhang Xiaoxu wrote:
> 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>
> Cc: <stable@vger.kernel.org>
> ---
>   fs/ksmbd/smb2pdu.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index c49f65146ab3..b56d7688ccf1 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7640,11 +7640,15 @@ 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 < sizeof(struct validate_negotiate_info_req)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}

In itself, this doesn't really fix the problem of requiring 4 dialects,
because it's still comparing to the incorrect Dialects[4] size. It's
only a fix once the 3/5 patch is applied.

So, I don't think it's appropriate for stable.

If you squash 2 and 3, then ok.

Tom.

> -		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 v6 3/5] ksmbd: Fix FSCTL_VALIDATE_NEGOTIATE_INFO message length check in smb2_ioctl()

[Tom Talpey]

On 9/13/2022 7:17 PM, Zhang Xiaoxu wrote:
> 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.
> 
> 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, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index b56d7688ccf1..09ae601e64f9 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
>   			goto out;
>   		}
>   
> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
> +		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
> +					  Dialects)) {
>   			ret = -EINVAL;
>   			goto out;
>   		}

Looks good, but see previous message regarding squashing this
with patch 2/5.

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

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

[Tom Talpey]

Steve, your opinion on including this? You may have missed the
question I asked earlier in the thread:

> 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? 

Tom.

On 9/13/2022 7:17 PM, 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>
> Acked-by: Tom Talpey <tom@talpey.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..e1407124c761 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),
>   };
>   
> +static __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,
> +};
> +
> +static __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,
> +};
> +
> +static __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,
> +};
> +
> +static __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,
> +};
> +
> +static __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 223056097b54..482ed480fbc6 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)
> @@ -1145,34 +1128,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 v6 5/5] cifs: Refactor dialects in validate_negotiate_info_req to variable array

[Tom Talpey]

On 9/13/2022 7:17 PM, 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        | 5 ++---
>   fs/smbfs_common/smb2pdu.h | 3 +--
>   3 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 482ed480fbc6..70a3fce85e7c 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1107,7 +1107,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;

I think this still needs to check the neg_dialect_cnt field for
sanity. we've established that it's now safe to dereference it,
but if the client sends 0xFFFF as a count, this will allocate
a bit over 128KB, uselessly.

I suggest limiting the dialect count to some sane smaller value,
perhaps 8 is a safe choice.

> +
> +	pneg_inbuf = kmalloc(inbuflen, GFP_NOFS);
>   	if (!pneg_inbuf)
>   		return -ENOMEM;
>   
> @@ -1131,8 +1134,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 09ae601e64f9..aa86f31aa2cd 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;

Here, it may be a valid check without the limit check, because it's
verifying that the message was properly constructed.

Tom.

> @@ -7640,8 +7640,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>   			goto out;
>   		}
>   
> -		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
> -					  Dialects)) {
> +		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
>   			ret = -EINVAL;
>   			goto out;
>   		}
> 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 v6 3/5] ksmbd: Fix FSCTL_VALIDATE_NEGOTIATE_INFO message length check in smb2_ioctl()

[Namjae Jeon]

2022-09-14 11:17 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.
>
> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
> move its struct to smbfs_common")
NACK. I am still thinking this tag is wrong.

> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> Cc: <stable@vger.kernel.org>
> ---
>  fs/ksmbd/smb2pdu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index b56d7688ccf1..09ae601e64f9 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
>  			goto out;
>  		}
>
> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
> +		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
> +					  Dialects)) {
>  			ret = -EINVAL;
>  			goto out;
>  		}
> --
> 2.31.1
>
>

Re: [PATCH v6 3/5] ksmbd: Fix FSCTL_VALIDATE_NEGOTIATE_INFO message length check in smb2_ioctl()

[zhangxiaoxu (A)]

在 2022/9/16 8:26, Namjae Jeon 写道:
> 2022-09-14 11:17 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.
>>
>> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
>> move its struct to smbfs_common")
> NACK. I am still thinking this tag is wrong.
Thanks for your comments.

In the v2, I remove the validation expression, and as your comments
in v2, duplicate check is unneed.

I add it back in v6 because tom's comments, we should ensure the req
has 'DialectCount', before validate the req has enough 'Dialects',
otherwise, dereferencing 'neg_req->DialectCount' will be OOB read.

Maybe the validation expression as below much better:
@@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
   ...
   if (in_buf_len < offsetof(struct validate_negotiate_info_req, Dialects[1]))

If even there's no one Dialects, 'the fsctl_validate_negotiate_info'
still return -EINVAL:

   fsctl_validate_negotiate_info
     ksmbd_lookup_dialect_by_id(neg_req->DialectCount=0)
       return BAD_PROT_ID
     ret = -EINVAL;

Same as before commit c7803b05f74b.

Could you give me more help about the fix tag.

Thanks.
Zhang Xiaoxu
> 
>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>>   fs/ksmbd/smb2pdu.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index b56d7688ccf1..09ae601e64f9 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
>>   			goto out;
>>   		}
>>
>> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
>> +		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
>> +					  Dialects)) {
>>   			ret = -EINVAL;
>>   			goto out;
>>   		}
>> --
>> 2.31.1
>>
>>

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

[zhangxiaoxu (A)]

Thanks steve and tom.

Now the client only use max 4 dialects, should we need to refactor
the 'Dialects' in 'validate_negotiate_info_req' to variable array?

Now the layout of 'validate_negotiate_info_req' is:

/* offset    |  size */  type = struct validate_negotiate_info_req {
/*    0      |     4 */    __le32 Capabilities;
/*    4      |    16 */    __u8 Guid[16];
/*   20      |     2 */    __le16 SecurityMode;
/*   22      |     2 */    __le16 DialectCount;
/*   24      |     8 */    __le16 Dialects[4];

                            /* total size (bytes):   32 */
                          }

the struct size already the 2 power, if use variable array, doesn't
conserve memory and made code complexity, since we should calculate
the size we allocate memory.

I think use the implicitly variable array is no problems.

This patch is the pre-patch of refactor 'Dialects' to variable array,
it can be easily to get the size of dialects in the negotiate message.

If this patch is unneed, the code to get the dialects size maybe:

int get_nego_dialect_cnt()
{
	if (strcmp(version_string, SMBxx_VERSION_STRING)
		return xx;
	...
}

void build_nego_dialect()
{
	if (strcmp(version_string, SMBxx_VERSION_STRING) {
		Dialects[x] = cpu_to_le16(SMBx_PROT_ID);
		...
	}
}

req = kmalloc(sizeof() + get_nego_dialect_cnt())
build_nego_dialect(req->Dialects)
...

or refactor 'Dialects' to variable array and use default 4
as the dialect count when allocate memory.

Thanks.
Zhang Xiaoxu

在 2022/9/16 8:13, Steve French 写道:
> I lean against this patch because it doesn't appear to fix anything and doesn't seem to make much difference in clarity. As Tom indicated, not likely to add any dialects in the future. I don't mind clean up that removes redundant code or is part of a related fix, but this one doesn't seem to help clarity much
> 
> On Thu, Sep 15, 2022, 11:47 Tom Talpey <tom@talpey.com <mailto:tom@talpey.com>> wrote:
> 
>     Steve, your opinion on including this? You may have missed the
>     question I asked earlier in the thread:
> 
>      > 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?
> 
>     Tom.
> 
>     On 9/13/2022 7:17 PM, 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 <mailto:zhangxiaoxu5@huawei.com>>
>      > Acked-by: Tom Talpey <tom@talpey.com <mailto:tom@talpey.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..e1407124c761 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),
>      >   };
>      >
>      > +static __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,
>      > +};
>      > +
>      > +static __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,
>      > +};
>      > +
>      > +static __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,
>      > +};
>      > +
>      > +static __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,
>      > +};
>      > +
>      > +static __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 223056097b54..482ed480fbc6 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)
>      > @@ -1145,34 +1128,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 v6 3/5] ksmbd: Fix FSCTL_VALIDATE_NEGOTIATE_INFO message length check in smb2_ioctl()

[Namjae Jeon]

2022-09-16 20:20 GMT+09:00, zhangxiaoxu (A) <zhangxiaoxu5@huawei.com>:
>
>
> 在 2022/9/16 8:26, Namjae Jeon 写道:
>> 2022-09-14 11:17 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.
>>>
>>> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and
>>> move its struct to smbfs_common")
>> NACK. I am still thinking this tag is wrong.
> Thanks for your comments.
>
> In the v2, I remove the validation expression, and as your comments
> in v2, duplicate check is unneed.
>
> I add it back in v6 because tom's comments, we should ensure the req
> has 'DialectCount', before validate the req has enough 'Dialects',
> otherwise, dereferencing 'neg_req->DialectCount' will be OOB read.
>
> Maybe the validation expression as below much better:
> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
>    ...
>    if (in_buf_len < offsetof(struct validate_negotiate_info_req,
> Dialects[1]))
>
> If even there's no one Dialects, 'the fsctl_validate_negotiate_info'
> still return -EINVAL:
>
>    fsctl_validate_negotiate_info
>      ksmbd_lookup_dialect_by_id(neg_req->DialectCount=0)
>        return BAD_PROT_ID
>      ret = -EINVAL;
>
> Same as before commit c7803b05f74b.
Sorry, I don't understand what you say.
>
> Could you give me more help about the fix tag.
Please change a tag to commit f7db8fd03a4bc in this patch.

Thanks.
>
> Thanks.
> Zhang Xiaoxu
>>
>>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>>   fs/ksmbd/smb2pdu.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>> index b56d7688ccf1..09ae601e64f9 100644
>>> --- a/fs/ksmbd/smb2pdu.c
>>> +++ b/fs/ksmbd/smb2pdu.c
>>> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work)
>>>   			goto out;
>>>   		}
>>>
>>> -		if (in_buf_len < sizeof(struct validate_negotiate_info_req)) {
>>> +		if (in_buf_len < offsetof(struct validate_negotiate_info_req,
>>> +					  Dialects)) {
>>>   			ret = -EINVAL;
>>>   			goto out;
>>>   		}
>>> --
>>> 2.31.1
>>>
>>>
>