在 2022/9/2 22:47, Namjae Jeon 写道:
2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>:
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@xxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
---
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; >
