| Message ID | CAH2r5mtkt6-ezyTC6zoi+DBjYQ3qFwW3UneF0_4qETEt51Tm9w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SMB3] ifs: refactor create_sd_buf() and and avoid corrupting the buffer | expand |
Updated patch with fixes for various endian sparse warnings On Mon, Nov 30, 2020 at 12:02 AM Steve French <smfrench@gmail.com> wrote: > > When mounting with "idsfromsid" mount option, Azure > corrupted the owner SIDs due to excessive padding > caused by placing the owner fields at the end of the > security descriptor on create. Placing owners at the > front of the security descriptor (rather than the end) > is also safer, as the number of ACEs (that follow it) > are variable. > > -- > Thanks, > > Steve
On Friday, I made similar changes as a part of my changes for sending default sec_desc, while testing against Azure. The idea is similar to Ronnie's patch. Move the owner and group SIDs up and push down the DACL. In addition to the changes Ronnie made, it looks like the 8-byte alignment is required for few of the other offset fields too. Hopefully, I'll have it tested out by EOD, and send out another patch. Regards, Shyam On Tue, Dec 1, 2020 at 9:54 AM Steve French <smfrench@gmail.com> wrote: > > Updated patch with fixes for various endian sparse warnings > > > On Mon, Nov 30, 2020 at 12:02 AM Steve French <smfrench@gmail.com> wrote: > > > > When mounting with "idsfromsid" mount option, Azure > > corrupted the owner SIDs due to excessive padding > > caused by placing the owner fields at the end of the > > security descriptor on create. Placing owners at the > > front of the security descriptor (rather than the end) > > is also safer, as the number of ACEs (that follow it) > > are variable. > > > > -- > > Thanks, > > > > Steve > > > > -- > Thanks, > > Steve
On Wed, Dec 2, 2020 at 7:52 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Friday, I made similar changes as a part of my changes for sending > default sec_desc, while testing against Azure. > The idea is similar to Ronnie's patch. Move the owner and group SIDs > up and push down the DACL. > In addition to the changes Ronnie made, it looks like the 8-byte > alignment is required for few of the other offset fields too. > Hopefully, I'll have it tested out by EOD, and send out another patch. Nice, but what types need 8 byte alignment did you find? MS-DTYP says that for self relative security descriptors that padding is not required. (section 2.4.6) If this is the case we might need to involve dochelp. > > Regards, > Shyam > > On Tue, Dec 1, 2020 at 9:54 AM Steve French <smfrench@gmail.com> wrote: > > > > Updated patch with fixes for various endian sparse warnings > > > > > > On Mon, Nov 30, 2020 at 12:02 AM Steve French <smfrench@gmail.com> wrote: > > > > > > When mounting with "idsfromsid" mount option, Azure > > > corrupted the owner SIDs due to excessive padding > > > caused by placing the owner fields at the end of the > > > security descriptor on create. Placing owners at the > > > front of the security descriptor (rather than the end) > > > is also safer, as the number of ACEs (that follow it) > > > are variable. > > > > > > -- > > > Thanks, > > > > > > Steve > > > > > > > > -- > > Thanks, > > > > Steve > > > > -- > -Shyam
updated the patch slightly by creating local variable for ace_count and acl_size to avoid excessive endian conversions On Mon, Nov 30, 2020 at 10:19 PM Steve French <smfrench@gmail.com> wrote: > > Updated patch with fixes for various endian sparse warnings > > > On Mon, Nov 30, 2020 at 12:02 AM Steve French <smfrench@gmail.com> wrote: > > > > When mounting with "idsfromsid" mount option, Azure > > corrupted the owner SIDs due to excessive padding > > caused by placing the owner fields at the end of the > > security descriptor on create. Placing owners at the > > front of the security descriptor (rather than the end) > > is also safer, as the number of ACEs (that follow it) > > are variable. > > > > -- > > Thanks, > > > > Steve > > > > -- > Thanks, > > Steve
LGTM. Maybe move acl.AclRevision down to where the other fields are assigned so they are all assigned in the same place? On Fri, Dec 4, 2020 at 2:46 AM Steve French <smfrench@gmail.com> wrote: > > updated the patch slightly by creating local variable for ace_count > and acl_size to avoid excessive endian conversions > > On Mon, Nov 30, 2020 at 10:19 PM Steve French <smfrench@gmail.com> wrote: > > > > Updated patch with fixes for various endian sparse warnings > > > > > > On Mon, Nov 30, 2020 at 12:02 AM Steve French <smfrench@gmail.com> wrote: > > > > > > When mounting with "idsfromsid" mount option, Azure > > > corrupted the owner SIDs due to excessive padding > > > caused by placing the owner fields at the end of the > > > security descriptor on create. Placing owners at the > > > front of the security descriptor (rather than the end) > > > is also safer, as the number of ACEs (that follow it) > > > are variable. > > > > > > -- > > > Thanks, > > > > > > Steve > > > > > > > > -- > > Thanks, > > > > Steve > > > > -- > Thanks, > > Steve
Updated with Ronnie's suggestion. On Thu, Dec 3, 2020 at 4:08 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > LGTM. > > Maybe move acl.AclRevision down to where the other fields are assigned > so they are all assigned in the same place? > > On Fri, Dec 4, 2020 at 2:46 AM Steve French <smfrench@gmail.com> wrote: > > > > updated the patch slightly by creating local variable for ace_count > > and acl_size to avoid excessive endian conversions > > > > On Mon, Nov 30, 2020 at 10:19 PM Steve French <smfrench@gmail.com> wrote: > > > > > > Updated patch with fixes for various endian sparse warnings > > > > > > > > > On Mon, Nov 30, 2020 at 12:02 AM Steve French <smfrench@gmail.com> wrote: > > > > > > > > When mounting with "idsfromsid" mount option, Azure > > > > corrupted the owner SIDs due to excessive padding > > > > caused by placing the owner fields at the end of the > > > > security descriptor on create. Placing owners at the > > > > front of the security descriptor (rather than the end) > > > > is also safer, as the number of ACEs (that follow it) > > > > are variable. > > > > > > > > -- > > > > Thanks, > > > > > > > > Steve > > > > > > > > > > > > -- > > > Thanks, > > > > > > Steve > > > > > > > > -- > > Thanks, > > > > Steve
Looks good! On Fri, Dec 4, 2020 at 9:12 AM Steve French <smfrench@gmail.com> wrote: > > Updated with Ronnie's suggestion. > > > On Thu, Dec 3, 2020 at 4:08 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > > > LGTM. > > > > Maybe move acl.AclRevision down to where the other fields are assigned > > so they are all assigned in the same place? > > > > On Fri, Dec 4, 2020 at 2:46 AM Steve French <smfrench@gmail.com> wrote: > > > > > > updated the patch slightly by creating local variable for ace_count > > > and acl_size to avoid excessive endian conversions > > > > > > On Mon, Nov 30, 2020 at 10:19 PM Steve French <smfrench@gmail.com> wrote: > > > > > > > > Updated patch with fixes for various endian sparse warnings > > > > > > > > > > > > On Mon, Nov 30, 2020 at 12:02 AM Steve French <smfrench@gmail.com> wrote: > > > > > > > > > > When mounting with "idsfromsid" mount option, Azure > > > > > corrupted the owner SIDs due to excessive padding > > > > > caused by placing the owner fields at the end of the > > > > > security descriptor on create. Placing owners at the > > > > > front of the security descriptor (rather than the end) > > > > > is also safer, as the number of ACEs (that follow it) > > > > > are variable. > > > > > > > > > > -- > > > > > Thanks, > > > > > > > > > > Steve > > > > > > > > > > > > > > > > -- > > > > Thanks, > > > > > > > > Steve > > > > > > > > > > > > -- > > > Thanks, > > > > > > Steve > > > > -- > Thanks, > > Steve
From 6688952606694a3ee8a8749456636dd26097f33e Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg <lsahlber@redhat.com> Date: Mon, 30 Nov 2020 11:29:20 +1000 Subject: [PATCH] cifs: refactor create_sd_buf() and and avoid corrupting the buffer When mounting with "idsfromsid" mount option, Azure corrupted the owner SIDs due to excessive padding caused by placing the owner fields at the end of the security descriptor on create. Placing owners at the front of the security descriptor (rather than the end) is also safer, as the number of ACEs (that follow it) are variable. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> Suggested-by: Rohith Surabattula <rohiths@microsoft.com> CC: Stable <stable@vger.kernel.org> # v5.8 Signed-off-by: Steven French <smfrench@gmail.com> --- fs/cifs/smb2pdu.c | 73 ++++++++++++++++++++++++++--------------------- fs/cifs/smb2pdu.h | 2 -- 2 files changed, 41 insertions(+), 34 deletions(-) diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index d504bc296349..6d69f59e56cf 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2260,17 +2260,15 @@ static struct crt_sd_ctxt * create_sd_buf(umode_t mode, bool set_owner, unsigned int *len) { struct crt_sd_ctxt *buf; - struct cifs_ace *pace; - unsigned int sdlen, acelen; + __u8 *ptr, *aclptr; + unsigned int acelen; unsigned int owner_offset = 0; unsigned int group_offset = 0; + struct smb3_acl acl; - *len = roundup(sizeof(struct crt_sd_ctxt) + (sizeof(struct cifs_ace) * 2), 8); + *len = roundup(sizeof(struct crt_sd_ctxt) + (sizeof(struct cifs_ace) * 4), 8); if (set_owner) { - /* offset fields are from beginning of security descriptor not of create context */ - owner_offset = sizeof(struct smb3_acl) + (sizeof(struct cifs_ace) * 2); - /* sizeof(struct owner_group_sids) is already multiple of 8 so no need to round */ *len += sizeof(struct owner_group_sids); } @@ -2279,26 +2277,22 @@ create_sd_buf(umode_t mode, bool set_owner, unsigned int *len) if (buf == NULL) return buf; + ptr = (__u8 *)&buf[1]; if (set_owner) { + /* offset fields are from beginning of security descriptor not of create context */ + owner_offset = ptr - (__u8 *)&buf->sd; buf->sd.OffsetOwner = cpu_to_le32(owner_offset); - group_offset = owner_offset + sizeof(struct owner_sid); + group_offset = owner_offset + offsetof(struct owner_group_sids, group); buf->sd.OffsetGroup = cpu_to_le32(group_offset); + + setup_owner_group_sids(ptr); + ptr += sizeof(struct owner_group_sids); } else { buf->sd.OffsetOwner = 0; buf->sd.OffsetGroup = 0; } - sdlen = sizeof(struct smb3_sd) + sizeof(struct smb3_acl) + - 2 * sizeof(struct cifs_ace); - if (set_owner) { - sdlen += sizeof(struct owner_group_sids); - setup_owner_group_sids(owner_offset + sizeof(struct create_context) + 8 /* name */ - + (char *)buf); - } - - buf->ccontext.DataOffset = cpu_to_le16(offsetof - (struct crt_sd_ctxt, sd)); - buf->ccontext.DataLength = cpu_to_le32(sdlen); + buf->ccontext.DataOffset = cpu_to_le16(offsetof(struct crt_sd_ctxt, sd)); buf->ccontext.NameOffset = cpu_to_le16(offsetof(struct crt_sd_ctxt, Name)); buf->ccontext.NameLength = cpu_to_le16(4); /* SMB2_CREATE_SD_BUFFER_TOKEN is "SecD" */ @@ -2307,6 +2301,7 @@ create_sd_buf(umode_t mode, bool set_owner, unsigned int *len) buf->Name[2] = 'c'; buf->Name[3] = 'D'; buf->sd.Revision = 1; /* Must be one see MS-DTYP 2.4.6 */ + /* * ACL is "self relative" ie ACL is stored in contiguous block of memory * and "DP" ie the DACL is present @@ -2314,28 +2309,42 @@ create_sd_buf(umode_t mode, bool set_owner, unsigned int *len) buf->sd.Control = cpu_to_le16(ACL_CONTROL_SR | ACL_CONTROL_DP); /* offset owner, group and Sbz1 and SACL are all zero */ - buf->sd.OffsetDacl = cpu_to_le32(sizeof(struct smb3_sd)); - buf->acl.AclRevision = ACL_REVISION; /* See 2.4.4.1 of MS-DTYP */ + buf->sd.OffsetDacl = cpu_to_le32(ptr - (__u8 *)&buf->sd); + /* Ship the ACL for now. we will copy it into buf later. */ + aclptr = ptr; + ptr += sizeof(struct cifs_acl); + + acl.AclRevision = ACL_REVISION; /* See 2.4.4.1 of MS-DTYP */ + acl.AclSize = sizeof(struct smb3_acl); + acl.AceCount = 0; /* create one ACE to hold the mode embedded in reserved special SID */ - pace = (struct cifs_ace *)(sizeof(struct crt_sd_ctxt) + (char *)buf); - acelen = setup_special_mode_ACE(pace, (__u64)mode); + acelen = setup_special_mode_ACE((struct cifs_ace *)ptr, (__u64)mode); + ptr += acelen; + acl.AclSize += acelen; + acl.AceCount++; if (set_owner) { /* we do not need to reallocate buffer to add the two more ACEs. plenty of space */ - pace = (struct cifs_ace *)(acelen + (sizeof(struct crt_sd_ctxt) + (char *)buf)); - acelen += setup_special_user_owner_ACE(pace); - /* it does not appear necessary to add an ACE for the NFS group SID */ - buf->acl.AceCount = cpu_to_le16(3); - } else - buf->acl.AceCount = cpu_to_le16(2); + acelen = setup_special_user_owner_ACE((struct cifs_ace *)ptr); + ptr += acelen; + acl.AclSize += acelen; + acl.AceCount++; + } /* and one more ACE to allow access for authenticated users */ - pace = (struct cifs_ace *)(acelen + (sizeof(struct crt_sd_ctxt) + - (char *)buf)); - acelen += setup_authusers_ACE(pace); + acelen = setup_authusers_ACE((struct cifs_ace *)ptr); + ptr += acelen; + acl.AclSize += acelen; + acl.AceCount++; + + + acl.AceCount = cpu_to_le16(acl.AceCount); + acl.AclSize = cpu_to_le16(acl.AclSize); + memcpy(aclptr, &acl, sizeof(struct cifs_acl)); - buf->acl.AclSize = cpu_to_le16(sizeof(struct cifs_acl) + acelen); + buf->ccontext.DataLength = cpu_to_le32(ptr - (__u8 *)&buf->sd); + *len = ptr - (__u8 *)buf; return buf; } diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h index 05b010e5a061..6a1b9cb92aa6 100644 --- a/fs/cifs/smb2pdu.h +++ b/fs/cifs/smb2pdu.h @@ -962,8 +962,6 @@ struct crt_sd_ctxt { struct create_context ccontext; __u8 Name[8]; struct smb3_sd sd; - struct smb3_acl acl; - /* Followed by at least 4 ACEs */ } __packed; -- 2.25.1