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"); \ ^