Re: [PATCH 2/3] cifs: Make cifs_strlcpy_to_host use this helper and fix incorrect NULL termination

Jeff Layton wrote:
> On Tue, 21 Apr 2009 17:55:15 +0530
> Suresh Jayaraman <[email protected]> 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;
>>  }
>> _______________________________________________



-- 
Suresh Jayaraman