[PATCH 0/3] ksmbd: harden IPC response arithmetic and ACE walk

[PATCH 0/3] ksmbd: harden IPC response arithmetic and ACE walk

[Michael Bommarito]

Hi,

Three ksmbd patches: two hardening fixes for the kernel <-> mountd
IPC arithmetic, plus one authenticated OOB-read fix in the DACL
ACE walker.  All three reproduced under UML + KASAN on v7.0-rc7.
Patch 3 reproduces end-to-end over loopback SMB2 from a guest
client against UML ksmbd + ksmbd.mountd:

  pre-fix:
    BUG: KASAN: slab-out-of-bounds in compare_sids+0x2b1/0x440
     compare_sids
     smb_check_perm_dacl+0x4fe/0x11a0
     smb2_open+0x4eb2/0xad50
     handle_ksmbd_work+0x3d3/0x1140
    "The buggy address is located 4 bytes to the right of
    allocated 32-byte region" with the allocation trace pointing
    at ndr_decode_v4_ntacl() reading the stored xattr.
  post-fix:
    CREATE returns STATUS_ACCESS_DENIED; no KASAN splat; granted
    bits stay at 0 because the tightened bound rejects ace_size=4
    before compare_sids is called.

Patches 1 and 2 reproduced with in-kernel synthetic triggers that
hand ipc_validate_msg() a response with a wrap-matching size:

  patch 1 (RPC_REQUEST payload_sz):
    pre-fix  returns 0 (u32 wrap bypass)
    post-fix returns -EINVAL (payload_sz > KSMBD_IPC_MAX_PAYLOAD)

  patch 1 + 2 (LOGIN_REQUEST_EXT ngroups=-1):
    pre-fix  returns 0 (signed->size_t wrap matches msg_sz)
    post-fix returns -EINVAL (explicit ngroups<0 gate)

Same threat model as the earlier hardening commits aab98e2dbd64
("ksmbd: fix integer overflows on 32 bit systems") and 6f40e50ceb99
("ksmbd: transport_ipc: validate payload size before reading
handle"): the kernel should not trust arithmetic on attacker-
controlled fields even when those fields come from a cooperating
root daemon or an authenticated client writing an xattr.

Patch 1/3 caps the attacker-controlled fields in ipc_validate_msg()
against the existing KSMBD_IPC_MAX_PAYLOAD / NGROUPS_MAX bounds
before they feed the size-computation arithmetic.  Three cases:

  - KSMBD_EVENT_RPC_REQUEST: sizeof(struct) + resp->payload_sz
    (__u32) can wrap in unsigned int; downstream consumer at
    smb2pdu.c:6742 uses rpc_resp->payload_sz for a memcpy.  Cap
    payload_sz against KSMBD_IPC_MAX_PAYLOAD, matching the
    request-side cap in aab98e2dbd64.
  - KSMBD_EVENT_SHARE_CONFIG_REQUEST: sizeof(struct) +
    resp->payload_sz same class; same cap.
  - KSMBD_EVENT_LOGIN_REQUEST_EXT: resp->ngroups is __s32 signed,
    so the existing > NGROUPS_MAX comparison at user_config.c:59
    misses negative values, and the mul sizeof(gid_t) mixes signed
    and size_t in a surprising way.  Reject ngroups outside the
    signed [0, NGROUPS_MAX] range up front.

Patch 2/3 fixes user_config.c so ksmbd_alloc_user() also rejects
negative ngroups explicitly, independent of ipc_validate_msg.

Patch 3/3 tightens bounds checking in smb_check_perm_dacl()'s two
ACE-walk loops.  Today they only require the 4-byte ACE header to
fit in the remaining DACL buffer; an attacker-declared ace->size
of 4 passes both guards, after which the loop reads access_req
(offset 4) and ace->sid (offset 8+) past the real buffer.
parse_sec_desc() already performs an equivalent check; this patch
brings smb_check_perm_dacl() up to the same bar.

Practical exploitation of patches 1-2 is narrow: the wrap-bypass
requires ksmbd.mountd to send a response crafted around the wrapped
size while preserving consistent field values, and the downstream
kvmalloc almost always fails for u32-wrap sizes.  Patch 3 is
reachable post-auth by any client that can SET an ACL and then
OPEN the affected file.

The patch 3 exploit chain is authenticated but otherwise untrusted:
guest session -> TREE_CONNECT -> CREATE evil.dat -> SET_INFO with a
crafted security descriptor (one ACE with size=4) -> close -> re-open
the file, which triggers smb_check_perm_dacl() in smb2_open().  The
malformed SD is accepted by SET_INFO without validation on the write
side; parsing happens on the next open.

Instrumentation, triggers, client, and both console logs are
available on request.

Michael Bommarito (3):
  ksmbd: cap response sizes in ipc_validate_msg()
  ksmbd: reject negative ngroups in ksmbd_alloc_user()
  ksmbd: require minimum ACE size in smb_check_perm_dacl()

 fs/smb/server/mgmt/user_config.c |  2 +-
 fs/smb/server/smbacl.c           | 17 +++++++++++++----
 fs/smb/server/transport_ipc.c    | 42 +++++++++++++++++++++++++++++-----
 3 files changed, 49 insertions(+), 12 deletions(-)

--
2.53.0

[PATCH 1/3] ksmbd: cap response sizes in ipc_validate_msg()

[Michael Bommarito]

ipc_validate_msg() computes the expected message size for each
response type by adding (or multiplying) attacker-controlled fields
from the daemon response to a fixed struct size in unsigned int
arithmetic.  Three cases can overflow:

  KSMBD_EVENT_RPC_REQUEST:
      msg_sz = sizeof(struct ksmbd_rpc_command) + resp->payload_sz;
  KSMBD_EVENT_SHARE_CONFIG_REQUEST:
      msg_sz = sizeof(struct ksmbd_share_config_response) +
               resp->payload_sz;
  KSMBD_EVENT_LOGIN_REQUEST_EXT:
      msg_sz = sizeof(struct ksmbd_login_response_ext) +
               resp->ngroups * sizeof(gid_t);

resp->payload_sz is __u32 and resp->ngroups is __s32.  Each addition
can wrap in unsigned int; the multiplication by sizeof(gid_t) mixes
signed and size_t, so a negative ngroups is converted to SIZE_MAX
before the multiply.  A wrapped value of msg_sz that happens to
equal entry->msg_sz bypasses the size check on the next line, and
downstream consumers (smb2pdu.c:6742 memcpy using rpc_resp->payload_sz,
kmemdup in ksmbd_alloc_user using resp_ext->ngroups) then trust the
unverified length.

This is the response-side analogue of aab98e2dbd64 ("ksmbd: fix
integer overflows on 32 bit systems"), which hardened the request
side by bounding attacker-controlled lengths against the existing
KSMBD_IPC_MAX_PAYLOAD / NGROUPS_MAX caps.  Apply the same caps on
the response side: reject resp->payload_sz > KSMBD_IPC_MAX_PAYLOAD
for RPC_REQUEST and SHARE_CONFIG_REQUEST, and reject resp->ngroups
outside the signed [0, NGROUPS_MAX] range for LOGIN_REQUEST_EXT.
With those caps the subsequent additions and multiplication are
bounded well below UINT_MAX.

Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Assisted-by: Codex:gpt-5-4
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 fs/smb/server/transport_ipc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/smb/server/transport_ipc.c b/fs/smb/server/transport_ipc.c
--- a/fs/smb/server/transport_ipc.c
+++ b/fs/smb/server/transport_ipc.c
@@ -497,6 +497,8 @@ static int ipc_validate_msg(struct ipc_msg_table_entry *entry)
 	{
 		struct ksmbd_rpc_command *resp = entry->response;

+		if (resp->payload_sz > KSMBD_IPC_MAX_PAYLOAD)
+			return -EINVAL;
 		msg_sz = sizeof(struct ksmbd_rpc_command) + resp->payload_sz;
 		break;
 	}
@@ -513,7 +515,8 @@ static int ipc_validate_msg(struct ipc_msg_table_entry *entry)
 		struct ksmbd_share_config_response *resp = entry->response;

 		if (resp->payload_sz) {
-			if (resp->payload_sz < resp->veto_list_sz)
+			if (resp->payload_sz < resp->veto_list_sz ||
+			    resp->payload_sz > KSMBD_IPC_MAX_PAYLOAD)
 				return -EINVAL;

 			msg_sz = sizeof(struct ksmbd_share_config_response) +
@@ -526,6 +529,8 @@ static int ipc_validate_msg(struct ipc_msg_table_entry *entry)
 		struct ksmbd_login_response_ext *resp = entry->response;

 		if (resp->ngroups) {
+			if (resp->ngroups < 0 || resp->ngroups > NGROUPS_MAX)
+				return -EINVAL;
 			msg_sz = sizeof(struct ksmbd_login_response_ext) +
 					resp->ngroups * sizeof(gid_t);
 		}
--
2.53.0

Re: [PATCH 1/3] ksmbd: cap response sizes in ipc_validate_msg()

[Namjae Jeon]

On Wed, Apr 15, 2026 at 4:15 AM Michael Bommarito
<michael.bommarito@gmail.com> wrote:
>
> ipc_validate_msg() computes the expected message size for each
> response type by adding (or multiplying) attacker-controlled fields
> from the daemon response to a fixed struct size in unsigned int
> arithmetic.  Three cases can overflow:
>
>   KSMBD_EVENT_RPC_REQUEST:
>       msg_sz = sizeof(struct ksmbd_rpc_command) + resp->payload_sz;
>   KSMBD_EVENT_SHARE_CONFIG_REQUEST:
>       msg_sz = sizeof(struct ksmbd_share_config_response) +
>                resp->payload_sz;
>   KSMBD_EVENT_LOGIN_REQUEST_EXT:
>       msg_sz = sizeof(struct ksmbd_login_response_ext) +
>                resp->ngroups * sizeof(gid_t);
>
> resp->payload_sz is __u32 and resp->ngroups is __s32.  Each addition
> can wrap in unsigned int; the multiplication by sizeof(gid_t) mixes
> signed and size_t, so a negative ngroups is converted to SIZE_MAX
> before the multiply.  A wrapped value of msg_sz that happens to
> equal entry->msg_sz bypasses the size check on the next line, and
> downstream consumers (smb2pdu.c:6742 memcpy using rpc_resp->payload_sz,
> kmemdup in ksmbd_alloc_user using resp_ext->ngroups) then trust the
> unverified length.
>
> This is the response-side analogue of aab98e2dbd64 ("ksmbd: fix
> integer overflows on 32 bit systems"), which hardened the request
> side by bounding attacker-controlled lengths against the existing
> KSMBD_IPC_MAX_PAYLOAD / NGROUPS_MAX caps.  Apply the same caps on
> the response side: reject resp->payload_sz > KSMBD_IPC_MAX_PAYLOAD
> for RPC_REQUEST and SHARE_CONFIG_REQUEST, and reject resp->ngroups
> outside the signed [0, NGROUPS_MAX] range for LOGIN_REQUEST_EXT.
> With those caps the subsequent additions and multiplication are
> bounded well below UINT_MAX.
>
> Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-6
> Assisted-by: Codex:gpt-5-4
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
>  fs/smb/server/transport_ipc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/smb/server/transport_ipc.c b/fs/smb/server/transport_ipc.c
> --- a/fs/smb/server/transport_ipc.c
> +++ b/fs/smb/server/transport_ipc.c
> @@ -497,6 +497,8 @@ static int ipc_validate_msg(struct ipc_msg_table_entry *entry)
>         {
>                 struct ksmbd_rpc_command *resp = entry->response;
>
> +               if (resp->payload_sz > KSMBD_IPC_MAX_PAYLOAD)
> +                       return -EINVAL;
However, on the userspace side (ksmbd-tools/mountd/rpc.c), the DCE/RPC
response builder (try_realloc_payload() and ndr_write_bytes())
dynamically grows the payload by 4096 bytes using g_try_realloc() when
preparing responses for calls such as NetShareEnumAll, etc..
This can cause share enumeration failures on servers with many shares.

>                 msg_sz = sizeof(struct ksmbd_rpc_command) + resp->payload_sz;
>                 break;
>         }
> @@ -513,7 +515,8 @@ static int ipc_validate_msg(struct ipc_msg_table_entry *entry)
>                 struct ksmbd_share_config_response *resp = entry->response;
>
>                 if (resp->payload_sz) {
> -                       if (resp->payload_sz < resp->veto_list_sz)
> +                       if (resp->payload_sz < resp->veto_list_sz ||
> +                           resp->payload_sz > KSMBD_IPC_MAX_PAYLOAD)
>                                 return -EINVAL;
You don't add the check for KSMBD_EVENT_SPNEGO_AUTHEN_REQUEST case.
We don't need to check resp->session_key_len and resp->spnego_blob_len?

Re: [PATCH 1/3] ksmbd: cap response sizes in ipc_validate_msg()

[Michael Bommarito]

On Wed, Apr 15, 2026 at 11:00:58AM +0900, Namjae Jeon wrote:
> However, on the userspace side (ksmbd-tools/mountd/rpc.c), the DCE/RPC
> response builder (try_realloc_payload() and ndr_write_bytes())
> dynamically grows the payload by 4096 bytes using g_try_realloc() when
> preparing responses for calls such as NetShareEnumAll, etc..
> This can cause share enumeration failures on servers with many shares.

OK, thanks for explaining.  Sorry for missing that context.  If you
are OK with it, I will send a v2 that drops the cap on RPC_REQUEST
and SHARE_CONFIG_REQUEST and uses check_add_overflow() to just
prevent msg_sz from wrapping.  The [0, NGROUPS_MAX] bound stays on
LOGIN_REQUEST_EXT.

> You don't add the check for KSMBD_EVENT_SPNEGO_AUTHEN_REQUEST case.
> We don't need to check resp->session_key_len and resp->spnego_blob_len?

They're both __u16 so the sum can't wrap the unsigned int msg_sz,
which is why I skipped them.  Happy to add check_add_overflow() there
too for symmetry and clarity in case anyone refactors.  Just let me
know which you prefer.

Thanks,
Mike

Re: [PATCH 1/3] ksmbd: cap response sizes in ipc_validate_msg()

[Namjae Jeon]

On Wed, Apr 15, 2026 at 11:35 AM Michael Bommarito
<michael.bommarito@gmail.com> wrote:
>
> On Wed, Apr 15, 2026 at 11:00:58AM +0900, Namjae Jeon wrote:
> > However, on the userspace side (ksmbd-tools/mountd/rpc.c), the DCE/RPC
> > response builder (try_realloc_payload() and ndr_write_bytes())
> > dynamically grows the payload by 4096 bytes using g_try_realloc() when
> > preparing responses for calls such as NetShareEnumAll, etc..
> > This can cause share enumeration failures on servers with many shares.
>
> OK, thanks for explaining.  Sorry for missing that context.  If you
> are OK with it, I will send a v2 that drops the cap on RPC_REQUEST
> and SHARE_CONFIG_REQUEST and uses check_add_overflow() to just
> prevent msg_sz from wrapping.  The [0, NGROUPS_MAX] bound stays on
> LOGIN_REQUEST_EXT.
OK, sounds good.
>
> > You don't add the check for KSMBD_EVENT_SPNEGO_AUTHEN_REQUEST case.
> > We don't need to check resp->session_key_len and resp->spnego_blob_len?
>
> They're both __u16 so the sum can't wrap the unsigned int msg_sz,
> which is why I skipped them.  Happy to add check_add_overflow() there
> too for symmetry and clarity in case anyone refactors.  Just let me
> know which you prefer.
Ah, Okay, no need to add the check for this if so.
Thanks.
>
> Thanks,
> Mike

[PATCH 2/3] ksmbd: reject negative ngroups in ksmbd_alloc_user()

[Michael Bommarito]

resp_ext->ngroups is __s32.  ksmbd_alloc_user() guards against
oversized group counts with

	if (resp_ext->ngroups > NGROUPS_MAX)
		goto err_free;

but the signed comparison does not catch negative values.  A
negative ngroups passes through into the subsequent multiplication

	resp_ext->ngroups * sizeof(gid_t)

where signed-to-size_t conversion turns e.g. -1 into SIZE_MAX, and
kmemdup() is handed an absurd size.  In practice kmemdup() fails
gracefully on the huge allocation, but the intent of the guard is
to reject out-of-range values up front, not rely on the allocator
to notice.

Reject negative ngroups explicitly so the check reflects the actual
valid range, and switch the log format for ngroups from %u to %d so
the bad signed value is printed correctly.

Fixes: a77e0e02af1c ("ksmbd: add support for supplementary groups")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Assisted-by: Codex:gpt-5-4
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---

fs/smb/server/mgmt/user_config.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/smb/server/mgmt/user_config.c b/fs/smb/server/mgmt/user_config.c
index a3183fe5c536..c62e2bf0ebef 100644
--- a/fs/smb/server/mgmt/user_config.c
+++ b/fs/smb/server/mgmt/user_config.c
@@ -56,8 +56,8 @@ struct ksmbd_user *ksmbd_alloc_user(struct ksmbd_login_response *resp,
 		goto err_free;
 
 	if (resp_ext) {
-		if (resp_ext->ngroups > NGROUPS_MAX) {
-			pr_err("ngroups(%u) from login response exceeds max groups(%d)\n",
+		if (resp_ext->ngroups < 0 || resp_ext->ngroups > NGROUPS_MAX) {
+			pr_err("ngroups(%d) from login response exceeds max groups(%d)\n",
 					resp_ext->ngroups, NGROUPS_MAX);
 			goto err_free;
 		}
--
2.53.0

Re: [PATCH 2/3] ksmbd: reject negative ngroups in ksmbd_alloc_user()

[Namjae Jeon]

> diff --git a/fs/smb/server/mgmt/user_config.c b/fs/smb/server/mgmt/user_config.c
> index a3183fe5c536..c62e2bf0ebef 100644
> --- a/fs/smb/server/mgmt/user_config.c
> +++ b/fs/smb/server/mgmt/user_config.c
> @@ -56,8 +56,8 @@ struct ksmbd_user *ksmbd_alloc_user(struct ksmbd_login_response *resp,
>                 goto err_free;
>
>         if (resp_ext) {
> -               if (resp_ext->ngroups > NGROUPS_MAX) {
> -                       pr_err("ngroups(%u) from login response exceeds max groups(%d)\n",
> +               if (resp_ext->ngroups < 0 || resp_ext->ngroups > NGROUPS_MAX) {
> +                       pr_err("ngroups(%d) from login response exceeds max groups(%d)\n",
With the previous patch ("ksmbd: cap response sizes in
ipc_validate_msg()"), negative ngroups is now rejected early in IPC
validation.
However, ksmbd_alloc_user() still needs an explicit negative check ?

>                                         resp_ext->ngroups, NGROUPS_MAX);
>                         goto err_free;
>                 }
> --
> 2.53.0

Re: [PATCH 2/3] ksmbd: reject negative ngroups in ksmbd_alloc_user()

[Michael Bommarito]

On Wed, Apr 15, 2026 at 11:05:45AM +0900, Namjae Jeon wrote:
> With the previous patch ("ksmbd: cap response sizes in
> ipc_validate_msg()"), negative ngroups is now rejected early in IPC
> validation.
> However, ksmbd_alloc_user() still needs an explicit negative check ?

Yup, good point.  I originally wrote the tests and fixes independently
and missed the overlap, so if you accept the cap in patch 1, then we
can skip it.

Two Qs:

1. Should I add a comment in case someone refactors the flow to
emphasize that a check would be needed here if not covered earlier?

2. Do you want me to fold this into 1/3 above?

Thanks,
Mike

Re: [PATCH 2/3] ksmbd: reject negative ngroups in ksmbd_alloc_user()

[Namjae Jeon]

On Wed, Apr 15, 2026 at 11:35 AM Michael Bommarito
<michael.bommarito@gmail.com> wrote:
>
> On Wed, Apr 15, 2026 at 11:05:45AM +0900, Namjae Jeon wrote:
> > With the previous patch ("ksmbd: cap response sizes in
> > ipc_validate_msg()"), negative ngroups is now rejected early in IPC
> > validation.
> > However, ksmbd_alloc_user() still needs an explicit negative check ?
>
> Yup, good point.  I originally wrote the tests and fixes independently
> and missed the overlap, so if you accept the cap in patch 1, then we
> can skip it.
>
> Two Qs:
>
> 1. Should I add a comment in case someone refactors the flow to
> emphasize that a check would be needed here if not covered earlier?
Yes, since ipc_validate_msg() now checks ngroups early, the same check
in ksmbd_alloc_user() has become redundant. So please remove the
ngroup check from ksmbd_alloc_user(). And we can also move pr_err
message to ipc_validate_msg() so that the error is reported earlier.
>
> 2. Do you want me to fold this into 1/3 above?
Please fold this change into patch 1/3.

Thanks.
>
> Thanks,
> Mike

[PATCH 3/3] ksmbd: require minimum ACE size in smb_check_perm_dacl()

[Michael Bommarito]

Both ACE-walk loops in smb_check_perm_dacl() only guard against an
under-sized remaining buffer, not against an ACE whose declared
`ace->size` is smaller than the struct it claims to describe:

  if (offsetof(struct smb_ace, access_req) > aces_size)
      break;
  ace_size = le16_to_cpu(ace->size);
  if (ace_size > aces_size)
      break;

The first check only requires the 4-byte ACE header to be in bounds;
it does not require access_req (4 bytes at offset 4) to be readable.
An attacker who has set a crafted DACL on a file they own can declare
ace->size == 4 with aces_size == 4, pass both checks, and then

  granted |= le32_to_cpu(ace->access_req);               /* upper loop */
  compare_sids(&sid, &ace->sid);                         /* lower loop */

reads access_req at offset 4 (OOB by up to 4 bytes) and ace->sid at
offset 8 (OOB by up to CIFS_SID_BASE_SIZE + SID_MAX_SUB_AUTHORITIES
* 4 bytes).

Tighten both loops to require

  ace_size >= offsetof(struct smb_ace, sid) + CIFS_SID_BASE_SIZE

which is the smallest valid on-wire ACE layout (4-byte header +
4-byte access_req + 8-byte sid base with zero sub-auths).  Also
reject ACEs whose sid.num_subauth exceeds SID_MAX_SUB_AUTHORITIES
before letting compare_sids() dereference sub_auth[] entries.

parse_sec_desc() already enforces an equivalent check (lines 441-448);
smb_check_perm_dacl() simply grew weaker validation over time.

Reachability: authenticated SMB client with permission to set an ACL
on a file.  On a subsequent CREATE against that file, the kernel
walks the stored DACL via smb_check_perm_dacl() and triggers the
OOB read.  Not pre-auth, and the OOB read is not reflected to the
attacker, but KASAN reports and kernel state corruption are
possible.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Assisted-by: Codex:gpt-5-4
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---

fs/smb/server/smbacl.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index c30d01877c41..d5943256c071 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1341,10 +1341,13 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 		ace = (struct smb_ace *)((char *)pdacl + sizeof(struct smb_acl));
 		aces_size = acl_size - sizeof(struct smb_acl);
 		for (i = 0; i < le16_to_cpu(pdacl->num_aces); i++) {
-			if (offsetof(struct smb_ace, access_req) > aces_size)
+			if (offsetof(struct smb_ace, sid) +
+			    CIFS_SID_BASE_SIZE > aces_size)
 				break;
 			ace_size = le16_to_cpu(ace->size);
-			if (ace_size > aces_size)
+			if (ace_size > aces_size ||
+			    ace_size < offsetof(struct smb_ace, sid) +
+				       CIFS_SID_BASE_SIZE)
 				break;
 			aces_size -= ace_size;
 			granted |= le32_to_cpu(ace->access_req);
@@ -1359,13 +1362,19 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 	ace = (struct smb_ace *)((char *)pdacl + sizeof(struct smb_acl));
 	aces_size = acl_size - sizeof(struct smb_acl);
 	for (i = 0; i < le16_to_cpu(pdacl->num_aces); i++) {
-		if (offsetof(struct smb_ace, access_req) > aces_size)
+		if (offsetof(struct smb_ace, sid) +
+		    CIFS_SID_BASE_SIZE > aces_size)
 			break;
 		ace_size = le16_to_cpu(ace->size);
-		if (ace_size > aces_size)
+		if (ace_size > aces_size ||
+		    ace_size < offsetof(struct smb_ace, sid) +
+			       CIFS_SID_BASE_SIZE)
 			break;
 		aces_size -= ace_size;
 
+		if (ace->sid.num_subauth > SID_MAX_SUB_AUTHORITIES)
+			break;
+
 		if (!compare_sids(&sid, &ace->sid) ||
 		    !compare_sids(&sid_unix_NFS_mode, &ace->sid)) {
 			found = 1;
--
2.53.0

[PATCH v2 0/2] ksmbd: harden ipc_validate_msg() and smb_check_perm_dacl()

[Michael Bommarito]

Two ksmbd hardening patches, respun from v1 [PATCH 0/3] per Namjae's
review.

Patch 1 folds v1 1/3 and 2/3 into a single response-side validation
change in ipc_validate_msg().

Patch 2 is v1 3/3 unchanged (minimum ACE size in
smb_check_perm_dacl()).  Please let me know if there's anything
on this 2/2 you want to think through or change.

Changes since v1
----------------

v1 -> v2:

  - 1/3 + 2/3 folded into a single patch (1/2) per Namjae.
  - Dropped the hard KSMBD_IPC_MAX_PAYLOAD (4096) cap on
    RPC_REQUEST and SHARE_CONFIG_REQUEST response paths.  A 4096
    cap would regress NetShareEnumAll and other NDR enumerations
    on servers with many shares -- userspace ksmbd-tools grows
    the response buffer in 4096-byte chunks via g_try_realloc().
    Use check_add_overflow() instead so functional payload size
    is unconstrained but msg_sz cannot wrap unsigned int.
    [Namjae]
  - LOGIN_REQUEST_EXT keeps the [0, NGROUPS_MAX] bound (POSIX
    semantic limit, not an IPC transport cap).  Moved the
    pr_err() into ipc_validate_msg() so the error is reported
    at the IPC boundary. [Namjae]
  - Removed the now-redundant ngroups check and pr_err() from
    ksmbd_alloc_user() in mgmt/user_config.c.  Both call sites
    (ksmbd_login_user and the SPNEGO path in auth.c) reach
    ksmbd_alloc_user() through ksmbd_ipc_login_request_ext(),
    which now rejects negative ngroups at the IPC gate. [Namjae]
  - SPNEGO_AUTHEN_REQUEST left untouched: session_key_len and
    spnego_blob_len are both __u16 so their sum cannot wrap the
    unsigned int msg_sz. [Namjae ack]
  - 2/2 (smb_check_perm_dacl minimum ACE size) unchanged from
    v1 3/3 -- no review yet.

Threading
---------

Sent --in-reply-to v1 [PATCH 0/3] cover
(Message-ID 20260414191533.1467353-1-michael.bommarito@gmail.com)
so v2 lives under the v1 thread.

Michael Bommarito (2):
  ksmbd: validate response sizes in ipc_validate_msg()
  ksmbd: require minimum ACE size in smb_check_perm_dacl()

 fs/smb/server/mgmt/user_config.c |  6 ------
 fs/smb/server/smbacl.c           | 17 +++++++++++++----
 fs/smb/server/transport_ipc.c    | 16 +++++++++++++---
 3 files changed, 26 insertions(+), 13 deletions(-)

--
2.53.0

[PATCH v2 1/2] ksmbd: validate response sizes in ipc_validate_msg()

[Michael Bommarito]

ipc_validate_msg() computes the expected message size for each
response type by adding (or multiplying) attacker-controlled fields
from the daemon response to a fixed struct size in unsigned int
arithmetic.  Three cases can overflow:

  KSMBD_EVENT_RPC_REQUEST:
      msg_sz = sizeof(struct ksmbd_rpc_command) + resp->payload_sz;
  KSMBD_EVENT_SHARE_CONFIG_REQUEST:
      msg_sz = sizeof(struct ksmbd_share_config_response) +
               resp->payload_sz;
  KSMBD_EVENT_LOGIN_REQUEST_EXT:
      msg_sz = sizeof(struct ksmbd_login_response_ext) +
               resp->ngroups * sizeof(gid_t);

resp->payload_sz is __u32 and resp->ngroups is __s32.  Each addition
can wrap in unsigned int; the multiplication by sizeof(gid_t) mixes
signed and size_t, so a negative ngroups is converted to SIZE_MAX
before the multiply.  A wrapped value of msg_sz that happens to
equal entry->msg_sz bypasses the size check on the next line, and
downstream consumers (smb2pdu.c:6742 memcpy using rpc_resp->payload_sz,
kmemdup in ksmbd_alloc_user using resp_ext->ngroups) then trust the
unverified length.

Use check_add_overflow() on the RPC_REQUEST and SHARE_CONFIG_REQUEST
paths to detect integer overflow without constraining functional
payload size; userspace ksmbd-tools grows NDR responses in 4096-byte
chunks for calls like NetShareEnumAll, so a hard transport cap is
unworkable on the response side.  For LOGIN_REQUEST_EXT, reject
resp->ngroups outside the signed [0, NGROUPS_MAX] range up front and
report the error from ipc_validate_msg() so it fires at the IPC
boundary; with that bound the subsequent multiplication and addition
stay well below UINT_MAX.  The now-redundant ngroups check and
pr_err in ksmbd_alloc_user() are removed.

This is the response-side analogue of aab98e2dbd64 ("ksmbd: fix
integer overflows on 32 bit systems"), which hardened the request
side.

Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers")
Fixes: a77e0e02af1c ("ksmbd: add support for supplementary groups")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Assisted-by: Codex:gpt-5-4
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 fs/smb/server/mgmt/user_config.c |  6 ------
 fs/smb/server/transport_ipc.c    | 16 +++++++++++++---
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/smb/server/mgmt/user_config.c b/fs/smb/server/mgmt/user_config.c
index a3183fe5c536..cf45841d9d1b 100644
--- a/fs/smb/server/mgmt/user_config.c
+++ b/fs/smb/server/mgmt/user_config.c
@@ -56,12 +56,6 @@ struct ksmbd_user *ksmbd_alloc_user(struct ksmbd_login_response *resp,
 		goto err_free;
 
 	if (resp_ext) {
-		if (resp_ext->ngroups > NGROUPS_MAX) {
-			pr_err("ngroups(%u) from login response exceeds max groups(%d)\n",
-					resp_ext->ngroups, NGROUPS_MAX);
-			goto err_free;
-		}
-
 		user->sgid = kmemdup(resp_ext->____payload,
 				     resp_ext->ngroups * sizeof(gid_t),
 				     KSMBD_DEFAULT_GFP);
diff --git a/fs/smb/server/transport_ipc.c b/fs/smb/server/transport_ipc.c
index 2dbabe2d8005..1c5645238bd3 100644
--- a/fs/smb/server/transport_ipc.c
+++ b/fs/smb/server/transport_ipc.c
@@ -13,6 +13,7 @@
 #include <net/genetlink.h>
 #include <linux/socket.h>
 #include <linux/workqueue.h>
+#include <linux/overflow.h>
 
 #include "vfs_cache.h"
 #include "transport_ipc.h"
@@ -497,7 +498,9 @@ static int ipc_validate_msg(struct ipc_msg_table_entry *entry)
 	{
 		struct ksmbd_rpc_command *resp = entry->response;
 
-		msg_sz = sizeof(struct ksmbd_rpc_command) + resp->payload_sz;
+		if (check_add_overflow(sizeof(struct ksmbd_rpc_command),
+				       resp->payload_sz, &msg_sz))
+			return -EINVAL;
 		break;
 	}
 	case KSMBD_EVENT_SPNEGO_AUTHEN_REQUEST:
@@ -516,8 +519,9 @@ static int ipc_validate_msg(struct ipc_msg_table_entry *entry)
 			if (resp->payload_sz < resp->veto_list_sz)
 				return -EINVAL;
 
-			msg_sz = sizeof(struct ksmbd_share_config_response) +
-					resp->payload_sz;
+			if (check_add_overflow(sizeof(struct ksmbd_share_config_response),
+					       resp->payload_sz, &msg_sz))
+				return -EINVAL;
 		}
 		break;
 	}
@@ -526,6 +530,12 @@ static int ipc_validate_msg(struct ipc_msg_table_entry *entry)
 		struct ksmbd_login_response_ext *resp = entry->response;
 
 		if (resp->ngroups) {
+			if (resp->ngroups < 0 ||
+			    resp->ngroups > NGROUPS_MAX) {
+				pr_err("ngroups(%d) from login response exceeds max groups(%d)\n",
+				       resp->ngroups, NGROUPS_MAX);
+				return -EINVAL;
+			}
 			msg_sz = sizeof(struct ksmbd_login_response_ext) +
 					resp->ngroups * sizeof(gid_t);
 		}
-- 
2.53.0

[PATCH v2 2/2] ksmbd: require minimum ACE size in smb_check_perm_dacl()

[Michael Bommarito]

Both ACE-walk loops in smb_check_perm_dacl() only guard against an
under-sized remaining buffer, not against an ACE whose declared
`ace->size` is smaller than the struct it claims to describe:

  if (offsetof(struct smb_ace, access_req) > aces_size)
      break;
  ace_size = le16_to_cpu(ace->size);
  if (ace_size > aces_size)
      break;

The first check only requires the 4-byte ACE header to be in bounds;
it does not require access_req (4 bytes at offset 4) to be readable.
An attacker who has set a crafted DACL on a file they own can declare
ace->size == 4 with aces_size == 4, pass both checks, and then

  granted |= le32_to_cpu(ace->access_req);               /* upper loop */
  compare_sids(&sid, &ace->sid);                         /* lower loop */

reads access_req at offset 4 (OOB by up to 4 bytes) and ace->sid at
offset 8 (OOB by up to CIFS_SID_BASE_SIZE + SID_MAX_SUB_AUTHORITIES
* 4 bytes).

Tighten both loops to require

  ace_size >= offsetof(struct smb_ace, sid) + CIFS_SID_BASE_SIZE

which is the smallest valid on-wire ACE layout (4-byte header +
4-byte access_req + 8-byte sid base with zero sub-auths).  Also
reject ACEs whose sid.num_subauth exceeds SID_MAX_SUB_AUTHORITIES
before letting compare_sids() dereference sub_auth[] entries.

parse_sec_desc() already enforces an equivalent check (lines 441-448);
smb_check_perm_dacl() simply grew weaker validation over time.

Reachability: authenticated SMB client with permission to set an ACL
on a file.  On a subsequent CREATE against that file, the kernel
walks the stored DACL via smb_check_perm_dacl() and triggers the
OOB read.  Not pre-auth, and the OOB read is not reflected to the
attacker, but KASAN reports and kernel state corruption are
possible.

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

diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index c30d01877c41..d5943256c071 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1341,10 +1341,13 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 		ace = (struct smb_ace *)((char *)pdacl + sizeof(struct smb_acl));
 		aces_size = acl_size - sizeof(struct smb_acl);
 		for (i = 0; i < le16_to_cpu(pdacl->num_aces); i++) {
-			if (offsetof(struct smb_ace, access_req) > aces_size)
+			if (offsetof(struct smb_ace, sid) +
+			    CIFS_SID_BASE_SIZE > aces_size)
 				break;
 			ace_size = le16_to_cpu(ace->size);
-			if (ace_size > aces_size)
+			if (ace_size > aces_size ||
+			    ace_size < offsetof(struct smb_ace, sid) +
+				       CIFS_SID_BASE_SIZE)
 				break;
 			aces_size -= ace_size;
 			granted |= le32_to_cpu(ace->access_req);
@@ -1359,13 +1362,19 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 	ace = (struct smb_ace *)((char *)pdacl + sizeof(struct smb_acl));
 	aces_size = acl_size - sizeof(struct smb_acl);
 	for (i = 0; i < le16_to_cpu(pdacl->num_aces); i++) {
-		if (offsetof(struct smb_ace, access_req) > aces_size)
+		if (offsetof(struct smb_ace, sid) +
+		    CIFS_SID_BASE_SIZE > aces_size)
 			break;
 		ace_size = le16_to_cpu(ace->size);
-		if (ace_size > aces_size)
+		if (ace_size > aces_size ||
+		    ace_size < offsetof(struct smb_ace, sid) +
+			       CIFS_SID_BASE_SIZE)
 			break;
 		aces_size -= ace_size;
 
+		if (ace->sid.num_subauth > SID_MAX_SUB_AUTHORITIES)
+			break;
+
 		if (!compare_sids(&sid, &ace->sid) ||
 		    !compare_sids(&sid_unix_NFS_mode, &ace->sid)) {
 			found = 1;
-- 
2.53.0

Re: [PATCH v2 0/2] ksmbd: harden ipc_validate_msg() and smb_check_perm_dacl()

[Namjae Jeon]

On Wed, Apr 15, 2026 at 8:25 PM Michael Bommarito
<michael.bommarito@gmail.com> wrote:
>
> Two ksmbd hardening patches, respun from v1 [PATCH 0/3] per Namjae's
> review.
>
> Patch 1 folds v1 1/3 and 2/3 into a single response-side validation
> change in ipc_validate_msg().
>
> Patch 2 is v1 3/3 unchanged (minimum ACE size in
> smb_check_perm_dacl()).  Please let me know if there's anything
> on this 2/2 you want to think through or change.
>
> Changes since v1
> ----------------
>
> v1 -> v2:
>
>   - 1/3 + 2/3 folded into a single patch (1/2) per Namjae.
>   - Dropped the hard KSMBD_IPC_MAX_PAYLOAD (4096) cap on
>     RPC_REQUEST and SHARE_CONFIG_REQUEST response paths.  A 4096
>     cap would regress NetShareEnumAll and other NDR enumerations
>     on servers with many shares -- userspace ksmbd-tools grows
>     the response buffer in 4096-byte chunks via g_try_realloc().
>     Use check_add_overflow() instead so functional payload size
>     is unconstrained but msg_sz cannot wrap unsigned int.
>     [Namjae]
>   - LOGIN_REQUEST_EXT keeps the [0, NGROUPS_MAX] bound (POSIX
>     semantic limit, not an IPC transport cap).  Moved the
>     pr_err() into ipc_validate_msg() so the error is reported
>     at the IPC boundary. [Namjae]
>   - Removed the now-redundant ngroups check and pr_err() from
>     ksmbd_alloc_user() in mgmt/user_config.c.  Both call sites
>     (ksmbd_login_user and the SPNEGO path in auth.c) reach
>     ksmbd_alloc_user() through ksmbd_ipc_login_request_ext(),
>     which now rejects negative ngroups at the IPC gate. [Namjae]
>   - SPNEGO_AUTHEN_REQUEST left untouched: session_key_len and
>     spnego_blob_len are both __u16 so their sum cannot wrap the
>     unsigned int msg_sz. [Namjae ack]
>   - 2/2 (smb_check_perm_dacl minimum ACE size) unchanged from
>     v1 3/3 -- no review yet.
>
> Threading
> ---------
>
> Sent --in-reply-to v1 [PATCH 0/3] cover
> (Message-ID 20260414191533.1467353-1-michael.bommarito@gmail.com)
> so v2 lives under the v1 thread.
>
> Michael Bommarito (2):
>   ksmbd: validate response sizes in ipc_validate_msg()
>   ksmbd: require minimum ACE size in smb_check_perm_dacl()
Applied them to #ksmbd-for-next-next.
Thanks!