linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Shyam Prasad N <nspmangalore@gmail.com>
To: Steve French <smfrench@gmail.com>
Cc: CIFS <linux-cifs@vger.kernel.org>,
	Stefan Metzmacher <metze@samba.org>,
	COMMON INTERNET FILE SYSTEM SERVER 
	<linux-cifsd-devel@lists.sourceforge.net>,
	Pavel Shilovsky <piastryyy@gmail.com>,
	Namjae Jeon <namjae.jeon@samsung.com>
Subject: Re: [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID
Date: Wed, 19 May 2021 17:32:01 +0530	[thread overview]
Message-ID: <CANT5p=pj9ybE=Fiy55fzJLGXQq1B8HJ31_ZDhtRenmOnDpHzrw@mail.gmail.com> (raw)
In-Reply-To: <CAH2r5ms564JrVgXMc6==GauVXFak9+Xj8yFSW081tBHP5HgA5Q@mail.gmail.com>

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

      reply	other threads:[~2021-05-19 12:02 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [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         ` 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 message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CANT5p=pj9ybE=Fiy55fzJLGXQq1B8HJ31_ZDhtRenmOnDpHzrw@mail.gmail.com' \
    --to=nspmangalore@gmail.com \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-cifsd-devel@lists.sourceforge.net \
    --cc=metze@samba.org \
    --cc=namjae.jeon@samsung.com \
    --cc=piastryyy@gmail.com \
    --cc=smfrench@gmail.com \
    --subject='Re: [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).