Skip to content

Commit 6a39e62

Browse files
daxtenstorvalds
authored andcommitted
lib: string.h: detect intra-object overflow in fortified string functions
Patch series "Fortify strscpy()", v7. This patch implements a fortified version of strscpy() enabled by setting CONFIG_FORTIFY_SOURCE=y. The new version ensures the following before calling vanilla strscpy(): 1. There is no read overflow because either size is smaller than src length or we shrink size to src length by calling fortified strnlen(). 2. There is no write overflow because we either failed during compilation or at runtime by checking that size is smaller than dest size. Note that, if src and dst size cannot be got, the patch defaults to call vanilla strscpy(). The patches adds the following: 1. Implement the fortified version of strscpy(). 2. Add a new LKDTM test to ensures the fortified version still returns the same value as the vanilla one while panic'ing when there is a write overflow. 3. Correct some typos in LKDTM related file. I based my modifications on top of two patches from Daniel Axtens which modify calls to __builtin_object_size, in fortified string functions, to ensure the true size of char * are returned and not the surrounding structure size. About performance, I measured the slow down of fortified strscpy(), using the vanilla one as baseline. The hardware I used is an Intel i3 2130 CPU clocked at 3.4 GHz. I ran "Linux 5.10.0-rc4+ SMP PREEMPT" inside qemu 3.10 with 4 CPU cores. The following code, called through LKDTM, was used as a benchmark: #define TIMES 10000 char *src; char dst[7]; int i; ktime_t begin; src = kstrdup("foobar", GFP_KERNEL); if (src == NULL) return; begin = ktime_get(); for (i = 0; i < TIMES; i++) strscpy(dst, src, strlen(src)); pr_info("%d fortified strscpy() tooks %lld", TIMES, ktime_get() - begin); begin = ktime_get(); for (i = 0; i < TIMES; i++) __real_strscpy(dst, src, strlen(src)); pr_info("%d vanilla strscpy() tooks %lld", TIMES, ktime_get() - begin); kfree(src); I called the above code 30 times to compute stats for each version (in ns, round to int): | version | mean | std | median | 95th | | --------- | ------- | ------ | ------- | ------- | | fortified | 245_069 | 54_657 | 216_230 | 331_122 | | vanilla | 172_501 | 70_281 | 143_539 | 219_553 | On average, fortified strscpy() is approximately 1.42 times slower than vanilla strscpy(). For the 95th percentile, the fortified version is about 1.50 times slower. So, clearly the stats are not in favor of fortified strscpy(). But, the fortified version loops the string twice (one in strnlen() and another in vanilla strscpy()) while the vanilla one only loops once. This can explain why fortified strscpy() is slower than the vanilla one. This patch (of 5): When the fortify feature was first introduced in commit 6974f0c ("include/linux/string.h: add the option of fortified string.h functions"), Daniel Micay observed: * It should be possible to optionally use __builtin_object_size(x, 1) for some functions (C strings) to detect intra-object overflows (like glibc's _FORTIFY_SOURCE=2), but for now this takes the conservative approach to avoid likely compatibility issues. This is a case that often cannot be caught by KASAN. Consider: struct foo { char a[10]; char b[10]; } void test() { char *msg; struct foo foo; msg = kmalloc(16, GFP_KERNEL); strcpy(msg, "Hello world!!"); // this copy overwrites foo.b strcpy(foo.a, msg); } The questionable copy overflows foo.a and writes to foo.b as well. It cannot be detected by KASAN. Currently it is also not detected by fortify, because strcpy considers __builtin_object_size(x, 0), which considers the size of the surrounding object (here, struct foo). However, if we switch the string functions over to use __builtin_object_size(x, 1), the compiler will measure the size of the closest surrounding subobject (here, foo.a), rather than the size of the surrounding object as a whole. See https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html for more info. Only do this for string functions: we cannot use it on things like memcpy, memmove, memcmp and memchr_inv due to code like this which purposefully operates on multiple structure members: (arch/x86/kernel/traps.c) /* * regs->sp points to the failing IRET frame on the * ESPFIX64 stack. Copy it to the entry stack. This fills * in gpregs->ss through gpregs->ip. * */ memmove(&gpregs->ip, (void *)regs->sp, 5*8); This change passes an allyesconfig on powerpc and x86, and an x86 kernel built with it survives running with syz-stress from syzkaller, so it seems safe so far. Link: https://lkml.kernel.org/r/[email protected] Link: https://lkml.kernel.org/r/[email protected] Signed-off-by: Daniel Axtens <[email protected]> Signed-off-by: Francis Laniel <[email protected]> Reviewed-by: Kees Cook <[email protected]> Cc: Daniel Micay <[email protected]> Signed-off-by: Andrew Morton <[email protected]> Signed-off-by: Linus Torvalds <[email protected]>
1 parent ff72daa commit 6a39e62

File tree

1 file changed

+16
-11
lines changed

1 file changed

+16
-11
lines changed

include/linux/string.h

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
292292

293293
__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
294294
{
295-
size_t p_size = __builtin_object_size(p, 0);
295+
size_t p_size = __builtin_object_size(p, 1);
296296
if (__builtin_constant_p(size) && p_size < size)
297297
__write_overflow();
298298
if (p_size < size)
@@ -302,7 +302,7 @@ __FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
302302

303303
__FORTIFY_INLINE char *strcat(char *p, const char *q)
304304
{
305-
size_t p_size = __builtin_object_size(p, 0);
305+
size_t p_size = __builtin_object_size(p, 1);
306306
if (p_size == (size_t)-1)
307307
return __underlying_strcat(p, q);
308308
if (strlcat(p, q, p_size) >= p_size)
@@ -313,7 +313,7 @@ __FORTIFY_INLINE char *strcat(char *p, const char *q)
313313
__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
314314
{
315315
__kernel_size_t ret;
316-
size_t p_size = __builtin_object_size(p, 0);
316+
size_t p_size = __builtin_object_size(p, 1);
317317

318318
/* Work around gcc excess stack consumption issue */
319319
if (p_size == (size_t)-1 ||
@@ -328,7 +328,7 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
328328
extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
329329
__FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
330330
{
331-
size_t p_size = __builtin_object_size(p, 0);
331+
size_t p_size = __builtin_object_size(p, 1);
332332
__kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
333333
if (p_size <= ret && maxlen != ret)
334334
fortify_panic(__func__);
@@ -340,8 +340,8 @@ extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
340340
__FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
341341
{
342342
size_t ret;
343-
size_t p_size = __builtin_object_size(p, 0);
344-
size_t q_size = __builtin_object_size(q, 0);
343+
size_t p_size = __builtin_object_size(p, 1);
344+
size_t q_size = __builtin_object_size(q, 1);
345345
if (p_size == (size_t)-1 && q_size == (size_t)-1)
346346
return __real_strlcpy(p, q, size);
347347
ret = strlen(q);
@@ -361,8 +361,8 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
361361
__FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
362362
{
363363
size_t p_len, copy_len;
364-
size_t p_size = __builtin_object_size(p, 0);
365-
size_t q_size = __builtin_object_size(q, 0);
364+
size_t p_size = __builtin_object_size(p, 1);
365+
size_t q_size = __builtin_object_size(q, 1);
366366
if (p_size == (size_t)-1 && q_size == (size_t)-1)
367367
return __underlying_strncat(p, q, count);
368368
p_len = strlen(p);
@@ -475,11 +475,16 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)
475475
/* defined after fortified strlen and memcpy to reuse them */
476476
__FORTIFY_INLINE char *strcpy(char *p, const char *q)
477477
{
478-
size_t p_size = __builtin_object_size(p, 0);
479-
size_t q_size = __builtin_object_size(q, 0);
478+
size_t p_size = __builtin_object_size(p, 1);
479+
size_t q_size = __builtin_object_size(q, 1);
480+
size_t size;
480481
if (p_size == (size_t)-1 && q_size == (size_t)-1)
481482
return __underlying_strcpy(p, q);
482-
memcpy(p, q, strlen(q) + 1);
483+
size = strlen(q) + 1;
484+
/* test here to use the more stringent object size */
485+
if (p_size < size)
486+
fortify_panic(__func__);
487+
memcpy(p, q, size);
483488
return p;
484489
}
485490

0 commit comments

Comments
 (0)