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: 1623 bytes --]

Am 01.10.21 um 03:10 schrieb Namjae Jeon:
> 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;
>                  }

yeah, I already mentioned that in the commit message iirc. The problem 
with this is that this logic is too brittle and hard to understand. The 
code should be robust and easy to understand, that's why I strongly 
suggest to add the check to ksmbd_smb2_check_message().

> 2. session id and tree id only needs to be checked in the header of
> the first pdu regardless of compound and single one.

Unless I'm completely off (which I sometimes are, so I'm prepared to be 
proven false :) ), this is not correct. Cf MS-SMB2 3.3.5.2.7. For 
non-related compound requests session-id and tree-id are to be taken 
from each PDU.

Cf also the Samba code in smbd_smb2_request_dispatch() which calls 
smbd_smb2_request_check_session() and smbd_smb2_request_check_tcon() to 
implement the relevant logic.

I'll send what I have in a second.

-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 --]