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

[Namjae Jeon <linkinjeon@kernel.org>]

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
>