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

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