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

[Steve French <smfrench@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6190 bytes --]

Patch attached to fix the problem with incorrect FileIDs for
compounded requests (on the client) - the problem pointed out by Metze

See MS-SMB2 3.2.4.1.4

"For an operation compounded with an SMB2 CREATE request, the FileId
field SHOULD be set to { 0xFFFFFFFFFFFFFFFF,
0xFFFFFFFFFFFFFFFF }.

On Sat, May 15, 2021 at 9:49 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> 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
> >
> >
> >
>
>
> _______________________________________________
> Linux-cifsd-devel mailing list
> Linux-cifsd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-cifsd-devel



-- 
Thanks,

Steve

[-- Attachment #2: 0001-SMB3-incorrect-file-id-in-requests-compounded-with-o.patch --]
[-- Type: text/x-patch, Size: 1216 bytes --]

From eb652867e0d1ff6062f4b0e504cd43013c15be58 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Sat, 15 May 2021 09:52:22 -0500
Subject: [PATCH] SMB3: incorrect file id in requests compounded with open

See MS-SMB2 3.2.4.1.4, file ids in compounded requests should be set to
0xFFFFFFFFFFFFFFFF (we were treating it as u32 not u64 and setting
it incorrectly).

Signed-off-by: Steve French <stfrench@microsoft.com>
Reported-by: Stefan Metzmacher <metze@samba.org>
---
 fs/cifs/smb2pdu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index a8bf43184773..9f24eb88297a 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -3900,10 +3900,10 @@ smb2_new_read_req(void **buf, unsigned int *total_len,
 			 * Related requests use info from previous read request
 			 * in chain.
 			 */
-			shdr->SessionId = 0xFFFFFFFF;
+			shdr->SessionId = 0xFFFFFFFFFFFFFFFF;
 			shdr->TreeId = 0xFFFFFFFF;
-			req->PersistentFileId = 0xFFFFFFFF;
-			req->VolatileFileId = 0xFFFFFFFF;
+			req->PersistentFileId = 0xFFFFFFFFFFFFFFFF;
+			req->VolatileFileId = 0xFFFFFFFFFFFFFFFF;
 		}
 	}
 	if (remaining_bytes > io_parms->length)
-- 
2.30.2