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 smbheader 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 -- Ralph Boehme, Samba Team https://samba.org/ SerNet Samba Team Lead https://sernet.de/en/team-samba
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
