[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8039728c-b41d-123c-e1ed-b35daac68fd3@rasmusvillemoes.dk>
Date: Thu, 26 Sep 2019 09:29:44 +0200
From: Rasmus Villemoes <linux@...musvillemoes.dk>
To: Stephen Kitt <steve@....org>, Andrew Morton <akpm@...ux-foundation.org>
Cc: Joe Perches <joe@...ches.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
linux-kernel@...r.kernel.org, Jonathan Corbet <corbet@....net>,
Kees Cook <keescook@...omium.org>, Nitin Gote <nitin.r.gote@...el.com>,
jannh@...gle.com, kernel-hardening@...ts.openwall.com,
Takashi Iwai <tiwai@...e.com>, Clemens Ladisch <clemens@...isch.de>,
alsa-devel@...a-project.org
Subject: Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms
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@...ches.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"); \
^
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.
