| Message ID | 20211005050343.268514-4-slow@samba.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Buffer validation and compound handling patches | expand |
2021-10-05 14:03 GMT+09:00, Ralph Boehme <slow@samba.org>: > No change in behaviour. > > Cc: Namjae Jeon <linkinjeon@kernel.org> > Cc: Tom Talpey <tom@talpey.com> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Steve French <smfrench@gmail.com> > Cc: Hyunchul Lee <hyc.lee@gmail.com> > Signed-off-by: Ralph Boehme <slow@samba.org> > --- > fs/ksmbd/smb2misc.c | 36 +++++++++++++++++++++++++++--------- > fs/ksmbd/smb2pdu.h | 1 + > 2 files changed, 28 insertions(+), 9 deletions(-) > > diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c > index 2cc031c39514..7ed266eb6c5e 100644 > --- a/fs/ksmbd/smb2misc.c > +++ b/fs/ksmbd/smb2misc.c > @@ -333,14 +333,7 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) > struct smb2_hdr *hdr = &pdu->hdr; > int command; > __u32 clc_len; /* calculated length */ > - __u32 len = get_rfc1002_len(pdu); > - > - if (le32_to_cpu(hdr->NextCommand) > 0) { > - len = le32_to_cpu(hdr->NextCommand); > - } else if (work->next_smb2_rcv_hdr_off) { > - len -= work->next_smb2_rcv_hdr_off; > - len = round_up(len, 8); > - } > + __u32 len = ksmbd_smb2_cur_pdu_buflen(work); > > if (check_smb2_hdr(hdr)) > return 1; > @@ -395,7 +388,7 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) > * Some windows servers (win2016) will pad also the final > * PDU in a compound to 8 bytes. > */ > - if (ALIGN(clc_len, 8) == len) > + if (ALIGN(clc_len, 8) == ALIGN(len, 8)) Can I know why you align rfc1002 len with 8 here ? Thanks!
Am 05.10.21 um 09:29 schrieb Namjae Jeon: > 2021-10-05 14:03 GMT+09:00, Ralph Boehme <slow@samba.org>: >> - if (ALIGN(clc_len, 8) == len) >> + if (ALIGN(clc_len, 8) == ALIGN(len, 8)) > Can I know why you align rfc1002 len with 8 here ? this should match the previous behaviour where for compound requests we called round_up(len, 8). This could be done differently though: len = ksmbd_smb2_cur_pdu_buflen(work); if (work->next_smb2_rcv_hdr_off) len = ALIGN(len, 8); and then just do if (ALIGN(clc_len, 8) == len) -slow
2021-10-05 16:46 GMT+09:00, Ralph Boehme <slow@samba.org>: > Am 05.10.21 um 09:29 schrieb Namjae Jeon: >> 2021-10-05 14:03 GMT+09:00, Ralph Boehme <slow@samba.org>: >>> - if (ALIGN(clc_len, 8) == len) >>> + if (ALIGN(clc_len, 8) == ALIGN(len, 8)) >> Can I know why you align rfc1002 len with 8 here ? > > this should match the previous behaviour where for compound requests we > called round_up(len, 8). > > This could be done differently though: > > len = ksmbd_smb2_cur_pdu_buflen(work); > if (work->next_smb2_rcv_hdr_off) > len = ALIGN(len, 8); I think that this code is wrong, we don't need to add pad about last compound *request* length. Thanks! > > and then just do > > if (ALIGN(clc_len, 8) == len) > > -slow > > -- > Ralph Boehme, Samba Team https://samba.org/ > SerNet Samba Team Lead https://sernet.de/en/team-samba >
diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c index 2cc031c39514..7ed266eb6c5e 100644 --- a/fs/ksmbd/smb2misc.c +++ b/fs/ksmbd/smb2misc.c @@ -333,14 +333,7 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) struct smb2_hdr *hdr = &pdu->hdr; int command; __u32 clc_len; /* calculated length */ - __u32 len = get_rfc1002_len(pdu); - - if (le32_to_cpu(hdr->NextCommand) > 0) { - len = le32_to_cpu(hdr->NextCommand); - } else if (work->next_smb2_rcv_hdr_off) { - len -= work->next_smb2_rcv_hdr_off; - len = round_up(len, 8); - } + __u32 len = ksmbd_smb2_cur_pdu_buflen(work); if (check_smb2_hdr(hdr)) return 1; @@ -395,7 +388,7 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) * Some windows servers (win2016) will pad also the final * PDU in a compound to 8 bytes. */ - if (ALIGN(clc_len, 8) == len) + if (ALIGN(clc_len, 8) == ALIGN(len, 8)) return 0; /* @@ -427,3 +420,28 @@ int smb2_negotiate_request(struct ksmbd_work *work) { return ksmbd_smb_negotiate_common(work, SMB2_NEGOTIATE_HE); } + +/** + * ksmbd_smb2_cur_pdu_buflen() - Get len of current SMB2 PDU buffer + * This returns the lenght including any possible padding. + * @work: smb work containing request buffer + */ +unsigned int ksmbd_smb2_cur_pdu_buflen(struct ksmbd_work *work) +{ + struct smb2_hdr *hdr = ksmbd_req_buf_next(work); + unsigned int buf_len; + unsigned int pdu_len; + + if (hdr->NextCommand != 0) { + /* + * hdr->NextCommand has already been validated by + * init_chained_smb2_rsp(). + */ + return __le32_to_cpu(hdr->NextCommand); + } + + buf_len = get_rfc1002_len(work->request_buf); + pdu_len = buf_len - work->next_smb2_rcv_hdr_off; + return pdu_len; +} + diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h index a6dec5ec6a54..c5fa8256b0bb 100644 --- a/fs/ksmbd/smb2pdu.h +++ b/fs/ksmbd/smb2pdu.h @@ -1680,6 +1680,7 @@ int smb2_set_rsp_credits(struct ksmbd_work *work); /* smb2 misc functions */ int ksmbd_smb2_check_message(struct ksmbd_work *work); +unsigned int ksmbd_smb2_cur_pdu_buflen(struct ksmbd_work *work); /* smb2 command handlers */ int smb2_handle_negotiate(struct ksmbd_work *work);
No change in behaviour. Cc: Namjae Jeon <linkinjeon@kernel.org> Cc: Tom Talpey <tom@talpey.com> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> Cc: Steve French <smfrench@gmail.com> Cc: Hyunchul Lee <hyc.lee@gmail.com> Signed-off-by: Ralph Boehme <slow@samba.org> --- fs/ksmbd/smb2misc.c | 36 +++++++++++++++++++++++++++--------- fs/ksmbd/smb2pdu.h | 1 + 2 files changed, 28 insertions(+), 9 deletions(-)