diff mbox series

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

Message ID 20220901142413.3351804-6-zhangxiaoxu5@huawei.com (mailing list archive)
State New, archived
Headers show
Series Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler | expand

Commit Message

Zhang Xiaoxu Sept. 1, 2022, 2:24 p.m. UTC 
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(-)

Comments

Tom Talpey Sept. 2, 2022, 1:34 p.m. UTC | #1 
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 {
diff mbox series

Patch

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 {