* [bug report] cifs: refactor create_sd_buf() and and avoid corrupting the buffer
@ 2021-09-17 11:58 Dan Carpenter
2021-09-23 21:08 ` Steve French
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2021-09-17 11:58 UTC (permalink / raw)
To: lsahlber; +Cc: linux-cifs
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] cifs: refactor create_sd_buf() and and avoid corrupting the buffer
2021-09-17 11:58 [bug report] cifs: refactor create_sd_buf() and and avoid corrupting the buffer Dan Carpenter
@ 2021-09-23 21:08 ` Steve French
0 siblings, 0 replies; 2+ messages in thread
From: Steve French @ 2021-09-23 21:08 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Ronnie Sahlberg, CIFS
[-- 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
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-09-23 21:09 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17 11:58 [bug report] cifs: refactor create_sd_buf() and and avoid corrupting the buffer Dan Carpenter
2021-09-23 21:08 ` Steve French
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).