diff mbox series

Update POSIX negotiate context during negprot to include GUID

Message ID CAH2r5msoRSXFj=SM54Dg7A74yMj9X1XB=S8JY1zM=FgH13qzog@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series Update POSIX negotiate context during negprot to include GUID | expand

Commit Message

Steve French Feb. 25, 2019, 12:01 a.m. UTC 
As requested - updated the POSIX negotiate context to include the GUID
of the only supported POSIX open/create context.

Let me know if any objections..

Tested with JRAs tree and worked fine, other than unrelated problem
(appears to be in server code) where the world writeable bit is always
masked off on mkdir (could be user error but smb.conf looked ok to me
and worked with his older tree for all modes).  Other modes (other
than setuid/setgid/sticky) work fine on directory create.

Comments

Stefan Metzmacher Feb. 25, 2019, 7:22 a.m. UTC | #1 
Am 25.02.19 um 01:01 schrieb Steve French via samba-technical:
> As requested - updated the POSIX negotiate context to include the GUID
> of the only supported POSIX open/create context.
> 
> Let me know if any objections..

Don't you need to change POSIX_CTXT_DATA_LEN?

metze
Steve French Feb. 25, 2019, 7:26 a.m. UTC | #2 
On Mon, Feb 25, 2019 at 1:22 AM Stefan Metzmacher <metze@samba.org> wrote:
>
> Am 25.02.19 um 01:01 schrieb Steve French via samba-technical:
> > As requested - updated the POSIX negotiate context to include the GUID
> > of the only supported POSIX open/create context.
> >
> > Let me know if any objections..
>
> Don't you need to change POSIX_CTXT_DATA_LEN?

Isn't 16 bytes correct (at least one GUID - in this case matching the
POSIX open context GUID)?  The patch included this change:

-#define POSIX_CTXT_DATA_LEN    8
+#define POSIX_CTXT_DATA_LEN    16
Stefan Metzmacher Feb. 25, 2019, 7:43 a.m. UTC | #3 
Am 25.02.19 um 08:26 schrieb Steve French via samba-technical:
> On Mon, Feb 25, 2019 at 1:22 AM Stefan Metzmacher <metze@samba.org> wrote:
>>
>> Am 25.02.19 um 01:01 schrieb Steve French via samba-technical:
>>> As requested - updated the POSIX negotiate context to include the GUID
>>> of the only supported POSIX open/create context.
>>>
>>> Let me know if any objections..
>>
>> Don't you need to change POSIX_CTXT_DATA_LEN?
> 
> Isn't 16 bytes correct (at least one GUID - in this case matching the
> POSIX open context GUID)?  The patch included this change:
> 
> -#define POSIX_CTXT_DATA_LEN    8
> +#define POSIX_CTXT_DATA_LEN    16
> 

Yes, that correct, I missed that sorry.

Does your client still work without that chance, just sending 8 zero
bytes? Or does that fail?

I guess it will fail at

+               if ((inbuflen % 16) != 0) {
+                       return smbd_smb2_request_error(
+                               req, NT_STATUS_INVALID_PARAMETER);
+               }

I think we need to have something like this before:

     static const uint64_t reserved64;
     DATA_BLOB reserved = data_blob_const(&reserved64,
sizeof(reserved6464));
     int cmp;

     cmp = data_blob_cmp(&in_posix->data, &reserved_value);
     if (cmp == 0) {
           inbuf = (const uint8_t *)SMB2_CREATE_TAG_POSIX;
           inbuflen = 16;
           outbuf = reserved.blob;
           outbuflen = reserved.length;
     }

metze
Jeremy Allison Feb. 25, 2019, 2:21 p.m. UTC | #4 
On Mon, Feb 25, 2019 at 08:43:33AM +0100, Stefan Metzmacher wrote:
> Yes, that correct, I missed that sorry.
> 
> Does your client still work without that chance, just sending 8 zero
> bytes? Or does that fail?
> 
> I guess it will fail at
> 
> +               if ((inbuflen % 16) != 0) {
> +                       return smbd_smb2_request_error(
> +                               req, NT_STATUS_INVALID_PARAMETER);
> +               }

Yes, it's supposed to fail there :-).

> I think we need to have something like this before:
> 
>      static const uint64_t reserved64;
>      DATA_BLOB reserved = data_blob_const(&reserved64,
> sizeof(reserved6464));
>      int cmp;
> 
>      cmp = data_blob_cmp(&in_posix->data, &reserved_value);
>      if (cmp == 0) {
>            inbuf = (const uint8_t *)SMB2_CREATE_TAG_POSIX;
>            inbuflen = 16;
>            outbuf = reserved.blob;
>            outbuflen = reserved.length;
>      }

I don't want to do that. None of this code is in any
mainline/master/released code bases so I don't want
to support what is "test" code in the Linux client.

We are close to the point of making something
that we can commit to support going forward,
but we're not there yet.

So I really don't want to support any Negprot
variants other than the latest "list of create
contexts" version.
Stefan Metzmacher Feb. 25, 2019, 2:34 p.m. UTC | #5 
Hi Jeremy,

> So I really don't want to support any Negprot
> variants other than the latest "list of create
> contexts" version.

Ok, that's also fine for me.

metze
Steve French Feb. 25, 2019, 3:54 p.m. UTC | #6 
Yes - it fails (Samba returns error invalid parameter) without this change.

On Mon, Feb 25, 2019 at 8:21 AM Jeremy Allison <jra@samba.org> wrote:
>
> On Mon, Feb 25, 2019 at 08:43:33AM +0100, Stefan Metzmacher wrote:
> > Yes, that correct, I missed that sorry.
> >
> > Does your client still work without that chance, just sending 8 zero
> > bytes? Or does that fail?
> >
> > I guess it will fail at
> >
> > +               if ((inbuflen % 16) != 0) {
> > +                       return smbd_smb2_request_error(
> > +                               req, NT_STATUS_INVALID_PARAMETER);
> > +               }
>
> Yes, it's supposed to fail there :-).
>
> > I think we need to have something like this before:
> >
> >      static const uint64_t reserved64;
> >      DATA_BLOB reserved = data_blob_const(&reserved64,
> > sizeof(reserved6464));
> >      int cmp;
> >
> >      cmp = data_blob_cmp(&in_posix->data, &reserved_value);
> >      if (cmp == 0) {
> >            inbuf = (const uint8_t *)SMB2_CREATE_TAG_POSIX;
> >            inbuflen = 16;
> >            outbuf = reserved.blob;
> >            outbuflen = reserved.length;
> >      }
>
> I don't want to do that. None of this code is in any
> mainline/master/released code bases so I don't want
> to support what is "test" code in the Linux client.
>
> We are close to the point of making something
> that we can commit to support going forward,
> but we're not there yet.
>
> So I really don't want to support any Negprot
> variants other than the latest "list of create
> contexts" version.
diff mbox series

Patch

From 56b32521d538eb1f4ad9aab15e10552cab19bdca Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Sun, 24 Feb 2019 17:56:33 -0600
Subject: [PATCH] smb3: Update POSIX negotiate context with POSIX ctxt GUID

POSIX negotiate context now includes the GUID specifying
which POSIX open context we support.

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/smb2pdu.c | 17 +++++++++++++++++
 fs/cifs/smb2pdu.h |  4 ++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 49c2843b1bcf..64e172633bc4 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -490,6 +490,23 @@  build_posix_ctxt(struct smb2_posix_neg_context *pneg_ctxt)
 {
 	pneg_ctxt->ContextType = SMB2_POSIX_EXTENSIONS_AVAILABLE;
 	pneg_ctxt->DataLength = cpu_to_le16(POSIX_CTXT_DATA_LEN);
+	/* SMB2_CREATE_TAG_POSIX is "0x93AD25509CB411E7B42383DE968BCD7C" */
+	pneg_ctxt->Name[0] = 0x93;
+	pneg_ctxt->Name[1] = 0xAD;
+	pneg_ctxt->Name[2] = 0x25;
+	pneg_ctxt->Name[3] = 0x50;
+	pneg_ctxt->Name[4] = 0x9C;
+	pneg_ctxt->Name[5] = 0xB4;
+	pneg_ctxt->Name[6] = 0x11;
+	pneg_ctxt->Name[7] = 0xE7;
+	pneg_ctxt->Name[8] = 0xB4;
+	pneg_ctxt->Name[9] = 0x23;
+	pneg_ctxt->Name[10] = 0x83;
+	pneg_ctxt->Name[11] = 0xDE;
+	pneg_ctxt->Name[12] = 0x96;
+	pneg_ctxt->Name[13] = 0x8B;
+	pneg_ctxt->Name[14] = 0xCD;
+	pneg_ctxt->Name[15] = 0x7C;
 }
 
 static void
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index 538e2299805f..29f974f86f28 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -288,12 +288,12 @@  struct smb2_encryption_neg_context {
 	__le16	Ciphers[1]; /* Ciphers[0] since only one used now */
 } __packed;
 
-#define POSIX_CTXT_DATA_LEN	8
+#define POSIX_CTXT_DATA_LEN	16
 struct smb2_posix_neg_context {
 	__le16	ContextType; /* 0x100 */
 	__le16	DataLength;
 	__le32	Reserved;
-	__le64	Reserved1; /* In case needed for future (eg version or caps) */
+	__u8	Name[16]; /* POSIX cntxt GUID 93AD25509CB411E7B42383DE968BCD7C */
 } __packed;
 
 struct smb2_negotiate_rsp {
-- 
2.17.1