Re: [PATCH v2] lib/string.c: implement stpcpy

[Nick Desaulniers]

On Sat, Aug 15, 2020 at 9:34 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Fri, Aug 14, 2020 at 07:09:44PM -0700, Nick Desaulniers wrote:
> > LLVM implemented a recent "libcall optimization" that lowers calls to
> > `sprintf(dest, "%s", str)` where the return value is used to
> > `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> > in parsing format strings.  Calling `sprintf` with overlapping arguments
> > was clarified in ISO C99 and POSIX.1-2001 to be undefined behavior.
> >
> > `stpcpy` is just like `strcpy` except it returns the pointer to the new
> > tail of `dest`. This allows you to chain multiple calls to `stpcpy` in
> > one statement.
>
> O_O What?
>
> No; this is a _terrible_ API: there is no bounds checking, there are no
> buffer sizes. Anything using the example sprintf() pattern is _already_
> wrong and must be removed from the kernel. (Yes, I realize that the
> kernel is *filled* with this bad assumption that "I'll never write more
> than PAGE_SIZE bytes to this buffer", but that's both theoretically
> wrong ("640k is enough for anybody") and has been known to be wrong in
> practice too (e.g. when suddenly your writing routine is reachable by
> splice(2) and you may not have a PAGE_SIZE buffer).
>
> But we cannot _add_ another dangerous string API. We're already in a
> terrible mess trying to remove strcpy[1], strlcpy[2], and strncpy[3]. This
> needs to be addressed up by removing the unbounded sprintf() uses. (And
> to do so without introducing bugs related to using snprintf() when
> scnprintf() is expected[4].)

Well, everything (-next, mainline, stable) is broken right now (with
ToT Clang) without providing this symbol.  I'm not going to go clean
the entire kernel's use of sprintf to get our CI back to being green.

Thoughts on not exposing the declaration in the header, and changing
the comment to be to the effect of:
"Exists only for optimizing compilers to replace calls to sprintf
with; generally unsafe as bounds checks aren't performed, do not use."
It's still a worthwhile optimization to have, even if the use that
generated it didn't do any bounds checking.

>
> -Kees
>
> [1] https://github.com/KSPP/linux/issues/88
> [2] https://github.com/KSPP/linux/issues/89
> [3] https://github.com/KSPP/linux/issues/90
> [4] https://lore.kernel.org/lkml/20200810092100.GA42813@2f5448a72a42/
>
> --
> Kees Cook



-- 
Thanks,
~Nick Desaulniers