smb: client: fix DACL-rewrite heap overflow in id_mode_to_cifs_acl()

Budget the destination buffer for the worst case in both branches:
every rewritten ACE may take sizeof(struct smb_ace) bytes (which already accounts for an smb_sid with SID_MAX_SUB_AUTHORITIES sub-authorities), plus the smb_acl header that replace_sids_and_copy_aces() emits.

---
 fs/smb/client/cifsacl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

index 07cf0e5782337..6d572dd995d79 100644
@@ -1812,11 +1812,13 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
 				cifs_put_tlink(tlink);
 				return rc;
 			}
-			if (mode_from_sid)
-				nsecdesclen +=
-					le16_to_cpu(dacl_ptr->num_aces) * sizeof(struct smb_ace);
-			else /* cifsacl */
-				nsecdesclen += le16_to_cpu(dacl_ptr->size);
+			/*
+			 * Worst case: every ACE is rewritten with a new SID of
+			 * SID_MAX_SUB_AUTHORITIES sub-auths -> sizeof(smb_ace) each,
+			 * plus the smb_acl header replace_sids_and_copy_aces() emits.
+			 */
+			nsecdesclen += sizeof(struct smb_acl) +
+				le16_to_cpu(dacl_ptr->num_aces) * sizeof(struct smb_ace);
 		}
 	}
 

2.50.1

When I tried this it changed the length used (for chown with cifsacl mount option) from 88 bytes to 236 bytes
which seems suspicious. Have you been able to reproduce the bug this patch is supposed to fix?

Thanks,

Steve

Hi Steve,

I shared my reproducer script via PM.

This triggered the following KASAN splat in testing (and no longer does with the patch applied):

[   46.251162] ==================================================================
[   46.278880] BUG: KASAN: slab-out-of-bounds in build_sec_desc.constprop.0+0x3010/0x3a70 [cifs]
[   46.319912] Write of size 4 at addr ffff888155556374 by task chown/3439
[   46.351546]
[   46.358846] CPU: 4 UID: 0 PID: 3439 Comm: chown Not tainted 7.1.0-rc6+ #35 PREEMPT(full)
[   46.358850] Hardware name: Amazon EC2 c6i.4xlarge/, BIOS 1.0 10/16/2017
[   46.358853] Call Trace:
[   46.358856]  <TASK>
[   46.358859]  dump_stack_lvl+0x51/0x70
[   46.358866]  print_address_description.constprop.0+0x2c/0x3a0
[   46.358871]  ? build_sec_desc.constprop.0+0x3010/0x3a70 [cifs]
[   46.358941]  print_report+0xb4/0x270
[   46.358944]  ? kasan_addr_to_slab+0x9/0x70
[   46.358948]  kasan_report+0xb4/0xe0
[   46.358951]  ? build_sec_desc.constprop.0+0x3010/0x3a70 [cifs]
[   46.359015]  build_sec_desc.constprop.0+0x3010/0x3a70 [cifs]
[   46.359077]  ? queue_folios_pte_range+0x45c/0x7a0
[   46.359082]  ? __pfx_build_sec_desc.constprop.0+0x10/0x10 [cifs]
[   46.359143]  ? __find_readable_file+0x310/0x540 [cifs]
[   46.359220]  ? kasan_save_track+0x10/0x30
[   46.359222]  ? __kasan_kmalloc+0x7b/0x90
[   46.359226]  id_mode_to_cifs_acl+0x31c/0x760 [cifs]
[   46.359295]  ? __pfx_id_mode_to_cifs_acl+0x10/0x10 [cifs]
[   46.359356]  ? __build_path_from_dentry_optional_prefix+0x176/0x620 [cifs]
[   46.359432]  cifs_setattr_nounix+0xc5b/0x1860 [cifs]
[   46.359505]  ? __pfx_cifs_setattr_nounix+0x10/0x10 [cifs]
[   46.359573]  ? __pfx___vfs_getxattr+0x10/0x10
[   46.359577]  ? __pfx_current_time+0x10/0x10
[   46.359580]  cifs_setattr+0x173/0x2b0 [cifs]
[   46.359648]  notify_change+0x832/0xf20
[   46.359651]  ? __pfx_from_vfsuid+0x10/0x10
[   46.359655]  ? chown_common+0x422/0x5e0
[   46.359658]  chown_common+0x422/0x5e0
[   46.359662]  ? __pfx_chown_common+0x10/0x10
[   46.359665]  ? check_heap_object+0x6f/0x490
[   46.359669]  ? strncpy_from_user+0x3b/0x1f0
[   46.359673]  do_fchownat+0x124/0x160
[   46.359677]  ? __pfx_do_fchownat+0x10/0x10
[   46.359680]  __x64_sys_fchownat+0xb9/0x150
[   46.359684]  ? arch_exit_to_user_mode_prepare.constprop.0+0x95/0xc0
[   46.359688]  do_syscall_64+0xaf/0x550
[   46.359693]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   46.359695] RIP: 0033:0x7fd413b00b0e
[   46.359709] Code: 48 8b 0d ed b2 0f 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 04 01 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d ba b2 0f 00 f7 d8 64 89 01 48
[   46.359712] RSP: 002b:00007ffc3eb973d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000104
[   46.359715] RAX: ffffffffffffffda RBX: 00007ffc3eb97700 RCX: 00007fd413b00b0e
[   46.359717] RDX: 0000000000001770 RSI: 000055e6afa81740 RDI: 00000000ffffff9c
[   46.359719] RBP: 000055e6afa7fb70 R08: 0000000000000000 R09: 0000000000000000
[   46.359720] R10: 00000000ffffffff R11: 0000000000000246 R12: 000055e6afa81740
[   46.359722] R13: 0000000000000001 R14: 00000000ffffff9c R15: 000055e6afa7fb00
[   46.359725]  </TASK>
[   46.359726]
[   47.532861] Allocated by task 3439:
[   47.549557]  kasan_save_stack+0x20/0x40
[   47.567957]  kasan_save_track+0x10/0x30
[   47.586353]  __kasan_kmalloc+0x7b/0x90
[   47.604316]  __kmalloc_noprof+0x1c1/0x530
[   47.623566]  id_mode_to_cifs_acl+0x2d0/0x760 [cifs]
[   47.646748]  cifs_setattr_nounix+0xc5b/0x1860 [cifs]
[   47.670768]  cifs_setattr+0x173/0x2b0 [cifs]
[   47.690945]  notify_change+0x832/0xf20
[   47.708918]  chown_common+0x422/0x5e0
[   47.726468]  do_fchownat+0x124/0x160
[   47.743582]  __x64_sys_fchownat+0xb9/0x150
[   47.763261]  do_syscall_64+0xaf/0x550
[   47.780816]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   47.804785]
[   47.812163] The buggy address belongs to the object at ffff888155556000
[   47.812163]  which belongs to the cache kmalloc-1k of size 1024
[   47.872006] The buggy address is located 0 bytes to the right of
[   47.872006]  allocated 884-byte region [ffff888155556000, ffff888155556374)
[   47.934359]
[   47.941237] The buggy address belongs to the physical page:
[   47.967735] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x155550
[   48.006191] head: order:3 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
[   48.042968] flags: 0x17ffffc0000040(head|node=0|zone=2|lastcpupid=0x1fffff)
[   48.076404] page_type: f5(slab)
[   48.091399] raw: 0017ffffc0000040 ffff888100042dc0 dead000000000100 dead000000000122
[   48.128595] raw: 0000000000000000 0000000800100010 00000000f5000000 0000000000000000
[   48.165814] head: 0017ffffc0000040 ffff888100042dc0 dead000000000100 dead000000000122
[   48.203005] head: 0000000000000000 0000000800100010 00000000f5000000 0000000000000000
[   48.240615] head: 0017ffffc0000003 fffffffffffffe01 00000000ffffffff 00000000ffffffff
[   48.278257] head: ffffffffffffffff 0000000000000000 00000000ffffffff 0000000000000008
[   48.315854] page dumped because: kasan: bad access detected
[   48.342361]
[   48.349658] Memory state around the buggy address:
[   48.372332]  ffff888155556200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   48.407026]  ffff888155556280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   48.441656] >ffff888155556300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04 fc
[   48.476272]                                                              ^
[   48.509238]  ffff888155556380: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[   48.543873]  ffff888155556400: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[   48.578508] ==================================================================

Bjoern

When id_mode_to_cifs_acl rewrites an existing DACL, it allocates a buffer sized according to the on-disk DACL length reported by dacl_ptr->size. However, replace_sids_and_copy_aces may rewrite each ACE with a new owner/group SID obtained from the cifs.idmap upcall. Those SIDs can have up to SID_MAX_SUB_AUTHORITIES (15) sub-authorities, making each ACE up to 76 bytes (sizeof(struct smb_ace)).

If the original DACL contains short SIDs (e.g., 1 sub-authority) while the replacement SIDs are long, the rewritten ACEs overflow the allocation.

Fix this by always budgeting for worst-case SID expansion: allocate sizeof(struct smb_acl) plus num_aces * sizeof(struct smb_ace), which covers the smb_acl header and room for every ACE at maximum SID size. This replaces the previous split logic that used dacl_ptr->size for cifsacl mounts but num_aces * sizeof(struct smb_ace) for mode_from_sid mounts—both paths can trigger the same rewrite and need the same headroom.

KASAN reports this as:

  BUG: KASAN: slab-out-of-bounds in build_sec_desc+0x1e8a/0x2680 [cifs]
  Write of size 4 at addr ffff8881a5e25374 by task chown/5298
  ...
  The buggy address is located 0 bytes to the right of
   allocated 884-byte region [ffff8881a5e25000, ffff8881a5e25374)

---
v2:
- Reword commit message to be more descriptive of what is happening
---

---
 fs/smb/client/cifsacl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

index 12005f46307de..2d785a3039585 100644
@@ -1815,11 +1815,13 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
 				cifs_put_tlink(tlink);
 				return rc;
 			}
-			if (mode_from_sid)
-				nsecdesclen +=
-					le16_to_cpu(dacl_ptr->num_aces) * sizeof(struct smb_ace);
-			else /* cifsacl */
-				nsecdesclen += le16_to_cpu(dacl_ptr->size);
+			/*
+			 * Worst case: every ACE is rewritten with a new SID of
+			 * SID_MAX_SUB_AUTHORITIES sub-auths -> sizeof(smb_ace) each,
+			 * plus the smb_acl header replace_sids_and_copy_aces() emits.
+			 */
+			nsecdesclen += sizeof(struct smb_acl) +
+				le16_to_cpu(dacl_ptr->num_aces) * sizeof(struct smb_ace);
 		}
 	}
 

2.50.1

Hi Namjae,

That looks sensible. I'll send a followup v3 (the original patch also needs an update to the Fixes: tag, as you point out.)

Bjoern

Two overflow fixes in the CIFS DACL owner/group rewrite path:

1. Patch 1 fixes a heap overflow when copying ACEs whose total size exceeds what kmalloc() allocated. The allocation used the old (pre-rewrite) ACE count, but the copy loop iterated over the new count.

2. Patch 2 fixes a u16 accumulator overflow in the size calculation that could wrap around for DACLs with 800+ ACEs, causing the check in patch 1 to pass despite the actual size exceeding 64KB. Returns -EOVERFLOW when this would occur.

Changes since v2:
- Patch 1 unchanged (already Reviewed-by Namjae Jeon)
- Patch 2: widened nsize from u16 to u32 and added explicit overflow guard instead of relying on check_add_overflow (which gcc optimized away for a u16 accumulator)
- Patch 2: fixed build_sec_desc() to propagate error from set_chmod_dacl() when it fails (discovered during testing)

Bjoern Doebel (2):
smb: client: fix heap overflow in DACL owner/group rewrite
smb: client: fail DACL rewrite when the new DACL exceeds 64K

 fs/smb/client/cifsacl.c | 51 ++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 18 deletions(-)

2.50.1

When id_mode_to_cifs_acl rewrites an existing DACL, it allocates a buffer sized according to the on-disk DACL length reported by dacl_ptr->size. However, replace_sids_and_copy_aces may rewrite each ACE with a new owner/group SID obtained from the cifs.idmap upcall. Those SIDs can have up to SID_MAX_SUB_AUTHORITIES (15) sub-authorities, making each ACE up to 76 bytes (sizeof(struct smb_ace)).

If the original DACL contains short SIDs (e.g., 1 sub-authority) while the replacement SIDs are long, the rewritten ACEs overflow the allocation.

Fix this by always budgeting for worst-case SID expansion: allocate sizeof(struct smb_acl) plus num_aces * sizeof(struct smb_ace), which covers the smb_acl header and room for every ACE at maximum SID size. This replaces the previous split logic that used dacl_ptr->size for cifsacl mounts but num_aces * sizeof(struct smb_ace) for mode_from_sid
mounts: both paths can trigger the same rewrite and need the same headroom.

KASAN reports this as:

  BUG: KASAN: slab-out-of-bounds in build_sec_desc+0x1e8a/0x2680 [cifs]
  Write of size 4 at addr ffff8881a5e25374 by task chown/5298
  ...
  The buggy address is located 0 bytes to the right of
   allocated 884-byte region [ffff8881a5e25000, ffff8881a5e25374)

---
v3:
- Correct the Fixes: tag; the previous commit id did not exist
- Add Namjae Jeon's Reviewed-by
v2:
- Reword commit message to be more descriptive of what is happening

---
 fs/smb/client/cifsacl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

index 12005f46307de..2d785a3039585 100644
@@ -1815,11 +1815,13 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
 				cifs_put_tlink(tlink);
 				return rc;
 			}
-			if (mode_from_sid)
-				nsecdesclen +=
-					le16_to_cpu(dacl_ptr->num_aces) * sizeof(struct smb_ace);
-			else /* cifsacl */
-				nsecdesclen += le16_to_cpu(dacl_ptr->size);
+			/*
+			 * Worst case: every ACE is rewritten with a new SID of
+			 * SID_MAX_SUB_AUTHORITIES sub-auths -> sizeof(smb_ace) each,
+			 * plus the smb_acl header replace_sids_and_copy_aces() emits.
+			 */
+			nsecdesclen += sizeof(struct smb_acl) +
+				le16_to_cpu(dacl_ptr->num_aces) * sizeof(struct smb_ace);
 		}
 	}
 

2.50.1

replace_sids_and_copy_aces() and set_chmod_dacl() accumulate the size of the DACL they build in a u16. That accumulator can wrap.

validate_dacl() caps num_aces at (dacl_size - sizeof(struct smb_acl)) / 20, i.e. 3276 for a maximally sized DACL, while each rewritten ACE can grow to sizeof(struct smb_ace) (76 bytes) once its SID is replaced with one carrying SID_MAX_SUB_AUTHORITIES sub-authorities. The worst case is therefore sizeof(struct smb_acl) + 3276 * 76 = 248984 bytes, far beyond what a u16 can hold. A wraparound is reached with 863 ACEs.

After the wraparound, ndacl_ptr->size becomes meaningless and the offset will point anywhere in the ACE array. As a result, we will see corruption of the DACL, which then gets sent to the server. This is not an out-of-bounds write as the allocation now covers the worst-case expansion, so writes will always go into the buffer.

Adjust the code to use a u32 internally and return -EOVERFLOW in the overflow case. The operation must be refused, because a DACL can only hold 2^16-1 bytes on the wire and larger DACLs cannot be represented.

set_chmod_dacl() carries the same pattern and is fixed the same way. It only wraps once the source DACL comes within roughly 380 bytes of the 64K ceiling, but the failure mode is identical.

---
v3:
- New patch, following Namjae Jeon's review question on v2 about whether replace_sids_and_copy_aces() has a similar overflow

---
 fs/smb/client/cifsacl.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

index 2d785a3039585..7c3c06cd5db3a 100644
@@ -1081,13 +1081,13 @@ unsigned int setup_special_user_owner_ACE(struct smb_ace *pntace)
 static void populate_new_aces(char *nacl_base,
 		struct smb_sid *pownersid,
 		struct smb_sid *pgrpsid,
-		__u64 *pnmode, u16 *pnum_aces, u16 *pnsize,
+		__u64 *pnmode, u16 *pnum_aces, u32 *pnsize,
 		bool modefromsid,
 		bool posix)
 {
 	__u64 nmode;
 	u16 num_aces = 0;
-	u16 nsize = 0;
+	u32 nsize = 0;
 	__u64 user_mode;
 	__u64 group_mode;
 	__u64 other_mode;

@@ -1186,17 +1186,17 @@ static void populate_new_aces(char *nacl_base,
 	*pnsize = nsize;
 }
 
-static __u16 replace_sids_and_copy_aces(struct smb_acl *pdacl, struct smb_acl *pndacl,
-		struct smb_sid *pownersid, struct smb_sid *pgrpsid,
-		struct smb_sid *pnownersid, struct smb_sid *pngrpsid,
-		int *aclflag)
+static int replace_sids_and_copy_aces(struct smb_acl *pdacl, struct smb_acl *pndacl,
+				      struct smb_sid *pownersid, struct smb_sid *pgrpsid,
+				      struct smb_sid *pnownersid, struct smb_sid *pngrpsid,
+				      int *aclflag, u16 *pnsize)
 {
 	int i;
 	u16 size = 0;
 	struct smb_ace *pntace = NULL;
 	char *acl_base = NULL;
 	u16 src_num_aces = 0;
-	u16 nsize = 0;
+	u32 nsize = 0;
 	struct smb_ace *pnntace = NULL;
 	char *nacl_base = NULL;
 	u16 ace_size = 0;

@@ -1225,9 +1225,12 @@ static __u16 replace_sids_and_copy_aces(struct smb_acl *pdacl, struct smb_acl *p
 
 		size += le16_to_cpu(pntace->size);
 		nsize += ace_size;
+		if (nsize > U16_MAX)
+			return -EOVERFLOW;
 	}
 
-	return nsize;
+	*pnsize = nsize;
+	return 0;
 }
 
 static int set_chmod_dacl(struct smb_acl *pdacl, struct smb_acl *pndacl,

@@ -1239,7 +1242,7 @@ static int set_chmod_dacl(struct smb_acl *pdacl, struct smb_acl *pndacl,
 	struct smb_ace *pntace = NULL;
 	char *acl_base = NULL;
 	u16 src_num_aces = 0;
-	u16 nsize = 0;
+	u32 nsize = 0;
 	struct smb_ace *pnntace = NULL;
 	char *nacl_base = NULL;
 	u16 num_aces = 0;

@@ -1290,6 +1293,8 @@ static int set_chmod_dacl(struct smb_acl *pdacl, struct smb_acl *pndacl,
 
 		nsize += cifs_copy_ace(pnntace, pntace, NULL);
 		num_aces++;
+		if (nsize > U16_MAX)
+			return -EOVERFLOW;
 
 next_ace:
 		size += le16_to_cpu(pntace->size);

@@ -1306,6 +1311,10 @@ static int set_chmod_dacl(struct smb_acl *pdacl, struct smb_acl *pndacl,
 	}
 
 finalize_dacl:
+	/* The DACL size field is 16-bit on the wire, see MS-DTYP 2.4.5 */
+	if (nsize > U16_MAX)
+		return -EOVERFLOW;
+
 	pndacl->num_aces = cpu_to_le16(num_aces);
 	pndacl->size = cpu_to_le16(nsize);
 

@@ -1451,6 +1460,8 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
 
 		rc = set_chmod_dacl(dacl_ptr, ndacl_ptr, owner_sid_ptr, group_sid_ptr,
 				    pnmode, mode_from_sid, posix);
+		if (rc)
+			return rc;
 
 		sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
 		/* copy the non-dacl portion of secdesc */

@@ -1526,10 +1537,12 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
 
 		if (dacloffset) {
 			/* Replace ACEs for old owner with new one */
-			size = replace_sids_and_copy_aces(dacl_ptr, ndacl_ptr,
-					owner_sid_ptr, group_sid_ptr,
-					nowner_sid_ptr, ngroup_sid_ptr,
-					aclflag);
+			rc = replace_sids_and_copy_aces(dacl_ptr, ndacl_ptr,
+							owner_sid_ptr, group_sid_ptr,
+							nowner_sid_ptr, ngroup_sid_ptr,
+							aclflag, &size);
+			if (rc)
+				goto chown_chgrp_exit;
 			ndacl_ptr->size = cpu_to_le16(size);
 		}
 

2.50.1

Thanks!

Applied.