[PATCH] smb: client: fix OOB reads from server-supplied dacloffset in cifsacl

[PATCH] smb: client: fix OOB reads from server-supplied dacloffset in cifsacl

[Michael Bommarito]

Hi again! Sorry for the piecemeal sends as I work through triaging
all of my personal "clanker" pipeline results.

Here is another malicious server issue, this time in two call sites
in fs/smb/client/cifsacl.c.  We compute a DACL pointer from a
server-supplied dacloffset and then read struct smb_acl fields 
(dacl_ptr->size, dacl_ptr->num_aces) without first checking that the
8-byte struct header fits within the security descriptor buffer.

1. build_sec_desc() -- chmod path:

    dacl_ptr = (struct smb_acl *)((char *)pntsd + dacloffset);
    if (end_of_acl < (char *)dacl_ptr + le16_to_cpu(dacl_ptr->size))

2. id_mode_to_cifs_acl() -- chown path:

    dacl_ptr = (struct smb_acl *)((char *)pntsd + dacloffset);
    ...
    le16_to_cpu(dacl_ptr->num_aces) * sizeof(struct smb_ace);
    le16_to_cpu(dacl_ptr->size);

In both cases, if dacloffset places dacl_ptr at or past end_of_acl,
the le16_to_cpu() dereferences are OOB heap reads.

Reproduced under UML + KASAN by constructing a 20-byte smb_ntsd
(sizeof(struct smb_ntsd)) with dacloffset = 20, placing dacl_ptr at
the exact end of the allocation.  The le16_to_cpu(dacl_ptr->size)
read triggers:

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

Confirmed -EINVAL without splat after patch applied.

parse_dacl() already handles this correctly with a two-step check that
validates the struct header fits before reading the size field:

    if (end_of_acl < (char *)pdacl + sizeof(struct smb_acl) ||
        end_of_acl < (char *)pdacl + le16_to_cpu(pdacl->size))

Apply the same pattern to both sites.  This is the client-side
counterpart of commit beff0bc9d69b ("ksmbd: fix overflow in dacloffset
bounds check") which fixed the identical class of issue on the server
side.

Fixes: bc3e9dd9d104 ("cifs: Change SIDs in ACEs while transferring file ownership.")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 fs/smb/client/cifsacl.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index c920039d733c..115990467dfa 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -1293,7 +1293,8 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
 	dacloffset = le32_to_cpu(pntsd->dacloffset);
 	if (dacloffset) {
 		dacl_ptr = (struct smb_acl *)((char *)pntsd + dacloffset);
-		if (end_of_acl < (char *)dacl_ptr + le16_to_cpu(dacl_ptr->size)) {
+		if (end_of_acl < (char *)dacl_ptr + sizeof(struct smb_acl) ||
+		    end_of_acl < (char *)dacl_ptr + le16_to_cpu(dacl_ptr->size)) {
 			cifs_dbg(VFS, "Server returned illegal ACL size\n");
 			return -EINVAL;
 		}
@@ -1662,6 +1663,14 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
 		dacloffset = le32_to_cpu(pntsd->dacloffset);
 		if (dacloffset) {
 			dacl_ptr = (struct smb_acl *)((char *)pntsd + dacloffset);
+			if ((char *)pntsd + secdesclen <
+			    (char *)dacl_ptr + sizeof(struct smb_acl)) {
+				cifs_dbg(VFS, "Server returned illegal dacloffset\n");
+				rc = -EINVAL;
+				kfree(pntsd);
+				cifs_put_tlink(tlink);
+				return rc;
+			}
 			if (mode_from_sid)
 				nsecdesclen +=
 					le16_to_cpu(dacl_ptr->num_aces) * sizeof(struct smb_ace);
-- 
2.53.0