Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: Stephen Kitt <steve@sk2.org>, Andrew Morton <akpm@linux-foundation.org>
Cc: Joe Perches <joe@perches.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-kernel@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
	Kees Cook <keescook@chromium.org>,
	Nitin Gote <nitin.r.gote@intel.com>,
	jannh@google.com, kernel-hardening@lists.openwall.com,
	Takashi Iwai <tiwai@suse.com>,
	Clemens Ladisch <clemens@ladisch.de>,
	alsa-devel@alsa-project.org
Subject: Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms
Date: Thu, 26 Sep 2019 09:29:44 +0200	[thread overview]
Message-ID: <8039728c-b41d-123c-e1ed-b35daac68fd3@rasmusvillemoes.dk> (raw)
In-Reply-To: <c0c2b8f6ac9f257b102b5a1a4b4dc949@sk2.org>

On 26/09/2019 02.01, Stephen Kitt wrote:
> Le 25/09/2019 23:50, Andrew Morton a écrit :
>> On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com> wrote:
>>
>>> Several uses of strlcpy and strscpy have had defects because the
>>> last argument of each function is misused or typoed.
>>>
>>> Add macro mechanisms to avoid this defect.
>>>
>>> stracpy (copy a string to a string array) must have a string
>>> array as the first argument (dest) and uses sizeof(dest) as the
>>> count of bytes to copy.
>>>
>>> These mechanisms verify that the dest argument is an array of
>>> char or other compatible types like u8 or s8 or equivalent.
>>>
>>> A BUILD_BUG is emitted when the type of dest is not compatible.
>>>
>>
>> I'm still reluctant to merge this because we don't have code in -next
>> which *uses* it.  You did have a patch for that against v1, I believe?
>> Please dust it off and send it along?
> 
> Joe had a Coccinelle script to mass-convert strlcpy and strscpy.
> Here's a different patch which converts some of ALSA's strcpy calls to
> stracpy:

Please don't. At least not for the cases where the source is a string
literal - that just gives worse code generation (because gcc doesn't
know anything about strscpy or strlcpy), and while a run-time (silent)
truncation is better than a run-time buffer overflow, wouldn't it be
even better with a build time error?

Something like

/** static_strcpy - run-time version of static initialization
 *
 * @d: destination array, must be array of char of known size
 * @s: source, must be a string literal
 *
 * This is a simple wrapper for strcpy(d, s), which checks at build-time
that the strcpy() won't overflow. In most cases (for short strings), gcc
won't even emit a call to strcpy but rather just do a few immediate
stores into the destination, e.g.

   0:   c7 07 66 6f 6f 00       movl   $0x6f6f66,(%rdi)

 * for strcpy(d->name, "foo").
 */

#define static_strcpy(d, s) ({ \
  static_assert(__same_type(d, char[]), "destination must be char array"); \
  static_assert(__same_type(s, const char[], "source must be a string
literal"); \
  static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
  strcpy(d, s); \
})

The "" s "" trick guarantees that s is a string literal - it probably
doesn't give a very nice error message, but that's why I added the
redundant type comparison to a const char[] which should hopefully give
a better clue.

Rasmus

PS: Yes, we already have a fortified strcpy(), but for some reason it
doesn't catch the common case where we're populating a char[] member of
some struct. So this

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e78017a3e1bd..3b0c5ae9f49e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3420,3 +3420,14 @@ int sscanf(const char *buf, const char *fmt, ...)
        return i;
 }
 EXPORT_SYMBOL(sscanf);
+
+struct s { char name[4]; };
+char buf[4];
+void f3(void)
+{
+       strcpy(buf, "long");
+}
+void f4(struct s *s)
+{
+       strcpy(s->name, "long");
+}

with CONFIG_FORTIFY_SOURCE=y complains about f3(), but f4() is just fine...

PPS: A quick test of static_strcpy():

// ss.c
#include <errno.h>
#include <string.h>
#include <assert.h>

#define __same_type(x, y) __builtin_types_compatible_p(typeof(x), typeof(y))

#define static_strcpy(d, s) ({						\
  static_assert(__same_type(d, char[]), "destination must be char array"); \
  static_assert(__same_type(s, const char[]), "source must be a string
literal"); \
  static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
  strcpy(d, s); \
})

struct s { char name[4]; };

void f0(struct s *s) { static_strcpy(s->name, "foo"); }
#if 1
void f1(struct s *s) { static_strcpy(s->name, strerror(EIO)); }
void f2(struct s *s) { static_strcpy(s->name, "long"); }
void f3(char *d) { static_strcpy(d, "foo"); }
void f4(char *d) { static_strcpy(d, strerror(EIO)); }
#endif

// gcc -Wall -O2 -o ss.o -c ss.c
In file included from ss.c:3:0:
ss.c: In function ‘f1’:
ss.c:9:3: error: static assertion failed: "source must be a string literal"
   static_assert(__same_type(s, const char[]), "source must be a string
literal"); \
   ^
ss.c:18:24: note: in expansion of macro ‘static_strcpy’
 void f1(struct s *s) { static_strcpy(s->name, strerror(EIO)); }
                        ^~~~~~~~~~~~~
ss.c:18:47: error: expected ‘)’ before ‘strerror’
 void f1(struct s *s) { static_strcpy(s->name, strerror(EIO)); }
                                               ^
ss.c:10:40: note: in definition of macro ‘static_strcpy’
   static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
                                        ^
In file included from ss.c:3:0:
ss.c: In function ‘f2’:
ss.c:10:3: error: static assertion failed: "source does not fit in
destination"
   static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
   ^
ss.c:19:24: note: in expansion of macro ‘static_strcpy’
 void f2(struct s *s) { static_strcpy(s->name, "long"); }
                        ^~~~~~~~~~~~~
ss.c: In function ‘f3’:
ss.c:8:3: error: static assertion failed: "destination must be char array"
   static_assert(__same_type(d, char[]), "destination must be char
array"); \
   ^
ss.c:20:20: note: in expansion of macro ‘static_strcpy’
 void f3(char *d) { static_strcpy(d, "foo"); }
                    ^~~~~~~~~~~~~
ss.c: In function ‘f4’:
ss.c:8:3: error: static assertion failed: "destination must be char array"
   static_assert(__same_type(d, char[]), "destination must be char
array"); \
   ^
ss.c:21:20: note: in expansion of macro ‘static_strcpy’
 void f4(char *d) { static_strcpy(d, strerror(EIO)); }
                    ^~~~~~~~~~~~~
ss.c:9:3: error: static assertion failed: "source must be a string literal"
   static_assert(__same_type(s, const char[]), "source must be a string
literal"); \
   ^
ss.c:21:20: note: in expansion of macro ‘static_strcpy’
 void f4(char *d) { static_strcpy(d, strerror(EIO)); }
                    ^~~~~~~~~~~~~
ss.c:21:37: error: expected ‘)’ before ‘strerror’
 void f4(char *d) { static_strcpy(d, strerror(EIO)); }
                                     ^
ss.c:10:40: note: in definition of macro ‘static_strcpy’
   static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
                                        ^

