[linux-cifs-client] [PATCH 2/3] cifs: Make cifs_strlcpy_to_host use this helper and fix incorrect NULL termination

[Jeff Layton]

On Wed, 22 Apr 2009 15:25:50 +0530
Suresh Jayaraman <sjayaraman at suse.de> wrote:

> > This is fine for now, but maybe we should just fix cifs_strfromUCS_le
> 
> Yes, I think.
> 
> > to do the double-byte termination? Is a single-byte terminator ever
> > useful?
> 
> I just looked at all the callers (15) and there is no caller where a
> single byte terminator is useful. There are couple of callers (mostly
> unused SessSetup/NTLMSSP code plus CIFSTCon) where this would not matter
> as they already use kzalloc().
> 
> > Fixing that will of course mean auditing all of the callers, but I
> > think they all have big enough buffers for this now. Right?
> 
> Most of them have sufficient buffers. The following ones needs attention:
> 
> - symlinkinfo buffer in CIFSSMBUnixQuerySymLink()(allocated in
> cifs_follow_link) - Is PATH_MAX (4096) the MAX for nls strings too?
> or may be we need to move the allocation to CIFSSMBUnixQuerySymLink()
> and use UniStrnlenBytes?
> 

That's probably the best solution -- just switch it to use
strlcpy_to_host. Let's use that helper wherever we can (unless doing so
is problematic for some reason).

> - CIFSSMBQueryReparseLinkInfo() (allocated in cifs_readlink() which is
> unused anyway. Not sure whether it will be used in future or not).
> 

You're right that that's unused. Steve, care to comment? What was the
intent of that code?

> - buffers in SessSetup/NTLMSP code. Do we need to fix this too?
> 

That code is heavily bitrotted now. Maybe we should just rip it out. If
we end up taking something like that patchset I proposed yesterday
it'll also lay the groundwork for doing NTLMSSP in cifs.upcall. We can
use the new "credinfo" arg to send the password to the upcall.

It's hard to imagine that anyone is setting the experimental flag in
/proc/fs/cifs in order to use it.

> It appears to me that we can fix cifs_strfromUCS_le to do double-byte
> NULL termination as most of the in-use buffers are big enough.
> 

I suggest that you go ahead and do it then.

> >>  	} else {
> >> -		plen = strnlen(src, maxlen);
> >> -		*dst = kmalloc(plen + 2, GFP_KERNEL);
> >> +		src_len = strnlen(src, maxlen);
> >> +		*dst = kmalloc(src_len + 1, GFP_KERNEL);
> >>  		if (!*dst)
> >> -			goto cifs_strlcpy_to_host_ErrExit;
> >> -		strlcpy(*dst, src, plen);
> >> +			goto err_exit;
> >> +		strlcpy(*dst, src, src_len);
> > 				   ^^^^^^^
> > should be src_len+1.
> 
> Yes, should be fixed.
> 
> >>  	}
> >>  	return 0;
> >>  
> >> -cifs_strlcpy_to_host_ErrExit:
> >> +err_exit:
> >>  	cERROR(1, ("Failed to allocate buffer for string\n"));
> >>  	return -ENOMEM;
> >>  }
> >> _______________________________________________
> 
> 
> 
> -- 
> Suresh Jayaraman


-- 
Jeff Layton <jlayton at redhat.com>