* Re: [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID
[not found] ` <CAKYAXd_64YRcho0Qnq+ccezVL7Tu2_9Jjb8PM+GDujZ9h8x6xw@mail.gmail.com>
@ 2021-05-15 14:27 ` Stefan Metzmacher
2021-05-15 14:48 ` Namjae Jeon
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Metzmacher @ 2021-05-15 14:27 UTC (permalink / raw)
To: Namjae Jeon; +Cc: Marios Makassikis, linux-cifsd-devel, linux-cifs
[-- Attachment #1.1: Type: text/plain, Size: 5164 bytes --]
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...
metze
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID
2021-05-15 14:27 ` [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID Stefan Metzmacher
@ 2021-05-15 14:48 ` Namjae Jeon
2021-05-15 14:59 ` Steve French
0 siblings, 1 reply; 4+ messages in thread
From: Namjae Jeon @ 2021-05-15 14:48 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: Marios Makassikis, linux-cifsd-devel, linux-cifs
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
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID
2021-05-15 14:48 ` Namjae Jeon
@ 2021-05-15 14:59 ` Steve French
2021-05-19 12:02 ` Shyam Prasad N
0 siblings, 1 reply; 4+ messages in thread
From: Steve French @ 2021-05-15 14:59 UTC (permalink / raw)
To: CIFS
Cc: Stefan Metzmacher, COMMON INTERNET FILE SYSTEM SERVER,
Pavel Shilovsky, Namjae Jeon
[-- 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID
2021-05-15 14:59 ` Steve French
@ 2021-05-19 12:02 ` Shyam Prasad N
0 siblings, 0 replies; 4+ messages in thread
From: Shyam Prasad N @ 2021-05-19 12:02 UTC (permalink / raw)
To: Steve French
Cc: CIFS, Stefan Metzmacher, COMMON INTERNET FILE SYSTEM SERVER,
Pavel Shilovsky, Namjae Jeon
The fix looks good to me.
Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
On Sun, May 16, 2021 at 3:48 AM Steve French <smfrench@gmail.com> wrote:
>
> 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
--
Regards,
Shyam
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-19 12:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210514125201.560775-1-mmakassikis@freebox.fr>
[not found] ` <f053a063-c8e2-14c8-2c4f-a34c10c39fa1@samba.org>
[not found] ` <CAKYAXd9xWW9VJ=EPrhgVUP+ES04zOnHcy4rkboAVYeOuE=bX=w@mail.gmail.com>
[not found] ` <e03eb8d7-e964-5fa5-840d-9d3292d6d03f@samba.org>
[not found] ` <CAKYAXd_64YRcho0Qnq+ccezVL7Tu2_9Jjb8PM+GDujZ9h8x6xw@mail.gmail.com>
2021-05-15 14:27 ` [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID Stefan Metzmacher
2021-05-15 14:48 ` Namjae Jeon
2021-05-15 14:59 ` Steve French
2021-05-19 12:02 ` Shyam Prasad N
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).