| Message ID | 20220901142413.3351804-3-zhangxiaoxu5@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler | expand |
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@huawei.com> > Cc: <stable@vger.kernel.org> > --- > 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; >
2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@talpey.com>: > 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") >> 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, 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; > >
在 2022/9/2 22:47, Namjae Jeon 写道: > 2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@talpey.com>: >> 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") > >>> 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, 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; > Thanks Tom. I will send next version. In smb2_ioctl, validate the in_buf_len with offsetof(Dialects), in fsctl_validate_negotiate_info, validate the DialectCount >>
在 2022/9/2 22:47, Namjae Jeon 写道: > 2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@talpey.com>: >> 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@huawei.com> >>> Cc: <stable@vger.kernel.org> >>> --- >>> 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; > >>
2022-09-05 11:29 GMT+09:00, zhangxiaoxu (A) <zhangxiaoxu5@huawei.com>: > > > 在 2022/9/2 22:47, Namjae Jeon 写道: >> 2022-09-02 22:28 GMT+09:00, Tom Talpey <tom@talpey.com>: >>> 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. Okay, Do you think that your patch is not needed without commit c7803b05f74b ? I understood that the InputCount check doesn't take the DialectCount into account and it's already a duplicate check, So you try to remove that. > > 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.
在 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@huawei.com> >> Cc: <stable@vger.kernel.org> >> --- >> 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; >
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; - if (out_buf_len < sizeof(struct validate_negotiate_info_rsp)) return -EINVAL;
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@huawei.com> Cc: <stable@vger.kernel.org> --- fs/ksmbd/smb2pdu.c | 3 --- 1 file changed, 3 deletions(-)