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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190722230102.442137dc@heffalump.sk2.org>
Date:   Mon, 22 Jul 2019 23:01:02 +0200
From:   Stephen Kitt <steve@....org>
To:     Joe Perches <joe@...ches.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Nitin Gote <nitin.r.gote@...el.com>, jannh@...gle.com,
        kernel-hardening@...ts.openwall.com, corbet@....net,
        linux-kernel@...r.kernel.org,
        Rasmus Villemoes <rasmus.villemoes@...vas.dk>
Subject: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

On Mon, 22 Jul 2019 10:59:00 -0700, Joe Perches <joe@...ches.com> wrote:
> On Mon, 2019-07-22 at 10:50 -0700, Kees Cook wrote:
> > 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@...omium.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@...el.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.  
> > 
> > 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] https://lkml.kernel.org/r/201907221031.8B87A9DE@keescook  
> 
> Easy enough to do.

How about you submit your current patch set, and I follow up with the above
adapted to stracpy?

Regards,

Stephen

Content of type "application/pgp-signature" skipped