Skip to content

Commit 3e4e2ea

Browse files
mjbommarsmfrench
authored andcommitted
ksmbd: validate num_aces and harden ACE walk in smb_inherit_dacl()
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: e2f3448 ("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> Acked-by: Namjae Jeon <linkinjeon@kernel.org> Signed-off-by: Steve French <stfrench@microsoft.com>
1 parent ce23158 commit 3e4e2ea

1 file changed

Lines changed: 23 additions & 5 deletions

File tree

fs/smb/server/smbacl.c

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1106,8 +1106,24 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
11061106
goto free_parent_pntsd;
11071107
}
11081108

1109-
aces_base = kmalloc(sizeof(struct smb_ace) * num_aces * 2,
1110-
KSMBD_DEFAULT_GFP);
1109+
aces_size = pdacl_size - sizeof(struct smb_acl);
1110+
1111+
/*
1112+
* Validate num_aces against the DACL payload before allocating.
1113+
* Each ACE must be at least as large as its fixed-size header
1114+
* (up to the SID base), so num_aces cannot exceed the payload
1115+
* divided by the minimum ACE size. This mirrors the existing
1116+
* check in parse_dacl().
1117+
*/
1118+
if (num_aces > aces_size / (offsetof(struct smb_ace, sid) +
1119+
offsetof(struct smb_sid, sub_auth) +
1120+
sizeof(__le16))) {
1121+
rc = -EINVAL;
1122+
goto free_parent_pntsd;
1123+
}
1124+
1125+
aces_base = kmalloc_array(num_aces * 2, sizeof(struct smb_ace),
1126+
KSMBD_DEFAULT_GFP);
11111127
if (!aces_base) {
11121128
rc = -ENOMEM;
11131129
goto free_parent_pntsd;
@@ -1116,19 +1132,21 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
11161132
aces = (struct smb_ace *)aces_base;
11171133
parent_aces = (struct smb_ace *)((char *)parent_pdacl +
11181134
sizeof(struct smb_acl));
1119-
aces_size = acl_len - sizeof(struct smb_acl);
11201135

11211136
if (pntsd_type & DACL_AUTO_INHERITED)
11221137
inherited_flags = INHERITED_ACE;
11231138

11241139
for (i = 0; i < num_aces; i++) {
11251140
int pace_size;
11261141

1127-
if (offsetof(struct smb_ace, access_req) > aces_size)
1142+
if (aces_size < offsetof(struct smb_ace, sid) +
1143+
CIFS_SID_BASE_SIZE)
11281144
break;
11291145

11301146
pace_size = le16_to_cpu(parent_aces->size);
1131-
if (pace_size > aces_size)
1147+
if (pace_size > aces_size ||
1148+
pace_size < offsetof(struct smb_ace, sid) +
1149+
CIFS_SID_BASE_SIZE)
11321150
break;
11331151

11341152
aces_size -= pace_size;

0 commit comments

Comments
 (0)