| Message ID | 20210929084501.94846-1-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | ksmbd: a bunch of patches that is being reviewed | expand |
Am 29.09.21 um 10:44 schrieb Namjae Jeon: > 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: Hyunchul Lee <hyc.lee@gmail.com> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > > v2: > - update comments of smb2_get_data_area_len(). > - fix wrong buffer size check in fsctl_query_iface_info_ioctl(). > - fix 32bit overflow in smb2_set_info. > > v3: > - add buffer check for ByteCount of smb negotiate request. > - Moved buffer check of to the top of loop to avoid unneeded behavior when > out_buf_len is smaller than network_interface_info_ioctl_rsp. > - get correct out_buf_len which doesn't exceed max stream protocol length. > - subtract single smb2_lock_element for correct buffer size check in > ksmbd_smb2_check_message(). > > v4: > - use work->response_sz for out_buf_len calculation in smb2_ioctl. > - move smb2_neg size check to above to validate NegotiateContextOffset > field. > - remove unneeded dialect checks in smb2_sess_setup() and > smb2_handle_negotiate(). > - split smb2_set_info patch into two patches(declaring > smb2_file_basic_info and buffer check) it looks like you dropped all my patches and didn't comment on the SQUASHES that pointed at some issues. Did I miss anything where you explained why you did this? The changes I made imho consolidated the SMB2 PDU packet size checking logic. With your changes the check for valid SMB2 PDU sizes of compound offsets is spread across the network receive layer and the compound parsing layer. The changes I made, adding a nice helper function along the way, moved the core PDU validation into the function were it should be done: inside ksmbd_smb2_check_message(). You also dropped the fix for the possible invalid read in ksmbd_verify_smb_message() of the protocol_id field. I might be missing something because I'm still new to the code. But generally we really sanitize the logic while we're at it now instead of adding band aids everywhere. Thanks! -slow
2021-09-30 2:55 GMT+09:00, Ralph Boehme <slow@samba.org>: > Am 29.09.21 um 10:44 schrieb Namjae Jeon: >> 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: Hyunchul Lee <hyc.lee@gmail.com> >> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> >> >> v2: >> - update comments of smb2_get_data_area_len(). >> - fix wrong buffer size check in fsctl_query_iface_info_ioctl(). >> - fix 32bit overflow in smb2_set_info. >> >> v3: >> - add buffer check for ByteCount of smb negotiate request. >> - Moved buffer check of to the top of loop to avoid unneeded behavior >> when >> out_buf_len is smaller than network_interface_info_ioctl_rsp. >> - get correct out_buf_len which doesn't exceed max stream protocol >> length. >> - subtract single smb2_lock_element for correct buffer size check in >> ksmbd_smb2_check_message(). >> >> v4: >> - use work->response_sz for out_buf_len calculation in smb2_ioctl. >> - move smb2_neg size check to above to validate NegotiateContextOffset >> field. >> - remove unneeded dialect checks in smb2_sess_setup() and >> smb2_handle_negotiate(). >> - split smb2_set_info patch into two patches(declaring >> smb2_file_basic_info and buffer check) > > it looks like you dropped all my patches and didn't comment on the > SQUASHES that pointed at some issues. > > Did I miss anything where you explained why you did this? Please see v4 change list in this cover letter - use work->response_sz for out_buf_len calculation in smb2_ioctl. - split smb2_set_info patch into two patches(declaring... I didn't apply "SQUASH: at this layer we should only against the MAX_STREAM_PROT_LEN …" because smb2 header is used before ksmbd_verify_smb_message is reached. See init_rsp_hdr and check_user_session() in __handle_ksmbd_work(). Have you checked my comments on your squash patches of github ? I didn't get any response from you :) > > The changes I made imho consolidated the SMB2 PDU packet size checking > logic. With your changes the check for valid SMB2 PDU sizes of compound > offsets is spread across the network receive layer and the compound > parsing layer. > > The changes I made, adding a nice helper function along the way, moved > the core PDU validation into the function were it should be done: inside > ksmbd_smb2_check_message(). ksmbd is checking whether session id and tree id are vaild in smb header before processing smb requests. is_chained_smb2_message is checking next command header size. > > You also dropped the fix for the possible invalid read in > ksmbd_verify_smb_message() of the protocol_id field. Where ? You are saying your patch in github ? If it is right, I didn't drop. > > I might be missing something because I'm still new to the code. But > generally we really sanitize the logic while we're at it now instead of > adding band aids everywhere. I saw your patch and it's nice. However, we have not yet agreed on where the review will be conducted. You also didn't respond to my comments on my squash patch in your github. So I thought I'd take a deeper look if you send the patch to the list. > > Thanks! Thanks :) > -slow > > -- > Ralph Boehme, Samba Team https://samba.org/ > SerNet Samba Team Lead https://sernet.de/en/team-samba >
Am 30.09.21 um 03:01 schrieb Namjae Jeon: > 2021-09-30 2:55 GMT+09:00, Ralph Boehme <slow@samba.org>: >> Am 29.09.21 um 10:44 schrieb Namjae Jeon: >>> 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: Hyunchul Lee <hyc.lee@gmail.com> >>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> >>> >>> v2: >>> - update comments of smb2_get_data_area_len(). >>> - fix wrong buffer size check in fsctl_query_iface_info_ioctl(). >>> - fix 32bit overflow in smb2_set_info. >>> >>> v3: >>> - add buffer check for ByteCount of smb negotiate request. >>> - Moved buffer check of to the top of loop to avoid unneeded behavior >>> when >>> out_buf_len is smaller than network_interface_info_ioctl_rsp. >>> - get correct out_buf_len which doesn't exceed max stream protocol >>> length. >>> - subtract single smb2_lock_element for correct buffer size check in >>> ksmbd_smb2_check_message(). >>> >>> v4: >>> - use work->response_sz for out_buf_len calculation in smb2_ioctl. >>> - move smb2_neg size check to above to validate NegotiateContextOffset >>> field. >>> - remove unneeded dialect checks in smb2_sess_setup() and >>> smb2_handle_negotiate(). >>> - split smb2_set_info patch into two patches(declaring >>> smb2_file_basic_info and buffer check) >> >> it looks like you dropped all my patches and didn't comment on the >> SQUASHES that pointed at some issues. >> >> Did I miss anything where you explained why you did this? > Please see v4 change list in this cover letter > - use work->response_sz for out_buf_len calculation in smb2_ioctl. > - split smb2_set_info patch into two patches(declaring... > > I didn't apply "SQUASH: at this layer we should only against the > MAX_STREAM_PROT_LEN …" > because smb2 header is used before ksmbd_verify_smb_message is reached. > See init_rsp_hdr and check_user_session() in __handle_ksmbd_work(). Let me check. > Have you checked my comments on your squash patches of github ? > I didn't get any response from you :) Oh my! Looks like I didn't get github email notifications so I wasn't aware of your comments... Sorry! :) Currently taking a look. >> >> The changes I made imho consolidated the SMB2 PDU packet size checking >> logic. With your changes the check for valid SMB2 PDU sizes of compound >> offsets is spread across the network receive layer and the compound >> parsing layer. >> >> The changes I made, adding a nice helper function along the way, moved >> the core PDU validation into the function were it should be done: inside >> ksmbd_smb2_check_message(). > ksmbd is checking whether session id and tree id are vaild in smb > header before processing smb requests. yes, this was next on my list, sorry forgot to mention this. Afaict in the current code the session and tcon checks are only done once on the first SMB2 PDU for a series of compound non-related PDUs, while for non-related PDUs the calls to check_user_session() and smb2_get_ksmbd_tcon() should be probably be done inside __process_request(), or eventually just inside ksmbd_smb2_check_message(). > is_chained_smb2_message is > checking next command header size. >> >> You also dropped the fix for the possible invalid read in >> ksmbd_verify_smb_message() of the protocol_id field. > Where ? You are saying your patch in github ? If it is right, I didn't drop. this one: <https://github.com/smfrench/smb3-kernel/commit/ffc410f1d19a0f06a952c7f28e9bca4fa4bd4a74> And also the cleanup commits using ksmbd_req_buf_next() in a few places. >> I might be missing something because I'm still new to the code. But >> generally we really sanitize the logic while we're at it now instead of >> adding band aids everywhere. > I saw your patch and it's nice. However, we have not yet agreed on > where the review will be conducted. You also didn't respond to my > comments on my squash patch in your github. So I thought I'd take a > deeper look if you send the patch to the list. I realize that my well thought idea to idea of simplifying things by pushing my larger changes to github is not going very well. :) Therefor I'll resubmit the patchset to the ML later on. Fwiw, here's is what an actual review on github on a MR would look like: <https://github.com/smfrench/smb3-kernel/pull/72> This was just an experiment. It demonstrates a few features: tracking comments, tracking new pushes to the source branch and other related actions. Note that I'm NOT PROPOSING using github MRs right away. Just showing what's possible. :) -slow
2021-09-30 21:53 GMT+09:00, Ralph Boehme <slow@samba.org>: > Am 30.09.21 um 03:01 schrieb Namjae Jeon: >> 2021-09-30 2:55 GMT+09:00, Ralph Boehme <slow@samba.org>: >>> Am 29.09.21 um 10:44 schrieb Namjae Jeon: >>>> 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: Hyunchul Lee <hyc.lee@gmail.com> >>>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> >>>> >>>> v2: >>>> - update comments of smb2_get_data_area_len(). >>>> - fix wrong buffer size check in fsctl_query_iface_info_ioctl(). >>>> - fix 32bit overflow in smb2_set_info. >>>> >>>> v3: >>>> - add buffer check for ByteCount of smb negotiate request. >>>> - Moved buffer check of to the top of loop to avoid unneeded >>>> behavior >>>> when >>>> out_buf_len is smaller than network_interface_info_ioctl_rsp. >>>> - get correct out_buf_len which doesn't exceed max stream protocol >>>> length. >>>> - subtract single smb2_lock_element for correct buffer size check >>>> in >>>> ksmbd_smb2_check_message(). >>>> >>>> v4: >>>> - use work->response_sz for out_buf_len calculation in smb2_ioctl. >>>> - move smb2_neg size check to above to validate >>>> NegotiateContextOffset >>>> field. >>>> - remove unneeded dialect checks in smb2_sess_setup() and >>>> smb2_handle_negotiate(). >>>> - split smb2_set_info patch into two patches(declaring >>>> smb2_file_basic_info and buffer check) >>> >>> it looks like you dropped all my patches and didn't comment on the >>> SQUASHES that pointed at some issues. >>> >>> Did I miss anything where you explained why you did this? >> Please see v4 change list in this cover letter >> - use work->response_sz for out_buf_len calculation in smb2_ioctl. >> - split smb2_set_info patch into two patches(declaring... >> >> I didn't apply "SQUASH: at this layer we should only against the >> MAX_STREAM_PROT_LEN …" >> because smb2 header is used before ksmbd_verify_smb_message is reached. >> See init_rsp_hdr and check_user_session() in __handle_ksmbd_work(). > > Let me check. > >> Have you checked my comments on your squash patches of github ? >> I didn't get any response from you :) > > Oh my! Looks like I didn't get github email notifications so I wasn't > aware of your comments... Sorry! :) Currently taking a look. > >>> >>> The changes I made imho consolidated the SMB2 PDU packet size checking >>> logic. With your changes the check for valid SMB2 PDU sizes of compound >>> offsets is spread across the network receive layer and the compound >>> parsing layer. >>> >>> The changes I made, adding a nice helper function along the way, moved >>> the core PDU validation into the function were it should be done: inside >>> ksmbd_smb2_check_message(). >> ksmbd is checking whether session id and tree id are vaild in smb >> header before processing smb requests. > > yes, this was next on my list, sorry forgot to mention this. Afaict in > the current code the session and tcon checks are only done once on the > first SMB2 PDU for a series of compound non-related PDUs, while for > non-related PDUs the calls to check_user_session() and > smb2_get_ksmbd_tcon() should be probably be done inside > __process_request(), or eventually just inside ksmbd_smb2_check_message(). check_user_session and get_ksmbd_tcon should not be moved inside __process_request() because session id and tree id of related pdu is 0xFFFFFFFFFFFFFFFF and 0xFFFFFFFF. > >> is_chained_smb2_message is >> checking next command header size. >>> >>> You also dropped the fix for the possible invalid read in >>> ksmbd_verify_smb_message() of the protocol_id field. >> Where ? You are saying your patch in github ? If it is right, I didn't >> drop. > > this one: > > <https://github.com/smfrench/smb3-kernel/commit/ffc410f1d19a0f06a952c7f28e9bca4fa4bd4a74> Ah.. You pushed this patch to ksmbd-for-next-pending ? Sorry for that, my mistake, I will check branch before applying my patch. > > And also the cleanup commits using ksmbd_req_buf_next() in a few places. > >>> I might be missing something because I'm still new to the code. But >>> generally we really sanitize the logic while we're at it now instead of >>> adding band aids everywhere. >> I saw your patch and it's nice. However, we have not yet agreed on >> where the review will be conducted. You also didn't respond to my >> comments on my squash patch in your github. So I thought I'd take a >> deeper look if you send the patch to the list. > > I realize that my well thought idea to idea of simplifying things by > pushing my larger changes to github is not going very well. :) Therefor > I'll resubmit the patchset to the ML later on. > > Fwiw, here's is what an actual review on github on a MR would look like: > > <https://github.com/smfrench/smb3-kernel/pull/72> > > This was just an experiment. It demonstrates a few features: tracking > comments, tracking new pushes to the source branch and other related > actions. > > Note that I'm NOT PROPOSING using github MRs right away. Just showing > what's possible. :) Sound good. Thanks for your check:) > > -slow > > -- > Ralph Boehme, Samba Team https://samba.org/ > SerNet Samba Team Lead https://sernet.de/en/team-samba >
Am 30.09.21 um 15:17 schrieb Namjae Jeon: > 2021-09-30 21:53 GMT+09:00, Ralph Boehme <slow@samba.org>: >> yes, this was next on my list, sorry forgot to mention this. Afaict in >> the current code the session and tcon checks are only done once on the >> first SMB2 PDU for a series of compound non-related PDUs, while for >> non-related PDUs the calls to check_user_session() and >> smb2_get_ksmbd_tcon() should be probably be done inside >> __process_request(), or eventually just inside ksmbd_smb2_check_message(). > check_user_session and get_ksmbd_tcon should not be moved inside > __process_request() > because session id and tree id of related pdu is 0xFFFFFFFFFFFFFFFF > and 0xFFFFFFFF. of course, but that must just be handled by those functions. I'm currently working on tentative fix for this. > >> >>> is_chained_smb2_message is >>> checking next command header size. >>>> >>>> You also dropped the fix for the possible invalid read in >>>> ksmbd_verify_smb_message() of the protocol_id field. >>> Where ? You are saying your patch in github ? If it is right, I didn't >>> drop. >> >> this one: >> >> <https://github.com/smfrench/smb3-kernel/commit/ffc410f1d19a0f06a952c7f28e9bca4fa4bd4a74> > Ah.. You pushed this patch to ksmbd-for-next-pending ? > Sorry for that, my mistake, I will check branch before applying my patch. Yeah, the whole patchset is in the branch ksmbd-for-next-pending which is actually the one you correctly used for the comments on github. :) Cheers! -slow
2021-09-30 22:33 GMT+09:00, Ralph Boehme <slow@samba.org>: > Am 30.09.21 um 15:17 schrieb Namjae Jeon: >> 2021-09-30 21:53 GMT+09:00, Ralph Boehme <slow@samba.org>: >>> yes, this was next on my list, sorry forgot to mention this. Afaict in >>> the current code the session and tcon checks are only done once on the >>> first SMB2 PDU for a series of compound non-related PDUs, while for >>> non-related PDUs the calls to check_user_session() and >>> smb2_get_ksmbd_tcon() should be probably be done inside >>> __process_request(), or eventually just inside >>> ksmbd_smb2_check_message(). >> check_user_session and get_ksmbd_tcon should not be moved inside >> __process_request() >> because session id and tree id of related pdu is 0xFFFFFFFFFFFFFFFF >> and 0xFFFFFFFF. > > of course, but that must just be handled by those functions. I'm > currently working on tentative fix for this. 1. You need to check header size of related pdu of compound request is already checked in the is_chained_smb2_message function. is_chained_smb2_message() ... if (next_cmd > 0) { if ((u64)work->next_smb2_rcv_hdr_off + next_cmd + __SMB2_HEADER_STRUCTURE_SIZE > get_rfc1002_len(work->request_buf)) { pr_err("next command(%u) offset exceeds smb msg size\n", next_cmd); return false; } 2. session id and tree id only needs to be checked in the header of the first pdu regardless of compound and single one. So I don't know what would be better if I moved it. Thanks! > >> >>> >>>> is_chained_smb2_message is >>>> checking next command header size. >>>>> >>>>> You also dropped the fix for the possible invalid read in >>>>> ksmbd_verify_smb_message() of the protocol_id field. >>>> Where ? You are saying your patch in github ? If it is right, I didn't >>>> drop. >>> >>> this one: >>> >>> <https://github.com/smfrench/smb3-kernel/commit/ffc410f1d19a0f06a952c7f28e9bca4fa4bd4a74> >> Ah.. You pushed this patch to ksmbd-for-next-pending ? >> Sorry for that, my mistake, I will check branch before applying my patch. > > Yeah, the whole patchset is in the branch ksmbd-for-next-pending which > is actually the one you correctly used for the comments on github. :) Ah.. okay. I will carefully check it next time. Thanks! > > Cheers! > -slow > > -- > Ralph Boehme, Samba Team https://samba.org/ > SerNet Samba Team Lead https://sernet.de/en/team-samba >
Am 01.10.21 um 03:10 schrieb Namjae Jeon: > 1. You need to check header size of related pdu of compound request is > already checked in the is_chained_smb2_message function. > > is_chained_smb2_message() > ... > if (next_cmd > 0) { > if ((u64)work->next_smb2_rcv_hdr_off + next_cmd + > __SMB2_HEADER_STRUCTURE_SIZE > > get_rfc1002_len(work->request_buf)) { > pr_err("next command(%u) offset exceeds smb msg size\n", > next_cmd); > return false; > } yeah, I already mentioned that in the commit message iirc. The problem with this is that this logic is too brittle and hard to understand. The code should be robust and easy to understand, that's why I strongly suggest to add the check to ksmbd_smb2_check_message(). > 2. session id and tree id only needs to be checked in the header of > the first pdu regardless of compound and single one. Unless I'm completely off (which I sometimes are, so I'm prepared to be proven false :) ), this is not correct. Cf MS-SMB2 3.3.5.2.7. For non-related compound requests session-id and tree-id are to be taken from each PDU. Cf also the Samba code in smbd_smb2_request_dispatch() which calls smbd_smb2_request_check_session() and smbd_smb2_request_check_tcon() to implement the relevant logic. I'll send what I have in a second. -slow
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: Hyunchul Lee <hyc.lee@gmail.com> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> v2: - update comments of smb2_get_data_area_len(). - fix wrong buffer size check in fsctl_query_iface_info_ioctl(). - fix 32bit overflow in smb2_set_info. v3: - add buffer check for ByteCount of smb negotiate request. - Moved buffer check of to the top of loop to avoid unneeded behavior when out_buf_len is smaller than network_interface_info_ioctl_rsp. - get correct out_buf_len which doesn't exceed max stream protocol length. - subtract single smb2_lock_element for correct buffer size check in ksmbd_smb2_check_message(). v4: - use work->response_sz for out_buf_len calculation in smb2_ioctl. - move smb2_neg size check to above to validate NegotiateContextOffset field. - remove unneeded dialect checks in smb2_sess_setup() and smb2_handle_negotiate(). - split smb2_set_info patch into two patches(declaring smb2_file_basic_info and buffer check) Hyunchul Lee (1): ksmbd: add buffer validation for SMB2_CREATE_CONTEXT Namjae Jeon (8): ksmbd: add the check to vaildate if stream protocol length exceeds maximum value ksmbd: add validation in smb2_ioctl ksmbd: use correct basic info level in set_file_basic_info() ksmbd: add request buffer validation in smb2_set_info ksmbd: check strictly data area in ksmbd_smb2_check_message() ksmbd: add validation in smb2 negotiate ksmbd: remove the leftover of smb2.0 dialect support ksmbd: remove NTLMv1 authentication fs/ksmbd/auth.c | 205 ------------------------ fs/ksmbd/connection.c | 10 +- fs/ksmbd/crypto_ctx.c | 16 -- fs/ksmbd/crypto_ctx.h | 8 - fs/ksmbd/oplock.c | 41 +++-- fs/ksmbd/smb2misc.c | 98 ++++++------ fs/ksmbd/smb2ops.c | 5 - fs/ksmbd/smb2pdu.c | 364 ++++++++++++++++++++++++++++++------------ fs/ksmbd/smb2pdu.h | 10 +- fs/ksmbd/smb_common.c | 44 +++-- fs/ksmbd/smb_common.h | 4 +- fs/ksmbd/smbacl.c | 21 ++- fs/ksmbd/vfs.c | 2 +- fs/ksmbd/vfs.h | 2 +- 14 files changed, 412 insertions(+), 418 deletions(-)