Re: RFC for a new string-copy function, using mixtures of strlcpy and strscpy

[Kees Cook]

On Mon, Nov 08, 2021 at 06:15:54PM +0530, Ajay Garg wrote:
> Hi Greg,
> 
> Thanks for your time.
> 
> >
> > Wait, why?  We have recently added new string copy functions to resolve
> > the type of issues you have pointed out.  The kernel is not yet
> > converted to use them, so why add yet-another-function that also is not
> > used?
> 
> Greg, would request your couple of minutes more, in suggesting a
> concrete way forward, by working through an example as below.
> 
> 
> In the file fs/kernfs/dir.c, there is a statement
> 
>         return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
> 
> Here, there is no information of how long kn->name might be, so there
> is a definite chance of overflow happening. Thus, accordingly, strlcpy
> is used, to bound copying of upto buflen bytes. Of course, buf
> (destination-buffer) is safe from any overflow now.
> 
> However, iffff strlen(kn->name) is greater than (buflen - 1), then
> strlcpy would return a different value than the number of bytes
> actually copied. Since there is no check, this (wrong) return value
> will be propagated to the clients down the stack.

The propagation issues are the real problem here, IMO. strlcpy returns
strlen(src), which is bound to cause problems if the result in used to
adjust string index, and strscpy returns -E2BIG on overflow, which can
cause different problems if the result in used to adjust string indexes.

strlcpy has the added problem of potentially performing OOB reads when
src is unterminated, so it needs to be entirely removed from the kernel
in favor of strscpy[1]. But yes, as you say, it isn't a drop-in
replacement, but that's because it shouldn't be -- the return values
need to be checked in both cases.

> Now, in the current situation, following is the remedy :
> 
> ########################################
>         int ret = strlcpy(buf, kn->parent ? kn->name : "/", buflen);
>         if(ret >= buflen)
>             ret = buflen - 1;
> 
>         return ret;
> ########################################

We do have other functions that "hide" the overflow (e.g. scnprintf),
but those are usually more common in large string constructions, or
chains of copies. When doing a single string copy, I think it's
important that the caller decide explicitly what to do in the overflow
case.

For the specific fs/kerfs/dir.c case, I don't see any problems --
nothing uses the result (cgroup_name() is the only caller of
kernfs_name() that I see).

-- 
Kees Cook