[PATCH] smb: client: fix heap overflow in cifs_do_set_acl() from mismatched ACL format sizes

[PATCH] smb: client: fix heap overflow in cifs_do_set_acl() from mismatched ACL format sizes

[Frank Sorenson]

cifs_set_acl() validates ACL size using posix_acl_xattr_size(), which
computes the Linux xattr format size:

        4 + (count * 8)  // 4-byte header + 8 bytes per ACE

But cifs_do_set_acl() calls posix_acl_to_cifs() to write the CIFS
wire format into the same buffer:

        6 + (count * 10)  // 6-byte header + 10 bytes per ACE

For the same entry count, the CIFS format is ~25% larger.  An ACL
with a near-maximum count that passes the xattr-based size check in
cifs_set_acl() can overflow the heap allocation when written by
posix_acl_to_cifs().

Before converting, validate that the CIFS format size fits in the
remaining buffer space.  Replace the bogus hardcoded MaxDataCount
value (1000) with CIFSMaxBufSize.

Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 fs/smb/client/cifssmb.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index f5aad5f61dce..4ce22fe3cb2e 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -3555,6 +3555,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	int rc = 0;
 	int bytes_returned = 0;
 	__u16 params, byte_count, data_count, param_offset, offset;
+	size_t cifs_acl_size, bytes_available;
 
 	cifs_dbg(FYI, "In SetPosixACL (Unix) for path %s\n", fileName);
 setAclRetry:
@@ -3575,7 +3576,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	params = 6 + name_len;
 	pSMB->MaxParameterCount = cpu_to_le16(2);
 	/* BB find max SMB size from sess */
-	pSMB->MaxDataCount = cpu_to_le16(1000);
+	pSMB->MaxDataCount = cpu_to_le16(CIFSMaxBufSize);
 	pSMB->MaxSetupCount = 0;
 	pSMB->Reserved = 0;
 	pSMB->Flags = 0;
@@ -3587,6 +3588,15 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	parm_data = ((char *)pSMB) + offset;
 	pSMB->ParameterOffset = cpu_to_le16(param_offset);
 
+	/* make sure we can fit the larger cifs_posix_aces in the buffer */
+	cifs_acl_size = sizeof(struct cifs_posix_acl) +
+		       (acl->a_count * sizeof(struct cifs_posix_ace));
+	bytes_available = (CIFSMaxBufSize + MAX_SMB2_HDR_SIZE) - offset;
+	if (cifs_acl_size > bytes_available) {
+		rc = -E2BIG;
+		goto setACLerrorExit;
+	}
+
 	/* convert to on the wire format for POSIX ACL */
 	data_count = posix_acl_to_cifs(parm_data, acl, acl_type);
 
-- 
2.55.0

[PATCH v2] smb: client: fix heap overflow in cifs_do_set_acl()

[Frank Sorenson]

cifs_set_acl() validates ACL size using posix_acl_xattr_size():

        4 + (count * 8)  // 4-byte header + 8 bytes per ACE

cifs_do_set_acl() then calls posix_acl_to_cifs() to write the CIFS
wire format into the same buffer:

        6 + (count * 10)  // 6-byte header + 10 bytes per ACE

An ACL that passes the xattr-based check in cifs_set_acl() can
overflow the heap when posix_acl_to_cifs() writes the larger CIFS
format.

Validate the CIFS format size against the remaining buffer space and
USHRT_MAX before converting--data_count is __u16, so sizes above
USHRT_MAX truncate the on-wire packet length, causing the server to
apply a partial ACL.  Replace MaxDataCount = 1000 with
min(CIFSMaxBufSize, USHRT_MAX).

Fixes: dc1af4c4b4721 ("cifs: implement set acl method")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
v2 changes:
 - Add USHRT_MAX bound to prevent u16 truncation of data_count for ACLs
   with more than 6553 entries, which would cause a partial ACL to be
   silently applied on the server
 - limit MaxDataCount to min(CIFSMaxBufSize, USHRT_MAX)

 fs/smb/client/cifssmb.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index f5aad5f61dce..621aca5d3b75 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -3555,6 +3555,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	int rc = 0;
 	int bytes_returned = 0;
 	__u16 params, byte_count, data_count, param_offset, offset;
+	size_t cifs_acl_size, bytes_available;
 
 	cifs_dbg(FYI, "In SetPosixACL (Unix) for path %s\n", fileName);
 setAclRetry:
@@ -3574,8 +3575,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	}
 	params = 6 + name_len;
 	pSMB->MaxParameterCount = cpu_to_le16(2);
-	/* BB find max SMB size from sess */
-	pSMB->MaxDataCount = cpu_to_le16(1000);
+	pSMB->MaxDataCount = cpu_to_le16(min_t(unsigned int, CIFSMaxBufSize, USHRT_MAX));
 	pSMB->MaxSetupCount = 0;
 	pSMB->Reserved = 0;
 	pSMB->Flags = 0;
@@ -3587,6 +3587,15 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	parm_data = ((char *)pSMB) + offset;
 	pSMB->ParameterOffset = cpu_to_le16(param_offset);
 
+	/* make sure we can fit the larger cifs_posix_aces in the buffer */
+	cifs_acl_size = sizeof(struct cifs_posix_acl) +
+		       (acl->a_count * sizeof(struct cifs_posix_ace));
+	bytes_available = (CIFSMaxBufSize + MAX_SMB2_HDR_SIZE) - offset;
+	if (cifs_acl_size > bytes_available || cifs_acl_size > USHRT_MAX) {
+		rc = -E2BIG;
+		goto setACLerrorExit;
+	}
+
 	/* convert to on the wire format for POSIX ACL */
 	data_count = posix_acl_to_cifs(parm_data, acl, acl_type);
 
-- 
2.55.0

Re: [PATCH v2] smb: client: fix heap overflow in cifs_do_set_acl()

[Paulo Alcantara]

Frank Sorenson <sorenson@redhat.com> writes:

> cifs_set_acl() validates ACL size using posix_acl_xattr_size():
>
>         4 + (count * 8)  // 4-byte header + 8 bytes per ACE
>
> cifs_do_set_acl() then calls posix_acl_to_cifs() to write the CIFS
> wire format into the same buffer:
>
>         6 + (count * 10)  // 6-byte header + 10 bytes per ACE
>
> An ACL that passes the xattr-based check in cifs_set_acl() can
> overflow the heap when posix_acl_to_cifs() writes the larger CIFS
> format.
>
> Validate the CIFS format size against the remaining buffer space and
> USHRT_MAX before converting--data_count is __u16, so sizes above
> USHRT_MAX truncate the on-wire packet length, causing the server to
> apply a partial ACL.  Replace MaxDataCount = 1000 with
> min(CIFSMaxBufSize, USHRT_MAX).
>
> Fixes: dc1af4c4b4721 ("cifs: implement set acl method")
> Cc: stable@vger.kernel.org
> Signed-off-by: Frank Sorenson <sorenson@redhat.com>
> ---
> v2 changes:
>  - Add USHRT_MAX bound to prevent u16 truncation of data_count for ACLs
>    with more than 6553 entries, which would cause a partial ACL to be
>    silently applied on the server
>  - limit MaxDataCount to min(CIFSMaxBufSize, USHRT_MAX)
>
>  fs/smb/client/cifssmb.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
> index f5aad5f61dce..621aca5d3b75 100644
> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@ -3555,6 +3555,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>  	int rc = 0;
>  	int bytes_returned = 0;
>  	__u16 params, byte_count, data_count, param_offset, offset;
> +	size_t cifs_acl_size, bytes_available;
>  
>  	cifs_dbg(FYI, "In SetPosixACL (Unix) for path %s\n", fileName);
>  setAclRetry:
> @@ -3574,8 +3575,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>  	}
>  	params = 6 + name_len;
>  	pSMB->MaxParameterCount = cpu_to_le16(2);
> -	/* BB find max SMB size from sess */
> -	pSMB->MaxDataCount = cpu_to_le16(1000);
> +	pSMB->MaxDataCount = cpu_to_le16(min_t(unsigned int, CIFSMaxBufSize, USHRT_MAX));
>  	pSMB->MaxSetupCount = 0;
>  	pSMB->Reserved = 0;
>  	pSMB->Flags = 0;
> @@ -3587,6 +3587,15 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>  	parm_data = ((char *)pSMB) + offset;
>  	pSMB->ParameterOffset = cpu_to_le16(param_offset);
>  
> +	/* make sure we can fit the larger cifs_posix_aces in the buffer */
> +	cifs_acl_size = sizeof(struct cifs_posix_acl) +
> +		       (acl->a_count * sizeof(struct cifs_posix_ace));
> +	bytes_available = (CIFSMaxBufSize + MAX_SMB2_HDR_SIZE) - offset;

Are you sure you want to use MAX_SMB2_HDR_SIZE?  This is SMB1 code, so I
would expect to see MAX_CIFS_HDR_SIZE.  Alternatively, use
MAX_HEADER_SIZE() helper.

Do you have any reproducer?

What server did you test these changes against?

Re: [PATCH v2] smb: client: fix heap overflow in cifs_do_set_acl()

[Frank Sorenson]

[-- Attachment #1: Type: text/plain, Size: 6382 bytes --]


On 8/25/26 7:03 PM, Paulo Alcantara wrote:
> Frank Sorenson <sorenson@redhat.com> writes:
>
>> cifs_set_acl() validates ACL size using posix_acl_xattr_size():
>>
>>          4 + (count * 8)  // 4-byte header + 8 bytes per ACE
>>
>> cifs_do_set_acl() then calls posix_acl_to_cifs() to write the CIFS
>> wire format into the same buffer:
>>
>>          6 + (count * 10)  // 6-byte header + 10 bytes per ACE
>>
>> An ACL that passes the xattr-based check in cifs_set_acl() can
>> overflow the heap when posix_acl_to_cifs() writes the larger CIFS
>> format.
>>
>> Validate the CIFS format size against the remaining buffer space and
>> USHRT_MAX before converting--data_count is __u16, so sizes above
>> USHRT_MAX truncate the on-wire packet length, causing the server to
>> apply a partial ACL.  Replace MaxDataCount = 1000 with
>> min(CIFSMaxBufSize, USHRT_MAX).
>>
>> Fixes: dc1af4c4b4721 ("cifs: implement set acl method")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Frank Sorenson <sorenson@redhat.com>
>> ---
>> v2 changes:
>>   - Add USHRT_MAX bound to prevent u16 truncation of data_count for ACLs
>>     with more than 6553 entries, which would cause a partial ACL to be
>>     silently applied on the server
>>   - limit MaxDataCount to min(CIFSMaxBufSize, USHRT_MAX)
>>
>>   fs/smb/client/cifssmb.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
>> index f5aad5f61dce..621aca5d3b75 100644
>> --- a/fs/smb/client/cifssmb.c
>> +++ b/fs/smb/client/cifssmb.c
>> @@ -3555,6 +3555,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>>   	int rc = 0;
>>   	int bytes_returned = 0;
>>   	__u16 params, byte_count, data_count, param_offset, offset;
>> +	size_t cifs_acl_size, bytes_available;
>>   
>>   	cifs_dbg(FYI, "In SetPosixACL (Unix) for path %s\n", fileName);
>>   setAclRetry:
>> @@ -3574,8 +3575,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>>   	}
>>   	params = 6 + name_len;
>>   	pSMB->MaxParameterCount = cpu_to_le16(2);
>> -	/* BB find max SMB size from sess */
>> -	pSMB->MaxDataCount = cpu_to_le16(1000);
>> +	pSMB->MaxDataCount = cpu_to_le16(min_t(unsigned int, CIFSMaxBufSize, USHRT_MAX));
>>   	pSMB->MaxSetupCount = 0;
>>   	pSMB->Reserved = 0;
>>   	pSMB->Flags = 0;
>> @@ -3587,6 +3587,15 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>>   	parm_data = ((char *)pSMB) + offset;
>>   	pSMB->ParameterOffset = cpu_to_le16(param_offset);
>>   
>> +	/* make sure we can fit the larger cifs_posix_aces in the buffer */
>> +	cifs_acl_size = sizeof(struct cifs_posix_acl) +
>> +		       (acl->a_count * sizeof(struct cifs_posix_ace));
>> +	bytes_available = (CIFSMaxBufSize + MAX_SMB2_HDR_SIZE) - offset;
> Are you sure you want to use MAX_SMB2_HDR_SIZE?  This is SMB1 code, so I
> would expect to see MAX_CIFS_HDR_SIZE.  Alternatively, use
> MAX_HEADER_SIZE() helper.

The only reason I used MAX_SMB2_HDR_SIZE is that it's the actual size 
allocated in cifs_buf_get() from cifs_req_cachep...  but now that I 
think about it, we don't actually want to use the extra 
(MAX_SMB2_HDR_SIZE - MAX_CIFS_HDR_SIZE) bytes, even though we have it 
allocated.  So as you say, MAX_CIFS_HDR_SIZE or 
MAX_HEADER_SIZE(tcon->ses->server) would be better.

I can respin.

> Do you have any reproducer?

Yes, I'll attach the script.  It sets 1800 named user ACEs, which is 
14436 xattr bytes, ~18046 CIFS bytes

7.2-ish unpatched:

# mount //vm25/user1 /mnt/vm25 -overs=1.0,username=user1,password=pass,unix
# python3 reproduce_acl_overflow.py /mnt/vm25/testfile
Setting ACL with 1800 named user ACEs (14436 xattr bytes, ~18046 CIFS bytes)
Unpatched kernel: expect KASAN report or silent heap corruption
Patched kernel:   expect -E2BIG from setxattr
setxattr failed: [Errno 5] Input/output error: '/mnt/vm25/testfile'

[ 1068.599600] BUG: KASAN: slab-out-of-bounds in 
posix_acl_to_cifs+0x60a/0x6b0 [cifs]
[ 1068.599952] Write of size 8 at addr ffff88810625c0ca by task 
python3/40835

[ 1068.600017] CPU: 2 UID: 0 PID: 40835 Comm: python3 Not tainted 
7.2.0-rc7+ #6 PREEMPT(full)
[ 1068.600029] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
1.17.0-10.fc44 06/10/2025
[ 1068.600041] Call Trace:
[ 1068.600047]  <TASK>
[ 1068.600050]  dump_stack_lvl+0x4e/0x70
[ 1068.600066]  ? posix_acl_to_cifs+0x60a/0x6b0 [cifs]
[ 1068.600312]  print_address_description.constprop.0+0x70/0x300
[ 1068.600324]  ? posix_acl_to_cifs+0x60a/0x6b0 [cifs]
[ 1068.600570]  print_report+0x108/0x209
[ 1068.600580]  ? posix_acl_to_cifs+0x60a/0x6b0 [cifs]
[ 1068.600818]  kasan_report+0xf0/0x120
[ 1068.600827]  ? posix_acl_to_cifs+0x60a/0x6b0 [cifs]
[ 1068.601059]  posix_acl_to_cifs+0x60a/0x6b0 [cifs]
[ 1068.601292]  cifs_do_set_acl+0x446/0xb90 [cifs]
...

[ 1068.616380] Allocated by task 40835:
[ 1068.617021]  kasan_save_stack+0x30/0x50
[ 1068.617033]  kasan_save_track+0x14/0x30
[ 1068.617039]  __kasan_slab_alloc+0x89/0x90
[ 1068.617045]  kmem_cache_alloc_noprof+0x155/0x430
[ 1068.617053]  mempool_alloc_noprof+0x11b/0x1e0
[ 1068.617062]  cifs_buf_get+0x36/0x90 [cifs]
[ 1068.617344]  smb_init+0x43/0x100 [cifs]
[ 1068.617618]  cifs_do_set_acl+0x147/0xb90 [cifs]
[ 1068.617860]  cifs_set_acl+0x717/0x920 [cifs]
[ 1068.618096]  vfs_set_acl+0x35a/0x880
[ 1068.618104]  do_set_acl+0xa6/0x160
[ 1068.618109]  filename_setxattr+0x129/0x170
[ 1068.618115]  path_setxattrat+0x156/0x290
[ 1068.618120]  __x64_sys_setxattr+0xc6/0x140
[ 1068.618125]  do_syscall_64+0xe3/0x540
[ 1068.618133]  entry_SYSCALL_64_after_hwframe+0x76/0x7e

[ 1068.618793] The buggy address belongs to the object at ffff888106258000
                 which belongs to the cache cifs_request of size 16588
[ 1068.619990] The buggy address is located 16586 bytes inside of
                 allocated 16588-byte region [ffff888106258000, 
ffff88810625c0cc)

7.2 patched

# python3 reproduce_acl_overflow.py /mnt/vm25/testfile Setting ACL with 
1800 named user ACEs (14436 xattr bytes, ~18046 CIFS bytes) Unpatched 
kernel: expect KASAN report or silent heap corruption Patched kernel: 
expect -E2BIG from setxattr Got -E2BIG -- fix is active

> What server did you test these changes against?

samba


-- 
Frank Sorenson
sorenson@redhat.com
Principal Software Maintenance Engineer, filesystems
Red Hat

[-- Attachment #2: reproduce_acl_overflow.py --]
[-- Type: text/x-python, Size: 2396 bytes --]

#!/usr/bin/env python3
"""
Reproducer for cifs_do_set_acl() heap overflow.

cifs_set_acl() validates ACL size using Linux xattr format (8 bytes/ACE),
but posix_acl_to_cifs() writes CIFS wire format (10 bytes/ACE) into the
same buffer.  With CIFSMaxBufSize=16384 (default), overflow zone is
~1650-2047 named user ACEs.

Requires Samba share mounted with Unix extensions:
  mount -t cifs //server/share /mnt -o unix,username=user,password=pass

Usage: python3 reproduce_acl_overflow.py <path_on_cifs_mount>
"""
import os, sys, struct

POSIX_ACL_XATTR_VERSION = 2
ACL_USER_OBJ  = 0x0001
ACL_USER      = 0x0002
ACL_GROUP_OBJ = 0x0004
ACL_MASK      = 0x0010
ACL_OTHER     = 0x0020
ACL_READ      = 0x04

def build_acl(n_user_aces):
    """Build a valid POSIX ACL xattr blob with n_user_aces named user entries."""
    entries = [(ACL_USER_OBJ, ACL_READ, 0xffffffff)]
    for uid in range(1, n_user_aces + 1):
        entries.append((ACL_USER, ACL_READ, uid))
    entries += [
        (ACL_GROUP_OBJ, ACL_READ, 0xffffffff),
        (ACL_MASK,      ACL_READ, 0xffffffff),
        (ACL_OTHER,     0,        0xffffffff),
    ]
    blob = struct.pack("<I", POSIX_ACL_XATTR_VERSION)
    for tag, perm, uid in entries:
        blob += struct.pack("<HHI", tag, perm, uid)
    return blob

def main():
    if len(sys.argv) != 2:
        print(f"Usage: {sys.argv[0]} <path_on_cifs_mount>")
        sys.exit(1)

    path = sys.argv[1]
    if not os.path.exists(path):
        open(path, 'w').close()

    # 1800 ACEs: comfortably in the overflow zone for default CIFSMaxBufSize=16384
    # xattr size: 4 + 1804*8 = 14436 bytes  (passes cifs_set_acl check)
    # cifs  size: 6 + 1804*10 = 18046 bytes (overflows ~16588-byte buffer)
    n_aces = 1800
    blob = build_acl(n_aces)
    print(f"Setting ACL with {n_aces} named user ACEs "
          f"({len(blob)} xattr bytes, ~{6 + (n_aces + 4) * 10} CIFS bytes)")
    print("Unpatched kernel: expect KASAN report or silent heap corruption")
    print("Patched kernel:   expect -E2BIG from setxattr")

    try:
        os.setxattr(path, "system.posix_acl_access", blob)
        print("setxattr succeeded (unexpected -- server accepted it)")
    except OSError as e:
        import errno
        if e.errno == errno.E2BIG:
            print("Got -E2BIG -- fix is active")
        else:
            print(f"setxattr failed: {e}")

if __name__ == "__main__":
    main()

Re: [PATCH v2] smb: client: fix heap overflow in cifs_do_set_acl()

[Paulo Alcantara]

Frank Sorenson <sorenson@redhat.com> writes:

> On 8/25/26 7:03 PM, Paulo Alcantara wrote:
>> Frank Sorenson <sorenson@redhat.com> writes:
>>
>>> cifs_set_acl() validates ACL size using posix_acl_xattr_size():
>>>
>>>          4 + (count * 8)  // 4-byte header + 8 bytes per ACE
>>>
>>> cifs_do_set_acl() then calls posix_acl_to_cifs() to write the CIFS
>>> wire format into the same buffer:
>>>
>>>          6 + (count * 10)  // 6-byte header + 10 bytes per ACE
>>>
>>> An ACL that passes the xattr-based check in cifs_set_acl() can
>>> overflow the heap when posix_acl_to_cifs() writes the larger CIFS
>>> format.
>>>
>>> Validate the CIFS format size against the remaining buffer space and
>>> USHRT_MAX before converting--data_count is __u16, so sizes above
>>> USHRT_MAX truncate the on-wire packet length, causing the server to
>>> apply a partial ACL.  Replace MaxDataCount = 1000 with
>>> min(CIFSMaxBufSize, USHRT_MAX).
>>>
>>> Fixes: dc1af4c4b4721 ("cifs: implement set acl method")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Frank Sorenson <sorenson@redhat.com>
>>> ---
>>> v2 changes:
>>>   - Add USHRT_MAX bound to prevent u16 truncation of data_count for ACLs
>>>     with more than 6553 entries, which would cause a partial ACL to be
>>>     silently applied on the server
>>>   - limit MaxDataCount to min(CIFSMaxBufSize, USHRT_MAX)
>>>
>>>   fs/smb/client/cifssmb.c | 13 +++++++++++--
>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
>>> index f5aad5f61dce..621aca5d3b75 100644
>>> --- a/fs/smb/client/cifssmb.c
>>> +++ b/fs/smb/client/cifssmb.c
>>> @@ -3555,6 +3555,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>>>   	int rc = 0;
>>>   	int bytes_returned = 0;
>>>   	__u16 params, byte_count, data_count, param_offset, offset;
>>> +	size_t cifs_acl_size, bytes_available;
>>>   
>>>   	cifs_dbg(FYI, "In SetPosixACL (Unix) for path %s\n", fileName);
>>>   setAclRetry:
>>> @@ -3574,8 +3575,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>>>   	}
>>>   	params = 6 + name_len;
>>>   	pSMB->MaxParameterCount = cpu_to_le16(2);
>>> -	/* BB find max SMB size from sess */
>>> -	pSMB->MaxDataCount = cpu_to_le16(1000);
>>> +	pSMB->MaxDataCount = cpu_to_le16(min_t(unsigned int, CIFSMaxBufSize, USHRT_MAX));
>>>   	pSMB->MaxSetupCount = 0;
>>>   	pSMB->Reserved = 0;
>>>   	pSMB->Flags = 0;
>>> @@ -3587,6 +3587,15 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>>>   	parm_data = ((char *)pSMB) + offset;
>>>   	pSMB->ParameterOffset = cpu_to_le16(param_offset);
>>>   
>>> +	/* make sure we can fit the larger cifs_posix_aces in the buffer */
>>> +	cifs_acl_size = sizeof(struct cifs_posix_acl) +
>>> +		       (acl->a_count * sizeof(struct cifs_posix_ace));
>>> +	bytes_available = (CIFSMaxBufSize + MAX_SMB2_HDR_SIZE) - offset;
>> Are you sure you want to use MAX_SMB2_HDR_SIZE?  This is SMB1 code, so I
>> would expect to see MAX_CIFS_HDR_SIZE.  Alternatively, use
>> MAX_HEADER_SIZE() helper.
>
> The only reason I used MAX_SMB2_HDR_SIZE is that it's the actual size 
> allocated in cifs_buf_get() from cifs_req_cachep...  but now that I 
> think about it, we don't actually want to use the extra 
> (MAX_SMB2_HDR_SIZE - MAX_CIFS_HDR_SIZE) bytes, even though we have it 
> allocated.  So as you say, MAX_CIFS_HDR_SIZE or 
> MAX_HEADER_SIZE(tcon->ses->server) would be better.

Thanks for checking.

> I can respin.

Please.

>> Do you have any reproducer?
>
> Yes, I'll attach the script.  It sets 1800 named user ACEs, which is 
> 14436 xattr bytes, ~18046 CIFS bytes

Thanks.  That's really useful.

[PATCH v4] smb: client: fix heap overflow in cifs_do_set_acl()

[Frank Sorenson]

cifs_set_acl() validates ACL size using posix_acl_xattr_size():

        4 + (count * 8)  // 4-byte header + 8 bytes per ACE

cifs_do_set_acl() then calls posix_acl_to_cifs() to write the CIFS
wire format into the same buffer:

        6 + (count * 10)  // 6-byte header + 10 bytes per ACE

An ACL that passes the xattr-based check in cifs_set_acl() can
overflow the heap when posix_acl_to_cifs() writes the larger CIFS
format.

Validate the CIFS format size against the remaining buffer space and
USHRT_MAX before converting--data_count is __u16, so sizes above
USHRT_MAX truncate the on-wire packet length, causing the server to
apply a partial ACL.  Replace MaxDataCount = 1000 with
min(CIFSMaxBufSize, USHRT_MAX).

Fixes: dc1af4c4b4721 ("cifs: implement set acl method")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
reproducer script which sets 1800 named user ACEs will overflow,
throwing KASAN error or silently corrupting heap

v3/4 changes:
 - change header size to use MAX_HEADER_SIZE helper

v2 changes:
 - Add USHRT_MAX bound to prevent u16 truncation of data_count for ACLs
   with more than 6553 entries, which would cause a partial ACL to be
   silently applied on the server
 - limit MaxDataCount to min(CIFSMaxBufSize, USHRT_MAX)

 fs/smb/client/cifssmb.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index f5aad5f61dce..230af243247c 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -3555,6 +3555,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	int rc = 0;
 	int bytes_returned = 0;
 	__u16 params, byte_count, data_count, param_offset, offset;
+	size_t cifs_acl_size, bytes_available;
 
 	cifs_dbg(FYI, "In SetPosixACL (Unix) for path %s\n", fileName);
 setAclRetry:
@@ -3574,8 +3575,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	}
 	params = 6 + name_len;
 	pSMB->MaxParameterCount = cpu_to_le16(2);
-	/* BB find max SMB size from sess */
-	pSMB->MaxDataCount = cpu_to_le16(1000);
+	pSMB->MaxDataCount = cpu_to_le16(min_t(unsigned int, CIFSMaxBufSize, USHRT_MAX));
 	pSMB->MaxSetupCount = 0;
 	pSMB->Reserved = 0;
 	pSMB->Flags = 0;
@@ -3587,6 +3587,15 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	parm_data = ((char *)pSMB) + offset;
 	pSMB->ParameterOffset = cpu_to_le16(param_offset);
 
+	/* make sure we can fit the larger cifs_posix_aces in the buffer */
+	cifs_acl_size = sizeof(struct cifs_posix_acl) +
+		       (acl->a_count * sizeof(struct cifs_posix_ace));
+	bytes_available = (CIFSMaxBufSize + MAX_HEADER_SIZE(tcon->ses->server)) - offset;
+	if (cifs_acl_size > bytes_available || cifs_acl_size > USHRT_MAX) {
+		rc = -E2BIG;
+		goto setACLerrorExit;
+	}
+
 	/* convert to on the wire format for POSIX ACL */
 	data_count = posix_acl_to_cifs(parm_data, acl, acl_type);
 
-- 
2.55.0

Re: [PATCH v4] smb: client: fix heap overflow in cifs_do_set_acl()

[Paulo Alcantara]

Frank Sorenson <sorenson@redhat.com> writes:

> cifs_set_acl() validates ACL size using posix_acl_xattr_size():
>
>         4 + (count * 8)  // 4-byte header + 8 bytes per ACE
>
> cifs_do_set_acl() then calls posix_acl_to_cifs() to write the CIFS
> wire format into the same buffer:
>
>         6 + (count * 10)  // 6-byte header + 10 bytes per ACE
>
> An ACL that passes the xattr-based check in cifs_set_acl() can
> overflow the heap when posix_acl_to_cifs() writes the larger CIFS
> format.
>
> Validate the CIFS format size against the remaining buffer space and
> USHRT_MAX before converting--data_count is __u16, so sizes above
> USHRT_MAX truncate the on-wire packet length, causing the server to
> apply a partial ACL.  Replace MaxDataCount = 1000 with
> min(CIFSMaxBufSize, USHRT_MAX).
> ...

Applied.