Re: [bug report] cifs: refactor create_sd_buf() and and avoid corrupting the buffer

[Steve French <smfrench@gmail.com>]

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

Fix attached.   Harmless in some sense because lengths are the same,
but is easier to read/understand with the change you suggest.


On Fri, Sep 17, 2021 at 9:19 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Ronnie Sahlberg,
>
> The patch ea64370bcae1: "cifs: refactor create_sd_buf() and and avoid
> corrupting the buffer" from Nov 30, 2020, leads to the following
> Smatch static checker warning:
>
>         fs/smbfs_client/smb2pdu.c:2425 create_sd_buf()
>         warn: struct type mismatch 'smb3_acl vs cifs_acl'
>
> fs/smbfs_client/smb2pdu.c
>     2344 static struct crt_sd_ctxt *
>     2345 create_sd_buf(umode_t mode, bool set_owner, unsigned int *len)
>     2346 {
>     2347         struct crt_sd_ctxt *buf;
>     2348         __u8 *ptr, *aclptr;
>     2349         unsigned int acelen, acl_size, ace_count;
>     2350         unsigned int owner_offset = 0;
>     2351         unsigned int group_offset = 0;
>     2352         struct smb3_acl acl;
>                  ^^^^^^^^^^^^^^^^^^^
>
>     2353
>     2354         *len = roundup(sizeof(struct crt_sd_ctxt) + (sizeof(struct cifs_ace) * 4), 8);
>     2355
>     2356         if (set_owner) {
>     2357                 /* sizeof(struct owner_group_sids) is already multiple of 8 so no need to round */
>     2358                 *len += sizeof(struct owner_group_sids);
>     2359         }
>     2360
>     2361         buf = kzalloc(*len, GFP_KERNEL);
>     2362         if (buf == NULL)
>     2363                 return buf;
>     2364
>     2365         ptr = (__u8 *)&buf[1];
>     2366         if (set_owner) {
>     2367                 /* offset fields are from beginning of security descriptor not of create context */
>     2368                 owner_offset = ptr - (__u8 *)&buf->sd;
>     2369                 buf->sd.OffsetOwner = cpu_to_le32(owner_offset);
>     2370                 group_offset = owner_offset + offsetof(struct owner_group_sids, group);
>     2371                 buf->sd.OffsetGroup = cpu_to_le32(group_offset);
>     2372
>     2373                 setup_owner_group_sids(ptr);
>     2374                 ptr += sizeof(struct owner_group_sids);
>     2375         } else {
>     2376                 buf->sd.OffsetOwner = 0;
>     2377                 buf->sd.OffsetGroup = 0;
>     2378         }
>     2379
>     2380         buf->ccontext.DataOffset = cpu_to_le16(offsetof(struct crt_sd_ctxt, sd));
>     2381         buf->ccontext.NameOffset = cpu_to_le16(offsetof(struct crt_sd_ctxt, Name));
>     2382         buf->ccontext.NameLength = cpu_to_le16(4);
>     2383         /* SMB2_CREATE_SD_BUFFER_TOKEN is "SecD" */
>     2384         buf->Name[0] = 'S';
>     2385         buf->Name[1] = 'e';
>     2386         buf->Name[2] = 'c';
>     2387         buf->Name[3] = 'D';
>     2388         buf->sd.Revision = 1;  /* Must be one see MS-DTYP 2.4.6 */
>     2389
>     2390         /*
>     2391          * ACL is "self relative" ie ACL is stored in contiguous block of memory
>     2392          * and "DP" ie the DACL is present
>     2393          */
>     2394         buf->sd.Control = cpu_to_le16(ACL_CONTROL_SR | ACL_CONTROL_DP);
>     2395
>     2396         /* offset owner, group and Sbz1 and SACL are all zero */
>     2397         buf->sd.OffsetDacl = cpu_to_le32(ptr - (__u8 *)&buf->sd);
>     2398         /* Ship the ACL for now. we will copy it into buf later. */
>     2399         aclptr = ptr;
>     2400         ptr += sizeof(struct cifs_acl);
>     2401
>     2402         /* create one ACE to hold the mode embedded in reserved special SID */
>     2403         acelen = setup_special_mode_ACE((struct cifs_ace *)ptr, (__u64)mode);
>     2404         ptr += acelen;
>     2405         acl_size = acelen + sizeof(struct smb3_acl);
>                                             ^^^^^^^^^^^^^^^
>     2406         ace_count = 1;
>     2407
>     2408         if (set_owner) {
>     2409                 /* we do not need to reallocate buffer to add the two more ACEs. plenty of space */
>     2410                 acelen = setup_special_user_owner_ACE((struct cifs_ace *)ptr);
>     2411                 ptr += acelen;
>     2412                 acl_size += acelen;
>     2413                 ace_count += 1;
>     2414         }
>     2415
>     2416         /* and one more ACE to allow access for authenticated users */
>     2417         acelen = setup_authusers_ACE((struct cifs_ace *)ptr);
>     2418         ptr += acelen;
>     2419         acl_size += acelen;
>     2420         ace_count += 1;
>     2421
>     2422         acl.AclRevision = ACL_REVISION; /* See 2.4.4.1 of MS-DTYP */
>     2423         acl.AclSize = cpu_to_le16(acl_size);
>     2424         acl.AceCount = cpu_to_le16(ace_count);
> --> 2425         memcpy(aclptr, &acl, sizeof(struct cifs_acl));
>                                       ^^^^^^^^^^^^^^^^^^^^^^^
> smb3_acl and cifs_acl structs are both 8 bytes, but their data is quite
> different.
>
> (This old warnings are all showing up as new warnings because the file
> was moved).
>
>     2426
>     2427         buf->ccontext.DataLength = cpu_to_le32(ptr - (__u8 *)&buf->sd);
>     2428         *len = roundup(ptr - (__u8 *)buf, 8);
>     2429
>     2430         return buf;
>     2431 }
>
> regards,
> dan carpenter



-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-correct-smb3-ACL-security-descriptor.patch --]
[-- Type: text/x-patch, Size: 1618 bytes --]

From eda56da0f2c5a6dccaa825833f87c3e9be3dc721 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 23 Sep 2021 16:00:31 -0500
Subject: [PATCH] smb3: correct smb3 ACL security descriptor

Address warning:

        fs/smbfs_client/smb2pdu.c:2425 create_sd_buf()
        warn: struct type mismatch 'smb3_acl vs cifs_acl'

Pointed out by Dan Carpenter via smatch code analysis tool

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/smb2pdu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 672ae78e866a..7829c590eeac 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -2397,7 +2397,7 @@ create_sd_buf(umode_t mode, bool set_owner, unsigned int *len)
 	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);
+	ptr += sizeof(struct smb3_acl);
 
 	/* create one ACE to hold the mode embedded in reserved special SID */
 	acelen = setup_special_mode_ACE((struct cifs_ace *)ptr, (__u64)mode);
@@ -2422,7 +2422,7 @@ create_sd_buf(umode_t mode, bool set_owner, unsigned int *len)
 	acl.AclRevision = ACL_REVISION; /* See 2.4.4.1 of MS-DTYP */
 	acl.AclSize = cpu_to_le16(acl_size);
 	acl.AceCount = cpu_to_le16(ace_count);
-	memcpy(aclptr, &acl, sizeof(struct cifs_acl));
+	memcpy(aclptr, &acl, sizeof(struct smb3_acl));
 
 	buf->ccontext.DataLength = cpu_to_le32(ptr - (__u8 *)&buf->sd);
 	*len = roundup(ptr - (__u8 *)buf, 8);
-- 
2.30.2