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@xxxxxxxxxx>
---
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 {
