在 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@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:
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; >
