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

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

[Bjoern Doebel]

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.

Fixes: bc3e9dd9d104 ("cifs: Change SIDs in ACEs while transferring file ownership.")
Cc: stable@vger.kernel.org
Signed-off-by: Bjoern Doebel <doebel@amazon.de>
Assisted-by: Kiro:claude-opus-4.6
---
 fs/smb/client/cifsacl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index 07cf0e5782337..6d572dd995d79 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -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

Re: [PATCH] smb: client: fix DACL-rewrite heap overflow in id_mode_to_cifs_acl()

[Steve French]

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?

On Thu, Jul 9, 2026 at 10:55 AM Bjoern Doebel <doebel@amazon.de> wrote:
>
> 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.
>
> Fixes: bc3e9dd9d104 ("cifs: Change SIDs in ACEs while transferring file ownership.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
> Assisted-by: Kiro:claude-opus-4.6
> ---
>  fs/smb/client/cifsacl.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
> index 07cf0e5782337..6d572dd995d79 100644
> --- a/fs/smb/client/cifsacl.c
> +++ b/fs/smb/client/cifsacl.c
> @@ -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
>
>


-- 
Thanks,

Steve

Re: [PATCH] smb: client: fix DACL-rewrite heap overflow in id_mode_to_cifs_acl()

[Bjoern Doebel]

Hi Steve,

On Thu, Jul 09, 2026 at 05:15:40PM -0500, Steve French wrote:
> 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?

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

> On Thu, Jul 9, 2026 at 10:55 AM Bjoern Doebel <doebel@amazon.de> wrote:
> >
> > 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.
> >
> > Fixes: bc3e9dd9d104 ("cifs: Change SIDs in ACEs while transferring file ownership.")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Bjoern Doebel <doebel@amazon.de>
> > Assisted-by: Kiro:claude-opus-4.6
> > ---
> >  fs/smb/client/cifsacl.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
> > index 07cf0e5782337..6d572dd995d79 100644
> > --- a/fs/smb/client/cifsacl.c
> > +++ b/fs/smb/client/cifsacl.c
> > @@ -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
> >
> >
> 
> 
> -- 
> Thanks,
> 
> Steve

[PATCH] smb: client: fix heap overflow in DACL owner/group rewrite

[Bjoern Doebel]

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)

Cc: stable@vger.kernel.org
Fixes: 5c3564852c58 ("cifs: Minimize the number of cifs_acl memory allocations")
Assisted-by: Kiro:claude-opus-4.6
Signed-off-by: Bjoern Doebel <doebel@amazon.de>

---
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(-)

diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index 12005f46307de..2d785a3039585 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -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

Re: [PATCH] smb: client: fix heap overflow in DACL owner/group rewrite

[Namjae Jeon]

> @@ -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.
Since you mentioned replace_sids_and_copy_aces(), please check whether
replace_sids_and_copy_aces() also has a potential overflow issue.

So for this patch,
Reviewed-by: Namjae Jeon <linkinjeon@kernel.org>

Plus, there is a checkpatch warning from this patch.
WARNING: Unknown commit id '5c3564852c58', maybe rebased or not pulled?
#78:
Fixes: 5c3564852c58 ("cifs: Minimize the number of cifs_acl memory allocations")
total: 0 errors, 1 warnings, 18 lines checked

Thanks.

Re: [PATCH] smb: client: fix heap overflow in DACL owner/group rewrite

[Bjoern Doebel]

Hi Namjae,

On Sat, Sep 05, 2026 at 10:15:37AM +0900, Namjae Jeon wrote:
> > @@ -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.
> Since you mentioned replace_sids_and_copy_aces(), please check whether
> replace_sids_and_copy_aces() also has a potential overflow issue.

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

[PATCH v3 0/2] smb: client: fix DACL rewrite overflows

[Bjoern Doebel]

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

[PATCH v3 1/2] smb: client: fix heap overflow in DACL owner/group rewrite

[Bjoern Doebel]

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)

Cc: stable@vger.kernel.org
Fixes: bc3e9dd9d104 ("cifs: Change SIDs in ACEs while transferring file ownership.")
Assisted-by: Kiro:claude-opus-4.6
Signed-off-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Namjae Jeon <linkinjeon@kernel.org>

---
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(-)

diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index 12005f46307de..2d785a3039585 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -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

[PATCH v3 2/2] smb: client: fail DACL rewrite when the new DACL exceeds 64K

[Bjoern Doebel]

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.

Suggested-by: Namjae Jeon <linkinjeon@kernel.org>
Cc: stable@vger.kernel.org
Fixes: f5065508897a ("cifs: Retain old ACEs when converting between mode bits and ACL.")
Assisted-by: Kiro:claude-opus-5
Signed-off-by: Bjoern Doebel <doebel@amazon.de>

---
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(-)

diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index 2d785a3039585..7c3c06cd5db3a 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -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

Re: [PATCH v3 2/2] smb: client: fail DACL rewrite when the new DACL exceeds 64K

[Namjae Jeon]

On Wed, Sep 9, 2026 at 1:10 AM Bjoern Doebel <doebel@amazon.de> wrote:
>
> 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.
>
> Suggested-by: Namjae Jeon <linkinjeon@kernel.org>
> Cc: stable@vger.kernel.org
> Fixes: f5065508897a ("cifs: Retain old ACEs when converting between mode bits and ACL.")
> Assisted-by: Kiro:claude-opus-5
> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Namjae Jeon <linkinjeon@kernel.org>
Thanks!

Re: [PATCH v3 0/2] smb: client: fix DACL rewrite overflows

[Paulo Alcantara]

Bjoern Doebel <doebel@amazon.de> writes:

> 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.
> ...

Applied.