Re: [PATCH 1/2] cifs: fix buffer size for tcon->nativeFileSystem field

On Thu, 16 Apr 2009 15:41:33 +0000
Dave Kleikamp <[email protected]> wrote:

> On Thu, 2009-04-16 at 11:21 -0400, Jeff Layton wrote:
> > The buffer for this was resized recently to fix a bug. It's still
> > possible however that a malicious server could overflow this field
> > by sending characters in it that are >2 bytes in the local charset.
> > Double the size of the buffer to account for this possibility.
> > 
> > Also get rid of some really strange and seemingly pointless NULL
> > termination. It's NULL terminating the string in the source buffer,
> > but by the time that happens, we've already copied the string.
> > 
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> >  fs/cifs/connect.c |    7 ++-----
> >  1 files changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 01e280c..1a93604 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -3756,16 +3756,13 @@ 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,
> >  						(__le16 *) bcc_ptr,
> >  						length, nls_codepage);
> > -				bcc_ptr += 2 * length;
> > -				bcc_ptr[0] = 0;	/* null terminate the string */
> > -				bcc_ptr[1] = 0;
> > -				bcc_ptr += 2;
> > +				bcc_ptr += (2 * length) + 2;
> 
> What's the point of updating bcc_ptr here?  It's not accurate anyway.
> The correct thing would be:
> 
> bcc_ptr += cifs_strfromUCS_le(... );
> 
> but bcc_ptr isn't used again, so there's no point.
> 
> Shaggy
> -- 
> David Kleikamp
> IBM Linux Technology Center

Here's a respun patch that removes the bcc_ptr update.

-- 
Jeff Layton <[email protected]>

_______________________________________________
linux-cifs-client mailing list
[email protected]
https://lists.samba.org/mailman/listinfo/linux-cifs-client

>From d4a32dbdeb49a77536744a16ddce6e04e0af9592 Mon Sep 17 00:00:00 2001
From: Jeff Layton <[email protected]>
Date: Thu, 16 Apr 2009 13:34:59 -0400
Subject: [PATCH] cifs: fix buffer size for tcon->nativeFileSystem field

The buffer for this was resized recently to fix a bug. It's still
possible however that a malicious server could overflow this field
by sending characters in it that are >2 bytes in the local charset.
Double the size of the buffer to account for this possibility.

Also get rid of some really strange and seemingly pointless NULL
termination. It's NULL terminating the string in the source buffer,
but by the time that happens, we've already copied the string.

Signed-off-by: Jeff Layton <[email protected]>
---
 fs/cifs/connect.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 01e280c..9cdc99c 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3756,16 +3756,12 @@ 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,
 						(__le16 *) bcc_ptr,
 						length, nls_codepage);
-				bcc_ptr += 2 * length;
-				bcc_ptr[0] = 0;	/* null terminate the string */
-				bcc_ptr[1] = 0;
-				bcc_ptr += 2;
 			}
 			/* else do not bother copying these information fields*/
 		} else {
-- 
1.5.5.6