Re: [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID

[Namjae Jeon <linkinjeon@kernel.org>]

2021-05-15 23:27 GMT+09:00, Stefan Metzmacher <metze@samba.org>:
> Am 15.05.21 um 16:10 schrieb Namjae Jeon:
>> 2021-05-15 17:57 GMT+09:00, Stefan Metzmacher <metze@samba.org>:
>>>
>>> Am 15.05.21 um 07:18 schrieb Namjae Jeon:
>>>> 2021-05-14 22:11 GMT+09:00, Stefan Metzmacher via Linux-cifsd-devel
>>>> <linux-cifsd-devel@lists.sourceforge.net>:
>>>>>
>>>>> Am 14.05.21 um 14:52 schrieb Marios Makassikis:
>>>>>> Returning TreeID=0 is valid behaviour according to [MS-SMB2] 2.2.1.2:
>>>>>>
>>>>>>   TreeId (4 bytes): Uniquely identifies the tree connect for the
>>>>>> command.
>>>>>>   This MUST be 0 for the SMB2 TREE_CONNECT Request. The TreeId can be
>>>>>>   any unsigned 32-bit integer that is received from a previous
>>>>>>   SMB2 TREE_CONNECT Response. TreeId SHOULD be set to 0 for the
>>>>>>   following commands:
>>>>>>    [...]
>>>>>>
>>>>>> However, some client implementations reject it as invalid. Windows
>>>>>> 7/10
>>>>>> assigns ids starting from 1, and samba4 returns a random uint32_t
>>>>>> which suggests there may be other clients that consider it is
>>>>>> invalid behaviour.
>>>>>>
>>>>>> While here, simplify ksmbd_acquire_smb2_tid. 0xFFFF is a reserved
>>>>>> value
>>>>>> for CIFS/SMB1:
>>>>>>   [MS-CIFS] 2.2.4.50.2
>>>>>>
>>>>>>   TID (2 bytes): The newly generated Tree ID, used in subsequent CIFS
>>>>>>   client requests to refer to a resource relative to the
>>>>>>   SMB_Data.Bytes.Path specified in the request. Most access to the
>>>>>>   server requires a valid TID, whether the resource is password
>>>>>>   protected or not. The value 0xFFFF is reserved; the server MUST NOT
>>>>>>   return a TID value of 0xFFFF.
>>>>>>
>>>>>> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
>>>>>> ---
>>>>>> Example library that treats zero TreeID as invalid:
>>>>>>
>>>>>> https://github.com/AgNO3/jcifs-ng/blob/master/src/main/java/jcifs/internal/smb2/tree/Smb2TreeConnectResponse.java#L201
>>>>>>
>>>>>>  mgmt/ksmbd_ida.c | 9 ++-------
>>>>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/mgmt/ksmbd_ida.c b/mgmt/ksmbd_ida.c
>>>>>> index 7eb6476..34e0d2e 100644
>>>>>> --- a/mgmt/ksmbd_ida.c
>>>>>> +++ b/mgmt/ksmbd_ida.c
>>>>>> @@ -13,19 +13,14 @@ static inline int __acquire_id(struct ida *ida,
>>>>>> int
>>>>>> from, int to)
>>>>>>  #ifdef CONFIG_SMB_INSECURE_SERVER
>>>>>>  int ksmbd_acquire_smb1_tid(struct ida *ida)
>>>>>>  {
>>>>>> -	return __acquire_id(ida, 0, 0xFFFF);
>>>>>> +	return __acquire_id(ida, 1, 0xFFFF);
>>>>>>  }
>>>>>>  #endif
>>>>>>
>>>>>>  int ksmbd_acquire_smb2_tid(struct ida *ida)
>>>>>>  {
>>>>>> -	int id;
>>>>>> +	return  __acquire_id(ida, 1, 0);
>>>>>
>>>>> I think that should be __acquire_id(ida, 1, 0xFFFFFFFF) (or a lower
>>>>> constraint)
>>>>>
>>>>> 0xFFFFFFFF is used for compound requests to inherit the tree id from
>>>>> the
>>>>> previous request.
>>>> Where is it defined in the specification ? As I know,
>>>> SMB2_FLAGS_RELATED_OPERATIONS flags in smb header indicate inherit
>>>> tree id in previous request.
>>>
>>> [MS-SMB2] 3.2.4.1.4 Sending Compounded Requests
>>>
>>> ...
>>>
>>>   The client MUST construct the subsequent request as it would do
>>> normally.
>>> For any subsequent
>>>   requests the client MUST set SMB2_FLAGS_RELATED_OPERATIONS in the
>>> Flags
>>> field of the SMB2
>>>   header to indicate that it is using the SessionId, TreeId, and FileId
>>> supplied in the previous
>>>   request (or generated by the server in processing that request). For
>>> an
>>> operation compounded
>>>   with an SMB2 CREATE request, the FileId field SHOULD be set to {
>>> 0xFFFFFFFFFFFFFFFF,
>>>   0xFFFFFFFFFFFFFFFF }.
>>>
>>> This only explicitly talks about FileId and I'm not any client would do
>>> that, but in theory it should be possible to
>>> compound, the 2nd session setup request (of an anonymous authentication)
>>> with a tree connect request
>>> and an open.
>>>
>>> Which means it's the safest behavior for a server to avoid 0 and all F
>>> as
>>> valid id,
>>> there're still enough ids to use....
>>>
>>> It also makes sure that we don't end up with very confusing network
>>> captures.
>> Okay, I have checked cifs client code like the following.
>>
>>         if (request_type & CHAINED_REQUEST) {
>>                 if (!(request_type & END_OF_CHAIN)) {
>>                         /* next 8-byte aligned request */
>>                         *total_len = DIV_ROUND_UP(*total_len, 8) * 8;
>>                         shdr->NextCommand = cpu_to_le32(*total_len);
>>                 } else /* END_OF_CHAIN */
>>                         shdr->NextCommand = 0;
>>                 if (request_type & RELATED_REQUEST) {
>>                         shdr->Flags |= SMB2_FLAGS_RELATED_OPERATIONS;
>>                         /*
>>                          * Related requests use info from previous read
>> request
>>                          * in chain.
>>                          */
>>                         shdr->SessionId = 0xFFFFFFFF;
>>                         shdr->TreeId = 0xFFFFFFFF;
>>                         req->PersistentFileId = 0xFFFFFFFF;
>>                         req->VolatileFileId = 0xFFFFFFFF;
>>                 }
>
> Which seems actually wrong and should be 0xFFFFFFFFFFFFFFFF for all but
> TreeId...
Oh that's right...
>
> metze
>
>
>