[linux-cifs-client] [PATCH 2/2] cifs: Increase size of tmp_buf in cifs_readdir to avoid potential overflows

[Suresh Jayaraman]

Günter Kukkukk wrote:
> Am Montag, 20. April 2009 schrieb Steve French:
>> Merged this and also patch 1 of 2
>>
>> thx
>>
>>>> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
>>>> index 1a8be62..ebd0da7 100644
>>>> --- a/fs/cifs/readdir.c
>>>> +++ b/fs/cifs/readdir.c
>>>> @@ -1074,7 +1074,7 @@ int cifs_readdir(struct file *file, void *direntry, filldir_t filldir)
>>>> ý ý ý ý ý ý ý with the rare long characters alloc more to account for
>>>> ý ý ý ý ý ý ý such multibyte target UTF-8 characters. cifs_unicode.c,
>>>> ý ý ý ý ý ý ý which actually does the conversion, has the same limit */
>>>> - ý ý ý ý ý ý tmp_buf = kmalloc((2 * NAME_MAX) + 4, GFP_KERNEL);
>>>> + ý ý ý ý ý ý tmp_buf = kmalloc((4 * NAME_MAX) + 2, GFP_KERNEL);
>>>> ý ý ý ý ý ý ý for (i = 0; (i < num_to_fill) && (rc == 0); i++) {
>>>> ý ý ý ý ý ý ý ý ý ý ý if (current_entry == NULL) {
>>>> ý ý ý ý ý ý ý ý ý ý ý ý ý ý ý /* evaluate whether this case is an error */
>>> Acked-by: Jeff Layton <jlayton at redhat.com>
>>>
>>
>>
> 
> I think patch 1 
>   - cifs: Rename cifs_strncpy_to_host and fix buffer size
> is wrong (which also results from using weak variables names).
> 
>                (*dst)[plen] = 0;
>                (*dst)[plen+1] = 0; /* needed for Unicode */
> 
> is using _source_ "plen" length when applied to dest. string.

Good catch, agreed, [PATCH 1/2] while only focused on allocating the
right buffer size, it didn't ensure correct NULL termination of dst
buffer after the change. Sorry about the oversight.

Also it is correct to use the length of destination string (in bytes)
while null terminating than using length of source string (in UCS
chars which was the case earlier too).

> In addition, hardcoded (numbered) stuff like this
>    *dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
> is also wrong.
> It's based on todays (right) assumption, that an UTF-8 string
> can only consume max. 4 bytes per char.
> But - the maximum length is defined in:
> ./include/linux/nls.h:#define NLS_MAX_CHARSET_SIZE 6 /* for UTF-8 */

NLS_MAX_CHARSET_SIZE is a bit too much, I think.

>From man utf-8:
UTF-8 encoded UCS characters may be up to six bytes long, however the
Unicode  standard  specifies no characters above 0x10ffff, so Unicode
characters can only be up to four bytes long in UTF-8.

There were a lot of discussion on this over the past weeks and we
really wanted to have common helpers to calculate the length instead
of going by assumptions. And since there are other filesystems that
would need this helper the decision was to generalise the helper and
make it part of common code (for e.g. fs/nls) and let other filesystems
such as jfs/ntfs make use of it. This patch is an interim measure to fix
the potential overflows.

Also, see discussions:
http://lists.samba.org/archive/linux-cifs-client/2009-April/004322.html
http://lists.samba.org/archive/linux-cifs-client/2009-April/004421.html


> At least that hardcoded "4" should be carefully defined somewhere - if
> ever used!
> 
> my current diff (not tested) - temporary:
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index a02c43b..c9dcc1b 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -91,22 +91,25 @@ static int
>  cifs_strlcpy_to_host(char **dst, const char *src, const int maxlen,
>                  const bool is_unicode, const struct nls_table *nls_codepage)
>  {
> -       int plen;
> +       int src_len, dst_len;
> 
>         if (is_unicode) {
> -               plen = UniStrnlen((wchar_t *)src, maxlen);
> -               *dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
> +               src_len = UniStrnlen((wchar_t *)src, maxlen);
> +               *dst = kmalloc((NLS_MAX_CHARSET_SIZE * src_len) + 2, GFP_KERNEL);
>                 if (!*dst)
>                         goto cifs_strlcpy_to_host_ErrExit;
> -               cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage);
> -               (*dst)[plen] = 0;
> -               (*dst)[plen+1] = 0; /* needed for Unicode */
> +               dst_len = cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage);
> +               /* Note:
> +                * cifs_strfromUCS_le already zero terminates the string
> +                * using 1 null byte
> +                */
> +               (*dst)[dst_len+1] = 0; /* needed for Unicode (really?)*/
>         } else {
> -               plen = strnlen(src, maxlen);
> -               *dst = kmalloc(plen + 2, GFP_KERNEL);
> +               src_len = strnlen(src, maxlen);
> +               *dst = kmalloc(src_len + 1, GFP_KERNEL);
>                 if (!*dst)
> 
need to pass src_len to strlcpy as well.

Here is the updated patch with 2 changes (mostly nits) to
Guenter's version, NLS_MAX_CHARSET_SIZE set to 4, pass src_len
to strlcpy in non-unicode case.

The previous patch while fixing the buffer size, left out the NULL
termination that is based on length of src buffer(in UCS characters)
as it is. Fix this by using the length of dst buffer got from
cifs_strfromUCS_le() for NULL termination.


---
 fs/cifs/cifssmb.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index a02c43b..9f5f7aa 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -91,22 +91,25 @@ static int
 cifs_strlcpy_to_host(char **dst, const char *src, const int maxlen,
 		 const bool is_unicode, const struct nls_table *nls_codepage)
 {
-	int plen;
+	int src_len, dst_len;
 
 	if (is_unicode) {
-		plen = UniStrnlen((wchar_t *)src, maxlen);
-		*dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
+		src_len = UniStrnlen((wchar_t *)src, maxlen);
+		*dst = kmalloc((4 * src_len) + 2, GFP_KERNEL);
 		if (!*dst)
 			goto cifs_strlcpy_to_host_ErrExit;
-		cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage);
-		(*dst)[plen] = 0;
-		(*dst)[plen+1] = 0; /* needed for Unicode */
+		dst_len = cifs_strfromUCS_le(*dst, (__le16 *)src, src_len,
+					     nls_codepage);
+		/*
+		 * cifs_strfromUCS_le() ensures single byte NULL termination
+		 */
+		(*dst)[dst_len + 1] = 0; /* needed for Unicode, to be safe */
 	} else {
-		plen = strnlen(src, maxlen);
-		*dst = kmalloc(plen + 2, GFP_KERNEL);
+		src_len = strnlen(src, maxlen);
+		*dst = kmalloc(src_len + 1, GFP_KERNEL);
 		if (!*dst)
 			goto cifs_strlcpy_to_host_ErrExit;
-		strlcpy(*dst, src, plen);
+		strlcpy(*dst, src, src_len);
 	}
 	return 0;