diff mbox

[linux-cifs-client,5/5] cifs: Fix buffer size in cifs_strncpy_to_host

Message ID 49E89E67.2030900@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Suresh Jayaraman April 17, 2009, 3:21 p.m. UTC
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 <sjayaraman@suse.de>
---
 fs/cifs/cifssmb.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

Comments

Jeff Layton April 17, 2009, 7:23 p.m. UTC | #1
On Fri, 17 Apr 2009 20:51:11 +0530
Suresh Jayaraman <sjayaraman@suse.de> 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 <sjayaraman@suse.de>
> ---
>  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:
diff mbox

Patch

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
 		 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);
 		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;
 	}
-	(*dst)[plen] = 0;
-	(*dst)[plen+1] = 0; /* harmless for ASCII case, needed for Unicode */
 	return 0;
 
 cifs_strncpy_to_host_ErrExit: