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

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.
>>
>> Signed-off-by: Suresh Jayaraman <[email protected]>
>> ---
>>  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,
					^^^^^^^^^^
It's already passed to CIFSSMBUnixQuerySymLink(). Now only the
allocation is done in CIFSSMBUnixQuerySymLink()

					     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.

Did I miss something ?

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

I must have been in a hurry,  Sorry, except for UniStrlenBytes patch the
others are only compile tested as the changes seemed trivial..

>>  	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);
> 
> 


-- 
Suresh Jayaraman