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

[Paulo Alcantara <pc@manguebit.org>]

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.