Re: [PATCH 5/5] cifs: Fix buffer size in cifs_strncpy_to_host

On Fri, 17 Apr 2009 20:51:11 +0530
Suresh Jayaraman <[email protected]> wrote:


It would be better to just switch all of the functions to use this
function instead of adding a unicode-specific one:

> Fix insufficient buffer allocation and replace kmalloc() with
> kzalloc() so that we ensure safe NULL termination always in
> unicode case.
> 
> Signed-off-by: Suresh Jayaraman <[email protected]>
> ---
>  fs/cifs/cifssmb.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> Index: cifs-2.6.git/fs/cifs/cifssmb.c
> ===================================================================
> --- cifs-2.6.git.orig/fs/cifs/cifssmb.c
> +++ cifs-2.6.git/fs/cifs/cifssmb.c
> @@ -123,22 +123,24 @@ cifs_strncpy_to_host(char **dst, const c

Let's rename this cifs_strlcpy_to_host since it guarantees a null terminated
string and the behavior is closer.

>  		 const bool is_unicode, const struct nls_table *nls_codepage)
>  {
>  	int plen;
> +	size_t nbytes;
>  
>  	if (is_unicode) {
> -		plen = UniStrnlen((wchar_t *)src, maxlen);
> -		*dst = kmalloc(plen + 2, GFP_KERNEL);
> +		nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, &plen,
> +					 nls_codepage);
> +		*dst = kzalloc(nbytes + 2, GFP_KERNEL);

I think we should probably just keep these as a kmalloc and just
forcably NULL terminate the end. That's a minor nit though.

>  		if (!*dst)
>  			goto cifs_strncpy_to_host_ErrExit;
>  		cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage);
> +		/* kzalloc() ensures NULL termination */
>  	} else {
>  		plen = strnlen(src, maxlen);
>  		*dst = kmalloc(plen + 2, GFP_KERNEL);
>  		if (!*dst)
>  			goto cifs_strncpy_to_host_ErrExit;
>  		strncpy(*dst, src, plen);
> +		(*dst)[plen] = 0;

		^^^^ just use strlcpy...
>  	}
> -	(*dst)[plen] = 0;
> -	(*dst)[plen+1] = 0; /* harmless for ASCII case, needed for Unicode */
>  	return 0;
>  
>  cifs_strncpy_to_host_ErrExit:

-- 
Jeff Layton <[email protected]>