[linux-cifs-client,2/3] cifs: Make cifs_strlcpy_to_host use this helper and fix incorrect NULL termination
diff mbox

Message ID 49EDBB2B.8020503@suse.de
State New, archived
Headers show

Commit Message

Suresh Jayaraman April 21, 2009, 12:25 p.m. UTC
Make cifs_strlcpy_to_host use UniStrnlenBytes(). Also fix a bug
introduced by a previous patch. 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.


Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
---
 fs/cifs/cifssmb.c |   29 +++++++++++++++++------------
 1 files changed, 17 insertions(+), 12 deletions(-)

Comments

Jeff Layton April 21, 2009, 4:15 p.m. UTC | #1
On Tue, 21 Apr 2009 17:55:15 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> Make cifs_strlcpy_to_host use UniStrnlenBytes(). Also fix a bug
> introduced by a previous patch. 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.
> 
> 
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> ---
>  fs/cifs/cifssmb.c |   29 +++++++++++++++++------------
>  1 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index a02c43b..aab1d32 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -91,26 +91,31 @@ 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;
> +	size_t nbytes;
>  
>  	if (is_unicode) {
> -		plen = UniStrnlen((wchar_t *)src, maxlen);
> -		*dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
> +		nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, &src_len,
> +					 nls_codepage);
> +		*dst = kmalloc(nbytes + 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 */
> +			goto err_exit;
> +		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 */

This is fine for now, but maybe we should just fix cifs_strfromUCS_le
to do the double-byte termination? Is a single-byte terminator ever
useful?

Fixing that will of course mean auditing all of the callers, but I
think they all have big enough buffers for this now. Right?

>  	} 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);
> +			goto err_exit;
> +		strlcpy(*dst, src, src_len);
				   ^^^^^^^
should be src_len+1.

>  	}
>  	return 0;
>  
> -cifs_strlcpy_to_host_ErrExit:
> +err_exit:
>  	cERROR(1, ("Failed to allocate buffer for string\n"));
>  	return -ENOMEM;
>  }
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>
Steve French April 21, 2009, 4:40 p.m. UTC | #2
On Tue, Apr 21, 2009 at 11:15 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Tue, 21 Apr 2009 17:55:15 +0530
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>
>> Make cifs_strlcpy_to_host use UniStrnlenBytes(). Also fix a bug
>> introduced by a previous patch. 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.
>> index a02c43b..aab1d32 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -91,26 +91,31 @@ 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;
>> +     size_t nbytes;
>>
>>       if (is_unicode) {
>> -             plen = UniStrnlen((wchar_t *)src, maxlen);
>> -             *dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
>> +             nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, &src_len,
>> +                                      nls_codepage);
>> +             *dst = kmalloc(nbytes + 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 */
>> +                     goto err_exit;
>> +             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 */
>
> This is fine for now, but maybe we should just fix cifs_strfromUCS_le
> to do the double-byte termination? Is a single-byte terminator ever
> useful?
>
> Fixing that will of course mean auditing all of the callers, but I
> think they all have big enough buffers for this now. Right?

Jeff's argument makes sense :)


>>       } 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);
>> +                     goto err_exit;
>> +             strlcpy(*dst, src, src_len);
>                                   ^^^^^^^
> should be src_len+1.
>
>>       }
>>       return 0;
>>
>> -cifs_strlcpy_to_host_ErrExit:
>> +err_exit:
>>       cERROR(1, ("Failed to allocate buffer for string\n"));
>>       return -ENOMEM;
>>  }
>> _______________________________________________
>> linux-cifs-client mailing list
>> linux-cifs-client@lists.samba.org
>> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>>
>
>
> --
> Jeff Layton <jlayton@redhat.com>
>
Suresh Jayaraman April 22, 2009, 9:55 a.m. UTC | #3
Jeff Layton wrote:
> On Tue, 21 Apr 2009 17:55:15 +0530
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
> 
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index a02c43b..aab1d32 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -91,26 +91,31 @@ 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;
>> +	size_t nbytes;
>>  
>>  	if (is_unicode) {
>> -		plen = UniStrnlen((wchar_t *)src, maxlen);
>> -		*dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
>> +		nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, &src_len,
>> +					 nls_codepage);
>> +		*dst = kmalloc(nbytes + 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 */
>> +			goto err_exit;
>> +		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 */
> 
> This is fine for now, but maybe we should just fix cifs_strfromUCS_le

Yes, I think.

> to do the double-byte termination? Is a single-byte terminator ever
> useful?

I just looked at all the callers (15) and there is no caller where a
single byte terminator is useful. There are couple of callers (mostly
unused SessSetup/NTLMSSP code plus CIFSTCon) where this would not matter
as they already use kzalloc().

> Fixing that will of course mean auditing all of the callers, but I
> think they all have big enough buffers for this now. Right?

Most of them have sufficient buffers. The following ones needs attention:

- symlinkinfo buffer in CIFSSMBUnixQuerySymLink()(allocated in
cifs_follow_link) - Is PATH_MAX (4096) the MAX for nls strings too?
or may be we need to move the allocation to CIFSSMBUnixQuerySymLink()
and use UniStrnlenBytes?

- CIFSSMBQueryReparseLinkInfo() (allocated in cifs_readlink() which is
unused anyway. Not sure whether it will be used in future or not).

- buffers in SessSetup/NTLMSP code. Do we need to fix this too?

It appears to me that we can fix cifs_strfromUCS_le to do double-byte
NULL termination as most of the in-use buffers are big enough.

>>  	} 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);
>> +			goto err_exit;
>> +		strlcpy(*dst, src, src_len);
> 				   ^^^^^^^
> should be src_len+1.

Yes, should be fixed.

>>  	}
>>  	return 0;
>>  
>> -cifs_strlcpy_to_host_ErrExit:
>> +err_exit:
>>  	cERROR(1, ("Failed to allocate buffer for string\n"));
>>  	return -ENOMEM;
>>  }
>> _______________________________________________
Jeff Layton April 22, 2009, 10:27 a.m. UTC | #4
On Wed, 22 Apr 2009 15:25:50 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> > This is fine for now, but maybe we should just fix cifs_strfromUCS_le
> 
> Yes, I think.
> 
> > to do the double-byte termination? Is a single-byte terminator ever
> > useful?
> 
> I just looked at all the callers (15) and there is no caller where a
> single byte terminator is useful. There are couple of callers (mostly
> unused SessSetup/NTLMSSP code plus CIFSTCon) where this would not matter
> as they already use kzalloc().
> 
> > Fixing that will of course mean auditing all of the callers, but I
> > think they all have big enough buffers for this now. Right?
> 
> Most of them have sufficient buffers. The following ones needs attention:
> 
> - symlinkinfo buffer in CIFSSMBUnixQuerySymLink()(allocated in
> cifs_follow_link) - Is PATH_MAX (4096) the MAX for nls strings too?
> or may be we need to move the allocation to CIFSSMBUnixQuerySymLink()
> and use UniStrnlenBytes?
> 

That's probably the best solution -- just switch it to use
strlcpy_to_host. Let's use that helper wherever we can (unless doing so
is problematic for some reason).

> - CIFSSMBQueryReparseLinkInfo() (allocated in cifs_readlink() which is
> unused anyway. Not sure whether it will be used in future or not).
> 

You're right that that's unused. Steve, care to comment? What was the
intent of that code?

> - buffers in SessSetup/NTLMSP code. Do we need to fix this too?
> 

That code is heavily bitrotted now. Maybe we should just rip it out. If
we end up taking something like that patchset I proposed yesterday
it'll also lay the groundwork for doing NTLMSSP in cifs.upcall. We can
use the new "credinfo" arg to send the password to the upcall.

It's hard to imagine that anyone is setting the experimental flag in
/proc/fs/cifs in order to use it.

> It appears to me that we can fix cifs_strfromUCS_le to do double-byte
> NULL termination as most of the in-use buffers are big enough.
> 

I suggest that you go ahead and do it then.

> >>  	} 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);
> >> +			goto err_exit;
> >> +		strlcpy(*dst, src, src_len);
> > 				   ^^^^^^^
> > should be src_len+1.
> 
> Yes, should be fixed.
> 
> >>  	}
> >>  	return 0;
> >>  
> >> -cifs_strlcpy_to_host_ErrExit:
> >> +err_exit:
> >>  	cERROR(1, ("Failed to allocate buffer for string\n"));
> >>  	return -ENOMEM;
> >>  }
> >> _______________________________________________
> 
> 
> 
> -- 
> Suresh Jayaraman

Patch
diff mbox

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index a02c43b..aab1d32 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -91,26 +91,31 @@  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;
+	size_t nbytes;
 
 	if (is_unicode) {
-		plen = UniStrnlen((wchar_t *)src, maxlen);
-		*dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
+		nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, &src_len,
+					 nls_codepage);
+		*dst = kmalloc(nbytes + 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 */
+			goto err_exit;
+		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);
+			goto err_exit;
+		strlcpy(*dst, src, src_len);
 	}
 	return 0;
 
-cifs_strlcpy_to_host_ErrExit:
+err_exit:
 	cERROR(1, ("Failed to allocate buffer for string\n"));
 	return -ENOMEM;
 }