Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Stephen Kitt <steve@sk2.org>]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

On Tue, 2 Jul 2019 10:25:04 -0700, Kees Cook <keescook@chromium.org> wrote:
> On Sat, Jun 29, 2019 at 06:15:37PM +0200, Stephen Kitt wrote:
> > On Fri, 28 Jun 2019 17:25:48 +0530, Nitin Gote <nitin.r.gote@intel.com>
> > wrote:  
> > > 1. Deprecate strcpy() in favor of strscpy().  
> > 
> > This isn’t a comment “against” this patch, but something I’ve been
> > wondering recently and which raises a question about how to handle
> > strcpy’s deprecation in particular. There is still one scenario where
> > strcpy is useful: when GCC replaces it with its builtin, inline version...
> > 
> > Would it be worth introducing a macro for strcpy-from-constant-string,
> > which would check that GCC’s builtin is being used (when building with
> > GCC), and fall back to strscpy otherwise?  
> 
> How would you suggest it operate? A separate API, or something like the
> existing overloaded strcpy() macros in string.h?

The latter; in my mind the point is to simplify the thought process for
developers, so strscpy should be the “obvious” choice in all cases, even when
dealing with constant strings in hot paths. Something like

__FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t count)
{
	size_t dest_size = __builtin_object_size(dest, 0);
	size_t src_size = __builtin_object_size(src, 0);
	if (__builtin_constant_p(count) &&
	    __builtin_constant_p(src_size) &&
	    __builtin_constant_p(dest_size) &&
	    src_size <= count &&
	    src_size <= dest_size &&
	    src[src_size - 1] == '\0') {
		strcpy(dest, src);
		return src_size - 1;
	} else {
		return __strscpy(dest, src, count);
	}
}

with the current strscpy renamed to __strscpy. I imagine it’s not necessary
to tie this to FORTIFY — __OPTIMIZE__ should be sufficient, shouldn’t it?
Although building on top of the fortified strcpy is reassuring, and I might
be missing something. I’m also not sure how to deal with the backing strscpy:
weak symbol, or something else... At least there aren’t (yet) any
arch-specific implementations of strscpy to deal with, but obviously they’d
still need to be supportable.

In my tests, this all gets optimised away, and we end up with code such as

	strscpy(raead.type, "aead", sizeof(raead.type));

being compiled down to

	movl    $1684104545, 4(%rsp)

on x86-64, and non-constant code being compiled down to a direct __strscpy
call.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]