| Message ID | 20211003043105.10453-1-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] ksmbd: use buf_data_size instead of recalculation in smb3_decrypt_req() | expand |
2021년 10월 3일 (일) 오후 1:31, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > > Tom suggested to use buf_data_size that is already calculated, to verify > these offsets. > > Cc: Tom Talpey <tom@talpey.com> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Ralph Böhme <slow@samba.org> > Cc: Steve French <smfrench@gmail.com> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > Cc: Hyunchul Lee <hyc.lee@gmail.com> > Suggested-by: Tom Talpey <tom@talpey.com> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > fs/ksmbd/smb2pdu.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index b06361313889..4d1be224dd8e 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -8457,15 +8457,13 @@ int smb3_decrypt_req(struct ksmbd_work *work) > struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr *)buf; > int rc = 0; > > - if (pdu_length + 4 < > - sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr)) { > + if (buf_data_size < sizeof(struct smb2_hdr)) { Could integer overflow occur when buf_data_size is initialized? buf_data_size is initialized with "pdu_length + 4 - sizeof(struct smb2_transform_hdr)". There was the check that the pdu size is greater than at least __SMB2_HEADER_STRUCTURE_SIZE at ksmbd_conn_handler_loop(), But I can't find this check in the latest patch set. > pr_err("Transform message is too small (%u)\n", > pdu_length); > return -ECONNABORTED; > } > > - if (pdu_length + 4 < > - le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct smb2_transform_hdr)) { > + if (buf_data_size < le32_to_cpu(tr_hdr->OriginalMessageSize)) { > pr_err("Transform message is broken\n"); > return -ECONNABORTED; > } > -- > 2.25.1 > -- Thanks, Hyunchul
2021-10-04 17:38 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > 2021년 10월 3일 (일) 오후 1:31, Namjae Jeon <linkinjeon@kernel.org>님이 작성: >> >> Tom suggested to use buf_data_size that is already calculated, to verify >> these offsets. >> >> Cc: Tom Talpey <tom@talpey.com> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> Cc: Ralph Böhme <slow@samba.org> >> Cc: Steve French <smfrench@gmail.com> >> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> >> Cc: Hyunchul Lee <hyc.lee@gmail.com> >> Suggested-by: Tom Talpey <tom@talpey.com> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> fs/ksmbd/smb2pdu.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> index b06361313889..4d1be224dd8e 100644 >> --- a/fs/ksmbd/smb2pdu.c >> +++ b/fs/ksmbd/smb2pdu.c >> @@ -8457,15 +8457,13 @@ int smb3_decrypt_req(struct ksmbd_work *work) >> struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr >> *)buf; >> int rc = 0; >> >> - if (pdu_length + 4 < >> - sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr)) >> { >> + if (buf_data_size < sizeof(struct smb2_hdr)) { > > Could integer overflow occur when buf_data_size is initialized? > buf_data_size is initialized with "pdu_length + 4 - > sizeof(struct smb2_transform_hdr)". overflow does not occur. See the comments below. > > There was the check that the pdu size is greater than at least > __SMB2_HEADER_STRUCTURE_SIZE at ksmbd_conn_handler_loop(), > But I can't find this check in the latest patch set. Please check "ksmbd: add the check to vaildate if stream protocol length exceeds maximum value". pdu_length will never exceed MAX_STREAM_PROT_LEN(0x00FFFFFF). Thanks! > > >> pr_err("Transform message is too small (%u)\n", >> pdu_length); >> return -ECONNABORTED; >> } >> >> - if (pdu_length + 4 < >> - le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct >> smb2_transform_hdr)) { >> + if (buf_data_size < le32_to_cpu(tr_hdr->OriginalMessageSize)) { >> pr_err("Transform message is broken\n"); >> return -ECONNABORTED; >> } >> -- >> 2.25.1 >> > > > -- > Thanks, > Hyunchul >
2021년 10월 4일 (월) 오후 5:58, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > > 2021-10-04 17:38 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > > 2021년 10월 3일 (일) 오후 1:31, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > >> > >> Tom suggested to use buf_data_size that is already calculated, to verify > >> these offsets. > >> > >> Cc: Tom Talpey <tom@talpey.com> > >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > >> Cc: Ralph Böhme <slow@samba.org> > >> Cc: Steve French <smfrench@gmail.com> > >> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > >> Cc: Hyunchul Lee <hyc.lee@gmail.com> > >> Suggested-by: Tom Talpey <tom@talpey.com> > >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > >> --- > >> fs/ksmbd/smb2pdu.c | 6 ++---- > >> 1 file changed, 2 insertions(+), 4 deletions(-) > >> > >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > >> index b06361313889..4d1be224dd8e 100644 > >> --- a/fs/ksmbd/smb2pdu.c > >> +++ b/fs/ksmbd/smb2pdu.c > >> @@ -8457,15 +8457,13 @@ int smb3_decrypt_req(struct ksmbd_work *work) > >> struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr > >> *)buf; > >> int rc = 0; > >> > >> - if (pdu_length + 4 < > >> - sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr)) > >> { > >> + if (buf_data_size < sizeof(struct smb2_hdr)) { > > > > Could integer overflow occur when buf_data_size is initialized? > > buf_data_size is initialized with "pdu_length + 4 - > > sizeof(struct smb2_transform_hdr)". > overflow does not occur. See the comments below. > > Ah, I am worried that pdu_length + 4 is less than sizeof(struct smb2_transform_hdr). And I can't find the check that pdu size is enough before this function is called. > > There was the check that the pdu size is greater than at least > > __SMB2_HEADER_STRUCTURE_SIZE at ksmbd_conn_handler_loop(), > > But I can't find this check in the latest patch set. > Please check "ksmbd: add the check to vaildate if stream protocol > length exceeds maximum value". pdu_length will never exceed > MAX_STREAM_PROT_LEN(0x00FFFFFF). > > Thanks! > > > > > >> pr_err("Transform message is too small (%u)\n", > >> pdu_length); > >> return -ECONNABORTED; > >> } > >> > >> - if (pdu_length + 4 < > >> - le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct > >> smb2_transform_hdr)) { > >> + if (buf_data_size < le32_to_cpu(tr_hdr->OriginalMessageSize)) { > >> pr_err("Transform message is broken\n"); > >> return -ECONNABORTED; > >> } > >> -- > >> 2.25.1 > >> > > > > > > -- > > Thanks, > > Hyunchul > > -- Thanks, Hyunchul
2021-10-04 20:14 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > 2021년 10월 4일 (월) 오후 5:58, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > >> >> 2021-10-04 17:38 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: >> > 2021년 10월 3일 (일) 오후 1:31, Namjae Jeon <linkinjeon@kernel.org>님이 작성: >> >> >> >> Tom suggested to use buf_data_size that is already calculated, to >> >> verify >> >> these offsets. >> >> >> >> Cc: Tom Talpey <tom@talpey.com> >> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> >> Cc: Ralph Böhme <slow@samba.org> >> >> Cc: Steve French <smfrench@gmail.com> >> >> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> >> >> Cc: Hyunchul Lee <hyc.lee@gmail.com> >> >> Suggested-by: Tom Talpey <tom@talpey.com> >> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> >> --- >> >> fs/ksmbd/smb2pdu.c | 6 ++---- >> >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> >> index b06361313889..4d1be224dd8e 100644 >> >> --- a/fs/ksmbd/smb2pdu.c >> >> +++ b/fs/ksmbd/smb2pdu.c >> >> @@ -8457,15 +8457,13 @@ int smb3_decrypt_req(struct ksmbd_work *work) >> >> struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr >> >> *)buf; >> >> int rc = 0; >> >> >> >> - if (pdu_length + 4 < >> >> - sizeof(struct smb2_transform_hdr) + sizeof(struct >> >> smb2_hdr)) >> >> { >> >> + if (buf_data_size < sizeof(struct smb2_hdr)) { >> > >> > Could integer overflow occur when buf_data_size is initialized? >> > buf_data_size is initialized with "pdu_length + 4 - >> > sizeof(struct smb2_transform_hdr)". >> overflow does not occur. See the comments below. >> > > > Ah, I am worried that pdu_length + 4 is less than > sizeof(struct smb2_transform_hdr). And I can't find the check > that pdu size is enough before this function is called. Got it, I will change data type of buf_data_size to signed on next version. Thanks! > > >> > There was the check that the pdu size is greater than at least >> > __SMB2_HEADER_STRUCTURE_SIZE at ksmbd_conn_handler_loop(), >> > But I can't find this check in the latest patch set. >> Please check "ksmbd: add the check to vaildate if stream protocol >> length exceeds maximum value". pdu_length will never exceed >> MAX_STREAM_PROT_LEN(0x00FFFFFF). >> >> Thanks! >> > >> > >> >> pr_err("Transform message is too small (%u)\n", >> >> pdu_length); >> >> return -ECONNABORTED; >> >> } >> >> >> >> - if (pdu_length + 4 < >> >> - le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct >> >> smb2_transform_hdr)) { >> >> + if (buf_data_size < le32_to_cpu(tr_hdr->OriginalMessageSize)) >> >> { >> >> pr_err("Transform message is broken\n"); >> >> return -ECONNABORTED; >> >> } >> >> -- >> >> 2.25.1 >> >> >> > >> > >> > -- >> > Thanks, >> > Hyunchul >> > > > > > -- > Thanks, > Hyunchul >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index b06361313889..4d1be224dd8e 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -8457,15 +8457,13 @@ int smb3_decrypt_req(struct ksmbd_work *work) struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr *)buf; int rc = 0; - if (pdu_length + 4 < - sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr)) { + if (buf_data_size < sizeof(struct smb2_hdr)) { pr_err("Transform message is too small (%u)\n", pdu_length); return -ECONNABORTED; } - if (pdu_length + 4 < - le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct smb2_transform_hdr)) { + if (buf_data_size < le32_to_cpu(tr_hdr->OriginalMessageSize)) { pr_err("Transform message is broken\n"); return -ECONNABORTED; }
Tom suggested to use buf_data_size that is already calculated, to verify these offsets. Cc: Tom Talpey <tom@talpey.com> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> Cc: Ralph Böhme <slow@samba.org> Cc: Steve French <smfrench@gmail.com> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> Cc: Hyunchul Lee <hyc.lee@gmail.com> Suggested-by: Tom Talpey <tom@talpey.com> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> --- fs/ksmbd/smb2pdu.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)