[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190707094041.6f7943a5@heffalump.sk2.org>
Date: Sun, 7 Jul 2019 09:40:41 +0200
From: Stephen Kitt <steve@....org>
To: Kees Cook <keescook@...omium.org>
Cc: Nitin Gote <nitin.r.gote@...el.com>, jannh@...gle.com,
kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().
On Sat, 6 Jul 2019 14:42:04 +0200, Stephen Kitt <steve@....org> 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.
And there are at least two baked-in assumptions here: src really is a
constant string (so the if should only trigger then), to avoid TOCTTOU races,
and there is only a single null byte at the end of src.
> 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
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
