[PATCH] ksmbd: validate num_aces and harden ACE walk in smb_inherit_dacl()

[PATCH] ksmbd: validate num_aces and harden ACE walk in smb_inherit_dacl()

[Michael Bommarito]

Another one on the smbd side this time. smb_inherit_dacl() trusts
the on-disk num_aces value from the parent directory's DACL xattr
and uses it to size a heap allocation:

  aces_base = kmalloc(sizeof(struct smb_ace) * num_aces * 2, ...);

num_aces is a u16 read from le16_to_cpu(parent_pdacl->num_aces)
without checking that it is consistent with the declared pdacl_size.
An authenticated client that can set a crafted DACL on a parent
directory can declare num_aces = 65535 while providing minimal actual
ACE data.  This causes a ~2.6 MB allocation (not kzalloc, so
uninitialized) that the subsequent loop only partially populates, and
may also overflow the three-way size_t multiply on 32-bit kernels.

Additionally, the ACE walk loop uses the weaker
offsetof(struct smb_ace, access_req) minimum size check rather than
the minimum valid on-wire ACE size, and does not reject ACEs whose
declared size is below the minimum.

Reproduced the ACE walk OOB under UML + KASAN by constructing a
12-byte DACL (smb_acl(8) + 4-byte undersized ACE with size=4,
num_aces=1).  The old 4-byte guard passes, then reading
ace->access_req at offset 4 within the ACE triggers:

  BUG: KASAN: slab-out-of-bounds in kcifs3_test_inherit_dacl_old
  Read of size 4 at addr ... by task mount.nfs4/220

Confirmed clean exit without splat after patch applied: the new
16-byte minimum (offsetof(smb_ace, sid) + CIFS_SID_BASE_SIZE)
rejects the undersized ACE before any field read.

Fix by:

  1. Validating num_aces against pdacl_size using the same formula
     applied in parse_dacl() by commit 1b8b67f3c5e5169535e2
     ("ksmbd: fix incorrect validation for num_aces field of
     smb_acl").

  2. Replacing the raw kmalloc(sizeof * num_aces * 2) with
     kmalloc_array(num_aces * 2, sizeof(...)) for overflow-safe
     allocation.

  3. Tightening the per-ACE loop guard to require the minimum valid
     ACE size (offsetof(smb_ace, sid) + CIFS_SID_BASE_SIZE) and
     rejecting under-sized ACEs, matching the hardening in
     smb_check_perm_dacl() and parse_dacl().

Let me know if you want 2/2 instead of this single patch.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 fs/smb/server/smbacl.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index d5943256c071..fc4fcd48d6c9 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1105,8 +1105,25 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
 		goto free_parent_pntsd;
 	}
 
-	aces_base = kmalloc(sizeof(struct smb_ace) * num_aces * 2,
-			    KSMBD_DEFAULT_GFP);
+	aces_size = pdacl_size - sizeof(struct smb_acl);
+
+	/*
+	 * Validate num_aces against the DACL payload before allocating.
+	 * Each ACE must be at least as large as its fixed-size header
+	 * (up to the SID base), so num_aces cannot exceed the payload
+	 * divided by the minimum ACE size.  This mirrors the check in
+	 * parse_dacl() added by commit 1b8b67f3c5e5 ("ksmbd: fix
+	 * incorrect validation for num_aces field of smb_acl").
+	 */
+	if (num_aces > aces_size / (offsetof(struct smb_ace, sid) +
+				    offsetof(struct smb_sid, sub_auth) +
+				    sizeof(__le16))) {
+		rc = -EINVAL;
+		goto free_parent_pntsd;
+	}
+
+	aces_base = kmalloc_array(num_aces * 2, sizeof(struct smb_ace),
+				  KSMBD_DEFAULT_GFP);
 	if (!aces_base) {
 		rc = -ENOMEM;
 		goto free_parent_pntsd;
@@ -1115,7 +1132,6 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
 	aces = (struct smb_ace *)aces_base;
 	parent_aces = (struct smb_ace *)((char *)parent_pdacl +
 			sizeof(struct smb_acl));
-	aces_size = acl_len - sizeof(struct smb_acl);
 
 	if (pntsd_type & DACL_AUTO_INHERITED)
 		inherited_flags = INHERITED_ACE;
@@ -1123,11 +1139,14 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
 	for (i = 0; i < num_aces; i++) {
 		int pace_size;
 
-		if (offsetof(struct smb_ace, access_req) > aces_size)
+		if (aces_size < offsetof(struct smb_ace, sid) +
+		    CIFS_SID_BASE_SIZE)
 			break;
 
 		pace_size = le16_to_cpu(parent_aces->size);
-		if (pace_size > aces_size)
+		if (pace_size > aces_size ||
+		    pace_size < offsetof(struct smb_ace, sid) +
+				CIFS_SID_BASE_SIZE)
 			break;
 
 		aces_size -= pace_size;
-- 
2.53.0

Re: [PATCH] ksmbd: validate num_aces and harden ACE walk in smb_inherit_dacl()

[Namjae Jeon]

On Fri, Apr 17, 2026 at 5:05 AM Michael Bommarito
<michael.bommarito@gmail.com> wrote:
>
> Another one on the smbd side this time. smb_inherit_dacl() trusts
> the on-disk num_aces value from the parent directory's DACL xattr
> and uses it to size a heap allocation:
>
>   aces_base = kmalloc(sizeof(struct smb_ace) * num_aces * 2, ...);
>
> num_aces is a u16 read from le16_to_cpu(parent_pdacl->num_aces)
> without checking that it is consistent with the declared pdacl_size.
> An authenticated client that can set a crafted DACL on a parent
> directory can declare num_aces = 65535 while providing minimal actual
> ACE data.  This causes a ~2.6 MB allocation (not kzalloc, so
> uninitialized) that the subsequent loop only partially populates, and
> may also overflow the three-way size_t multiply on 32-bit kernels.
>
> Additionally, the ACE walk loop uses the weaker
> offsetof(struct smb_ace, access_req) minimum size check rather than
> the minimum valid on-wire ACE size, and does not reject ACEs whose
> declared size is below the minimum.
>
> Reproduced the ACE walk OOB under UML + KASAN by constructing a
> 12-byte DACL (smb_acl(8) + 4-byte undersized ACE with size=4,
> num_aces=1).  The old 4-byte guard passes, then reading
> ace->access_req at offset 4 within the ACE triggers:
>
>   BUG: KASAN: slab-out-of-bounds in kcifs3_test_inherit_dacl_old
>   Read of size 4 at addr ... by task mount.nfs4/220
There is no kcifs3_test_inherit_dacl_old function in ksmbd. How did
you reproduce the problem?

Re: [PATCH] ksmbd: validate num_aces and harden ACE walk in smb_inherit_dacl()

[Michael Bommarito]

On Thu, Apr 16, 2026 at 10:47 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
> There is no kcifs3_test_inherit_dacl_old function in ksmbd. How did
> you reproduce the problem?

Sorry for the confusing splat.  I pulled the pre-fix ACE-walk loop
from smb_inherit_dacl()  to simplify the setup in a test module and
re-used names from another harness.  _old keeps the original weak
offset guard, _new uses the tightened size, and the patched version
survives.  If you want, I can run through a full repro with qemu
tomorrow using the real paths.

Re: [PATCH] ksmbd: validate num_aces and harden ACE walk in smb_inherit_dacl()

[Namjae Jeon]

> diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
> index d5943256c071..fc4fcd48d6c9 100644
> --- a/fs/smb/server/smbacl.c
> +++ b/fs/smb/server/smbacl.c
> @@ -1105,8 +1105,25 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
>                 goto free_parent_pntsd;
>         }
>
> -       aces_base = kmalloc(sizeof(struct smb_ace) * num_aces * 2,
> -                           KSMBD_DEFAULT_GFP);
> +       aces_size = pdacl_size - sizeof(struct smb_acl);
> +
> +       /*
> +        * Validate num_aces against the DACL payload before allocating.
> +        * Each ACE must be at least as large as its fixed-size header
> +        * (up to the SID base), so num_aces cannot exceed the payload
> +        * divided by the minimum ACE size.  This mirrors the check in
> +        * parse_dacl() added by commit 1b8b67f3c5e5 ("ksmbd: fix
> +        * incorrect validation for num_aces field of smb_acl").
> +        */
Please remove the specific commit hash and patch name in the comments.
Thanks.

[PATCH v2] ksmbd: validate num_aces and harden ACE walk in smb_inherit_dacl()

[Michael Bommarito]

smb_inherit_dacl() trusts the on-disk num_aces value from the parent
directory's DACL xattr and uses it to size a heap allocation:

  aces_base = kmalloc(sizeof(struct smb_ace) * num_aces * 2, ...);

num_aces is a u16 read from le16_to_cpu(parent_pdacl->num_aces)
without checking that it is consistent with the declared pdacl_size.
An authenticated client whose parent directory's security.NTACL is
tampered (e.g. via offline xattr corruption or a concurrent path that
bypasses parse_dacl()) can present num_aces = 65535 with minimal
actual ACE data.  This causes a ~8 MB allocation (not kzalloc, so
uninitialized) that the subsequent loop only partially populates, and
may also overflow the three-way size_t multiply on 32-bit kernels.

Additionally, the ACE walk loop uses the weaker
offsetof(struct smb_ace, access_req) minimum size check rather than
the minimum valid on-wire ACE size, and does not reject ACEs whose
declared size is below the minimum.

Reproduced on UML + KASAN + LOCKDEP against the real ksmbd code path.
A legitimate mount.cifs client creates a parent directory over SMB
(ksmbd writes a valid security.NTACL xattr), then the NTACL blob on
the backing filesystem is rewritten to set num_aces = 0xFFFF while
keeping the posix_acl_hash bytes intact so ksmbd_vfs_get_sd_xattr()'s
hash check still passes.  A subsequent SMB2 CREATE of a child under
that parent drives smb2_open() into smb_inherit_dacl() (share has
"vfs objects = acl_xattr" set), which fails the page allocator:

  WARNING: mm/page_alloc.c:5226 at __alloc_frozen_pages_noprof+0x46c/0x9c0
  Workqueue: ksmbd-io handle_ksmbd_work
   __alloc_frozen_pages_noprof+0x46c/0x9c0
   ___kmalloc_large_node+0x68/0x130
   __kmalloc_large_node_noprof+0x24/0x70
   __kmalloc_noprof+0x4c9/0x690
   smb_inherit_dacl+0x394/0x2430
   smb2_open+0x595d/0xabe0
   handle_ksmbd_work+0x3d3/0x1140

With the patch applied the added guard rejects the tampered value
with -EINVAL before any large allocation runs, smb2_open() falls back
to smb2_create_sd_buffer(), and the child is created with a default
SD.  No warning, no splat.

Fix by:

  1. Validating num_aces against pdacl_size using the same formula
     applied in parse_dacl().

  2. Replacing the raw kmalloc(sizeof * num_aces * 2) with
     kmalloc_array(num_aces * 2, sizeof(...)) for overflow-safe
     allocation.

  3. Tightening the per-ACE loop guard to require the minimum valid
     ACE size (offsetof(smb_ace, sid) + CIFS_SID_BASE_SIZE) and
     rejecting under-sized ACEs, matching the hardening in
     smb_check_perm_dacl() and parse_dacl().

v1 -> v2:
  - Replace the synthetic test-module splat in the changelog with a
    real-path UML + KASAN reproduction driven through mount.cifs and
    SMB2 CREATE; Namjae flagged the kcifs3_test_inherit_dacl_old name
    in v1 since it does not exist in ksmbd.
  - Drop the commit-hash citation from the code comment per Namjae's
    review; keep the parse_dacl() pointer.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 fs/smb/server/smbacl.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index d5943256c071..4a341c1f6630 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1105,8 +1105,24 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
 		goto free_parent_pntsd;
 	}
 
-	aces_base = kmalloc(sizeof(struct smb_ace) * num_aces * 2,
-			    KSMBD_DEFAULT_GFP);
+	aces_size = pdacl_size - sizeof(struct smb_acl);
+
+	/*
+	 * Validate num_aces against the DACL payload before allocating.
+	 * Each ACE must be at least as large as its fixed-size header
+	 * (up to the SID base), so num_aces cannot exceed the payload
+	 * divided by the minimum ACE size.  This mirrors the existing
+	 * check in parse_dacl().
+	 */
+	if (num_aces > aces_size / (offsetof(struct smb_ace, sid) +
+				    offsetof(struct smb_sid, sub_auth) +
+				    sizeof(__le16))) {
+		rc = -EINVAL;
+		goto free_parent_pntsd;
+	}
+
+	aces_base = kmalloc_array(num_aces * 2, sizeof(struct smb_ace),
+				  KSMBD_DEFAULT_GFP);
 	if (!aces_base) {
 		rc = -ENOMEM;
 		goto free_parent_pntsd;
@@ -1115,7 +1131,6 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
 	aces = (struct smb_ace *)aces_base;
 	parent_aces = (struct smb_ace *)((char *)parent_pdacl +
 			sizeof(struct smb_acl));
-	aces_size = acl_len - sizeof(struct smb_acl);
 
 	if (pntsd_type & DACL_AUTO_INHERITED)
 		inherited_flags = INHERITED_ACE;
@@ -1123,11 +1138,14 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
 	for (i = 0; i < num_aces; i++) {
 		int pace_size;
 
-		if (offsetof(struct smb_ace, access_req) > aces_size)
+		if (aces_size < offsetof(struct smb_ace, sid) +
+		    CIFS_SID_BASE_SIZE)
 			break;
 
 		pace_size = le16_to_cpu(parent_aces->size);
-		if (pace_size > aces_size)
+		if (pace_size > aces_size ||
+		    pace_size < offsetof(struct smb_ace, sid) +
+				CIFS_SID_BASE_SIZE)
 			break;
 
 		aces_size -= pace_size;
-- 
2.53.0

Re: [PATCH v2] ksmbd: validate num_aces and harden ACE walk in smb_inherit_dacl()

[Namjae Jeon]

On Sat, Apr 18, 2026 at 3:46 AM Michael Bommarito
<michael.bommarito@gmail.com> wrote:
>
> smb_inherit_dacl() trusts the on-disk num_aces value from the parent
> directory's DACL xattr and uses it to size a heap allocation:
>
>   aces_base = kmalloc(sizeof(struct smb_ace) * num_aces * 2, ...);
>
> num_aces is a u16 read from le16_to_cpu(parent_pdacl->num_aces)
> without checking that it is consistent with the declared pdacl_size.
> An authenticated client whose parent directory's security.NTACL is
> tampered (e.g. via offline xattr corruption or a concurrent path that
> bypasses parse_dacl()) can present num_aces = 65535 with minimal
> actual ACE data.  This causes a ~8 MB allocation (not kzalloc, so
> uninitialized) that the subsequent loop only partially populates, and
> may also overflow the three-way size_t multiply on 32-bit kernels.
>
> Additionally, the ACE walk loop uses the weaker
> offsetof(struct smb_ace, access_req) minimum size check rather than
> the minimum valid on-wire ACE size, and does not reject ACEs whose
> declared size is below the minimum.
>
> Reproduced on UML + KASAN + LOCKDEP against the real ksmbd code path.
> A legitimate mount.cifs client creates a parent directory over SMB
> (ksmbd writes a valid security.NTACL xattr), then the NTACL blob on
> the backing filesystem is rewritten to set num_aces = 0xFFFF while
> keeping the posix_acl_hash bytes intact so ksmbd_vfs_get_sd_xattr()'s
> hash check still passes.  A subsequent SMB2 CREATE of a child under
> that parent drives smb2_open() into smb_inherit_dacl() (share has
> "vfs objects = acl_xattr" set), which fails the page allocator:
>
>   WARNING: mm/page_alloc.c:5226 at __alloc_frozen_pages_noprof+0x46c/0x9c0
>   Workqueue: ksmbd-io handle_ksmbd_work
>    __alloc_frozen_pages_noprof+0x46c/0x9c0
>    ___kmalloc_large_node+0x68/0x130
>    __kmalloc_large_node_noprof+0x24/0x70
>    __kmalloc_noprof+0x4c9/0x690
>    smb_inherit_dacl+0x394/0x2430
>    smb2_open+0x595d/0xabe0
>    handle_ksmbd_work+0x3d3/0x1140
>
> With the patch applied the added guard rejects the tampered value
> with -EINVAL before any large allocation runs, smb2_open() falls back
> to smb2_create_sd_buffer(), and the child is created with a default
> SD.  No warning, no splat.
>
> Fix by:
>
>   1. Validating num_aces against pdacl_size using the same formula
>      applied in parse_dacl().
>
>   2. Replacing the raw kmalloc(sizeof * num_aces * 2) with
>      kmalloc_array(num_aces * 2, sizeof(...)) for overflow-safe
>      allocation.
>
>   3. Tightening the per-ACE loop guard to require the minimum valid
>      ACE size (offsetof(smb_ace, sid) + CIFS_SID_BASE_SIZE) and
>      rejecting under-sized ACEs, matching the hardening in
>      smb_check_perm_dacl() and parse_dacl().
>
> v1 -> v2:
>   - Replace the synthetic test-module splat in the changelog with a
>     real-path UML + KASAN reproduction driven through mount.cifs and
>     SMB2 CREATE; Namjae flagged the kcifs3_test_inherit_dacl_old name
>     in v1 since it does not exist in ksmbd.
>   - Drop the commit-hash citation from the code comment per Namjae's
>     review; keep the parse_dacl() pointer.
>
> Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-6
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
Applied it to #ksmbd-for-next-next.
Thanks!