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

[Jeff Layton]

On Mon, 06 Apr 2009 18:20:21 +0530
Suresh Jayaraman <sjayaraman at suse.de> wrote:

> 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 */
> 

That looks like that's wrong (or at least potentially so). AFAICT, UTF-8
allows up to 6 bytes per character. I suppose that it's possible that
none of the characters allowed in UCS-16 will ever translate to a
character that's more than 4 bytes, but I'd like to see that confirmed
before we depend on it.

> > 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..
> 

Good Q. This is the problem with sprinkling "magic numbers" around the
code.

> > 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.
> 
> 

Excellent. I look forward to seeing it.
-- 
Jeff Layton <jlayton at redhat.com>