| Message ID | 20220914021741.2672982-4-zhangxiaoxu5@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some bug in FSCTL_VALIDATE_NEGOTIATE_INFO handler | expand |
On 9/13/2022 7:17 PM, Zhang Xiaoxu wrote: > 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. > > 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, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index b56d7688ccf1..09ae601e64f9 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work) > goto out; > } > > - if (in_buf_len < sizeof(struct validate_negotiate_info_req)) { > + if (in_buf_len < offsetof(struct validate_negotiate_info_req, > + Dialects)) { > ret = -EINVAL; > goto out; > } Looks good, but see previous message regarding squashing this with patch 2/5. Reviewed-by: Tom Talpey <tom@talpey.com>
2022-09-14 11:17 GMT+09:00, Zhang Xiaoxu <zhangxiaoxu5@huawei.com>: > 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. > > Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and > move its struct to smbfs_common") NACK. I am still thinking this tag is wrong. > Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > Cc: <stable@vger.kernel.org> > --- > fs/ksmbd/smb2pdu.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index b56d7688ccf1..09ae601e64f9 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work) > goto out; > } > > - if (in_buf_len < sizeof(struct validate_negotiate_info_req)) { > + if (in_buf_len < offsetof(struct validate_negotiate_info_req, > + Dialects)) { > ret = -EINVAL; > goto out; > } > -- > 2.31.1 > >
在 2022/9/16 8:26, Namjae Jeon 写道: > 2022-09-14 11:17 GMT+09:00, Zhang Xiaoxu <zhangxiaoxu5@huawei.com>: >> 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. >> >> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and >> move its struct to smbfs_common") > NACK. I am still thinking this tag is wrong. Thanks for your comments. In the v2, I remove the validation expression, and as your comments in v2, duplicate check is unneed. I add it back in v6 because tom's comments, we should ensure the req has 'DialectCount', before validate the req has enough 'Dialects', otherwise, dereferencing 'neg_req->DialectCount' will be OOB read. Maybe the validation expression as below much better: @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work) ... if (in_buf_len < offsetof(struct validate_negotiate_info_req, Dialects[1])) If even there's no one Dialects, 'the fsctl_validate_negotiate_info' still return -EINVAL: fsctl_validate_negotiate_info ksmbd_lookup_dialect_by_id(neg_req->DialectCount=0) return BAD_PROT_ID ret = -EINVAL; Same as before commit c7803b05f74b. Could you give me more help about the fix tag. Thanks. Zhang Xiaoxu > >> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> >> Cc: <stable@vger.kernel.org> >> --- >> fs/ksmbd/smb2pdu.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> index b56d7688ccf1..09ae601e64f9 100644 >> --- a/fs/ksmbd/smb2pdu.c >> +++ b/fs/ksmbd/smb2pdu.c >> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work) >> goto out; >> } >> >> - if (in_buf_len < sizeof(struct validate_negotiate_info_req)) { >> + if (in_buf_len < offsetof(struct validate_negotiate_info_req, >> + Dialects)) { >> ret = -EINVAL; >> goto out; >> } >> -- >> 2.31.1 >> >>
2022-09-16 20:20 GMT+09:00, zhangxiaoxu (A) <zhangxiaoxu5@huawei.com>: > > > 在 2022/9/16 8:26, Namjae Jeon 写道: >> 2022-09-14 11:17 GMT+09:00, Zhang Xiaoxu <zhangxiaoxu5@huawei.com>: >>> 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. >>> >>> Fixes: c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and >>> move its struct to smbfs_common") >> NACK. I am still thinking this tag is wrong. > Thanks for your comments. > > In the v2, I remove the validation expression, and as your comments > in v2, duplicate check is unneed. > > I add it back in v6 because tom's comments, we should ensure the req > has 'DialectCount', before validate the req has enough 'Dialects', > otherwise, dereferencing 'neg_req->DialectCount' will be OOB read. > > Maybe the validation expression as below much better: > @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work) > ... > if (in_buf_len < offsetof(struct validate_negotiate_info_req, > Dialects[1])) > > If even there's no one Dialects, 'the fsctl_validate_negotiate_info' > still return -EINVAL: > > fsctl_validate_negotiate_info > ksmbd_lookup_dialect_by_id(neg_req->DialectCount=0) > return BAD_PROT_ID > ret = -EINVAL; > > Same as before commit c7803b05f74b. Sorry, I don't understand what you say. > > Could you give me more help about the fix tag. Please change a tag to commit f7db8fd03a4bc in this patch. Thanks. > > Thanks. > Zhang Xiaoxu >> >>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> >>> Cc: <stable@vger.kernel.org> >>> --- >>> fs/ksmbd/smb2pdu.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >>> index b56d7688ccf1..09ae601e64f9 100644 >>> --- a/fs/ksmbd/smb2pdu.c >>> +++ b/fs/ksmbd/smb2pdu.c >>> @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work) >>> goto out; >>> } >>> >>> - if (in_buf_len < sizeof(struct validate_negotiate_info_req)) { >>> + if (in_buf_len < offsetof(struct validate_negotiate_info_req, >>> + Dialects)) { >>> ret = -EINVAL; >>> goto out; >>> } >>> -- >>> 2.31.1 >>> >>> >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index b56d7688ccf1..09ae601e64f9 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -7640,7 +7640,8 @@ int smb2_ioctl(struct ksmbd_work *work) goto out; } - if (in_buf_len < sizeof(struct validate_negotiate_info_req)) { + if (in_buf_len < offsetof(struct validate_negotiate_info_req, + Dialects)) { ret = -EINVAL; goto out; }
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. 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, 2 insertions(+), 1 deletion(-)