Re: [PATCH 5/5] cifs: Fix symlink info buffer sizing in cifs_follow_link (try #2)

On Thu, 23 Apr 2009 00:22:08 +0530
Suresh Jayaraman <[email protected]> wrote:

> Jeff Layton wrote:
> > On Wed, 22 Apr 2009 19:14:16 +0530
> > Suresh Jayaraman <[email protected]> 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.
> >>
> >> -	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.
> 
> Here's the fixed and tested patch:
> 
> 
> 
>  fs/cifs/cifsproto.h |    2 +-
>  fs/cifs/cifssmb.c   |   26 +++++++++++---------------
>  fs/cifs/link.c      |   25 +++++++------------------
>  3 files changed, 19 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 4167716..46b926d 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -261,7 +261,7 @@ extern int CIFSUnixCreateSymLink(const int xid,
>  extern int CIFSSMBUnixQuerySymLink(const int xid,
>  			struct cifsTconInfo *tcon,
>  			const unsigned char *searchName,
> -			char *syminfo, const int buflen,
> +			char **syminfo, const int buflen,
>  			const struct nls_table *nls_codepage);
>  extern int CIFSSMBQueryReparseLinkInfo(const int xid,
>  			struct cifsTconInfo *tcon,
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index a02c43b..e64edea 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -2418,7 +2418,7 @@ winCreateHardLinkRetry:
>  int
>  CIFSSMBUnixQuerySymLink(const int xid, struct cifsTconInfo *tcon,
>  			const unsigned char *searchName,
> -			char *symlinkinfo, const int buflen,
> +			char **symlinkinfo, const int buflen,
>  			const struct nls_table *nls_codepage)
>  {
>  /* SMB_QUERY_FILE_UNIX_LINK */
> @@ -2488,24 +2488,20 @@ 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);
> +				rc = cifs_strlcpy_to_host(symlinkinfo, src,
> +						src_len / 2, 1, nls_codepage);
> +				cFYI(1, ("symlinkinfo = %s", *symlinkinfo));
>  			} else {
> -				strncpy(symlinkinfo,
> -					(char *) &pSMBr->hdr.Protocol +
> -						data_offset,
> -					min_t(const int, buflen, count));
> +				*symlinkinfo = kmalloc(src_len + 1, GFP_KERNEL);
> +				if (*symlinkinfo)
> +					strlcpy(*symlinkinfo, src, src_len + 1);
> +				else
> +					rc = -ENOMEM;
>  			}
> -			symlinkinfo[buflen] = 0;
> -	/* just in case so calling code does not go off the end of buffer */
>  		}
>  	}
>  	cifs_buf_release(pSMB);
> diff --git a/fs/cifs/link.c b/fs/cifs/link.c
> index 63f6440..03b3793 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,
> +					     &target_path,
>  					     PATH_MAX-1,
>  					     cifs_sb->local_nls);
> +		if (rc) {
> +			kfree(target_path);
> +			target_path = ERR_PTR(rc);
> +		}
> +	}
>  	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);

Much better.

Acked-by: Jeff Layton <[email protected]>

...you can also add my Acked-by to patches 1-4 in the earlier set as well.