Re: [PATCH 1/1] cifs: fix buffer overflow in SMB2_tcon

[Paulo Alcantara <pc@manguebit.org>]

Ren Wei <n05ec@lzu.edu.cn> writes:

> From: Luxiao Xu <rakukuip@gmail.com>
>
> In SMB2_tcon(), unc_path is allocated with a fixed buffer size of
> MAX_SHARENAME_LENGTH * 2 bytes (672 bytes). However, the user-controlled
> UNC path (tree) length is passed to cifs_strtoUTF16(), which assumes the
> destination buffer is large enough and writes strlen(tree) + 1 UTF-16 code
> units.
>
> Commit 62a1a439e0fd ("cifs: clean up handling of unc= option") removed the
> length check on UNC mount options, allowing mount paths to exceed
> MAX_SHARENAME_LENGTH while retaining the fixed-size buffer allocation in
> SMB2_tcon(). Consequently, an oversized UNC string causes cifs_strtoUTF16()
> to write past the end of the heap object, resulting in a slab-out-of-bounds
> write.
>
> Fix this by dynamically allocating unc_path based on the length of tree so
> that it safely accommodates all UTF-16 code units.

I don't think we should allow SMB2_tcon() with a @tree more than
MAX_TREE_SIZE characters.

With your fix, we might end up with a truncated tree name set in
cifs_tcon::tree_name.  It will work in the first tree connect, but if
the client needs to issue another tree connect for some reason
(e.g. reconnect/failover), then it will fail.

Some places rely on the tree name not having more than MAX_TREE_SIZE
characters.  I'd rather return -EINVAL along with an error message in
SMB2_tcon() if @tree exceeds that length.

From another patch, we could also add an early MAX_TREE_SIZE check
against smb3_fs_context::UNC in smb3_parse_devname().

BTW, mount.cifs(8) will not allow an UNC path that has more than 1024
characters.

What do you think?