2021-09-30 21:53 GMT+09:00, Ralph Boehme <slow@xxxxxxxxx>: > Am 30.09.21 um 03:01 schrieb Namjae Jeon: >> 2021-09-30 2:55 GMT+09:00, Ralph Boehme <slow@xxxxxxxxx>: >>> Am 29.09.21 um 10:44 schrieb Namjae Jeon: >>>> Cc: Tom Talpey <tom@xxxxxxxxxx> >>>> Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx> >>>> Cc: Ralph Böhme <slow@xxxxxxxxx> >>>> Cc: Steve French <smfrench@xxxxxxxxx> >>>> Cc: Hyunchul Lee <hyc.lee@xxxxxxxxx> >>>> Cc: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx> >>>> >>>> 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 >
