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(). 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 >
