Re: [PATCH v4 0/9] ksmbd: a bunch of patches that is being reviewed

[Ralph Boehme <slow@samba.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4911 bytes --]

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

-- 
Ralph Boehme, Samba Team                 https://samba.org/
SerNet Samba Team Lead      https://sernet.de/en/team-samba

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]