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

[Kees Cook]

On Sat, Jul 06, 2019 at 02:42:04PM +0200, Stephen Kitt wrote:
> On Tue, 2 Jul 2019 10:25:04 -0700, Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > > 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.

Thanks for the details! Yeah, that seems nice. I wonder if there is a
sensible way to combine these also with the stracpy*() proposal[1], so the
call in your example above could just be:

	stracpy(raead.type, "aead");

(It seems both proposals together would have the correct result...)

[1] 201907221031.8B87A9DE@keescook">https://lkml.kernel.org/r/201907221031.8B87A9DE@keescook

-- 
Kees Cook