[linux-cifs-client] Re: [PATCH 5/5] cifs: Fix symlink info buffer sizing in cifs_follow_link

[Jeff Layton]

On Wed, 22 Apr 2009 19:14:16 +0530
Suresh Jayaraman <sjayaraman at suse.de> wrote:

> Move the memory allocation from cifs_follow_link to 
> CIFSSMBUnixQuerySymLink and make use of cifs_strlcpy_to_host(). Also
> cleaned up a bit while at it.
> 
> Signed-off-by: Suresh Jayaraman <sjayaraman at suse.de>
> ---
>  fs/cifs/cifssmb.c |   23 +++++++----------------
>  fs/cifs/link.c    |   23 ++++++-----------------
>  2 files changed, 13 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index a02c43b..b5deb47 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -2488,24 +2488,15 @@ querySymLinkRetry:
>  		else {
>  			__u16 data_offset = le16_to_cpu(pSMBr->t2.DataOffset);
>  			__u16 count = le16_to_cpu(pSMBr->t2.DataCount);
> +			char *src = (char *) &pSMBr->hdr.Protocol + data_offset;
> +			int src_len = min_t(const int, buflen, count);
>  
>  			if (pSMBr->hdr.Flags2 & SMBFLG2_UNICODE) {
> -				name_len = UniStrnlen((wchar_t *) ((char *)
> -					&pSMBr->hdr.Protocol + data_offset),
> -					min_t(const int, buflen, count) / 2);
> -			/* BB FIXME investigate remapping reserved chars here */
> -				cifs_strfromUCS_le(symlinkinfo,
> -					(__le16 *) ((char *)&pSMBr->hdr.Protocol
> -							+ data_offset),
> -					name_len, nls_codepage);
> -			} else {
> -				strncpy(symlinkinfo,
> -					(char *) &pSMBr->hdr.Protocol +
> -						data_offset,
> -					min_t(const int, buflen, count));
> -			}
> -			symlinkinfo[buflen] = 0;
> -	/* just in case so calling code does not go off the end of buffer */
> +				rc = cifs_strlcpy_to_host(&symlinkinfo, src,
> +						src_len / 2, 1, nls_codepage);
> +				cFYI(1, ("symlinkinfo = %s", symlinkinfo));
> +			} else
> +				strlcpy(symlinkinfo, src, src_len + 1);
>  		}
>  	}
>  	cifs_buf_release(pSMB);
> diff --git a/fs/cifs/link.c b/fs/cifs/link.c
> index 63f6440..5c1d87c 100644
> --- a/fs/cifs/link.c
> +++ b/fs/cifs/link.c
> @@ -124,11 +124,6 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd)
>  	cFYI(1, ("Full path: %s inode = 0x%p", full_path, inode));
>  	cifs_sb = CIFS_SB(inode->i_sb);
>  	pTcon = cifs_sb->tcon;
> -	target_path = kmalloc(PATH_MAX, GFP_KERNEL);
> -	if (!target_path) {
> -		target_path = ERR_PTR(-ENOMEM);
> -		goto out;
> -	}
>  
>  	/* We could change this to:
>  		if (pTcon->unix_ext)
> @@ -136,11 +131,16 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd)
>  	   get symlink info if we can, even if unix extensions
>  	   turned off for this mount */
>  
> -	if (pTcon->ses->capabilities & CAP_UNIX)
> +	if (pTcon->ses->capabilities & CAP_UNIX) {
>  		rc = CIFSSMBUnixQuerySymLink(xid, pTcon, full_path,
>  					     target_path,
>  					     PATH_MAX-1,
>  					     cifs_sb->local_nls);
> +		if (rc) {
> +			kfree(target_path);
> +			target_path = ERR_PTR(rc);
> +		}
> +	}


Ummmm....that won't work. You're now allocating the buffer in
CIFSSMBUnixQuerySymLink, but target_path is just a normal char pointer.
There's no way to pass the pointer back to the caller unless you change
the args for CIFSSMBUnixQuerySymLink.

Please make sure you test these patches before sending them to the list.

>  	else {
>  		/* BB add read reparse point symlink code here */
>  		/* rc = CIFSSMBQueryReparseLinkInfo */
> @@ -148,17 +148,6 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd)
>  		/* BB Add MAC style xsymlink check here if enabled */
>  	}
>  
> -	if (rc == 0) {
> -
> -/* BB Add special case check for Samba DFS symlinks */
> -
> -		target_path[PATH_MAX-1] = 0;
> -	} else {
> -		kfree(target_path);
> -		target_path = ERR_PTR(rc);
> -	}
> -
> -out:
>  	kfree(full_path);
>  out_no_free:
>  	FreeXid(xid);


-- 
Jeff Layton <jlayton at redhat.com>