[linux-cifs-client] [PATCH] cifs: Fix insufficient memory allocation for nativeFileSystem field

[Suresh Jayaraman]

Jeff Layton wrote:
> On Mon, 06 Apr 2009 12:35:58 +0530
> Suresh Jayaraman <sjayaraman at suse.de> wrote:

>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 0de3b56..b361be0 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -3674,7 +3674,7 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses,
>>  			    BCC(smb_buffer_response)) {
>>  				kfree(tcon->nativeFileSystem);
>>  				tcon->nativeFileSystem =
>> -				    kzalloc(2*(length + 1), GFP_KERNEL);
>> +				    kzalloc((4 * length) + 2, GFP_KERNEL);
>>  				if (tcon->nativeFileSystem)
>>  					cifs_strfromUCS_le(
>>  						tcon->nativeFileSystem,
> 
> Wait...is this even enough? It looks like nls.h defines this:
> 
> /* this value hold the maximum octet of charset */
> #define NLS_MAX_CHARSET_SIZE 6 /* for UTF-8 */
> 
> ...it really looks like this needs to use the same constant.

True. Seems I was influenced by a comment in fs/cifs/sess.c

313         /* UTF-8 string will not grow more than four times as big as
UCS-16 */

> There are other places in this code that make this sort of allocation.
> Could you audit and fix them too? A better solution is really needed
> here.

Yeah, there are other files (for e.g sess.c) as well. Looks like we
intentionally limit maxlen to 512, may be we want to limit in NL case
too? Wondering what could be the rationale behind this..

> A helper function that basically does the allocation and buffer-length
> limited conversion would be ideal. We have some functions that sort of
> do this, but none of them seem to be quite right. Maybe the best thing
> is just to fix cifs_strncpy_to_host() so that it's right and fix most
> of the places that do this allocation manually to do it using that
> function instead.
> 

I agree. This seems the right thing to me, too. I'll try to fix and
resend this patch.


Thanks,

-- 
Suresh Jayaraman