All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: akpm@linux-foundation.org, danielmicay@gmail.com, dja@axtens.net,
	keescook@chromium.org, laniel_francis@privacyrequired.com,
	linux-mm@kvack.org, mm-commits@vger.kernel.org,
	torvalds@linux-foundation.org
Subject: [patch 26/95] lib: string.h: detect intra-object overflow in fortified string functions
Date: Tue, 15 Dec 2020 20:43:44 -0800	[thread overview]
Message-ID: <20201216044344.Ht0m_MRFn%akpm@linux-foundation.org> (raw)
In-Reply-To: <20201215204156.f05ec694b907845bcfab5c44@linux-foundation.org>

From: Daniel Axtens <dja@axtens.net>
Subject: 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 6974f0c4555e
("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/20201122162451.27551-1-laniel_francis@privacyrequired.com
Link: https://lkml.kernel.org/r/20201122162451.27551-2-laniel_francis@privacyrequired.com
Signed-off-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Francis Laniel <laniel_francis@privacyrequired.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Daniel Micay <danielmicay@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/string.h |   27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

--- a/include/linux/string.h~stringh-detect-intra-object-overflow-in-fortified-string-functions
+++ a/include/linux/string.h
@@ -292,7 +292,7 @@ extern char *__underlying_strncpy(char *
 
 __FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
 {
-	size_t p_size = __builtin_object_size(p, 0);
+	size_t p_size = __builtin_object_size(p, 1);
 	if (__builtin_constant_p(size) && p_size < size)
 		__write_overflow();
 	if (p_size < size)
@@ -302,7 +302,7 @@ __FORTIFY_INLINE char *strncpy(char *p,
 
 __FORTIFY_INLINE char *strcat(char *p, const char *q)
 {
-	size_t p_size = __builtin_object_size(p, 0);
+	size_t p_size = __builtin_object_size(p, 1);
 	if (p_size == (size_t)-1)
 		return __underlying_strcat(p, q);
 	if (strlcat(p, q, p_size) >= p_size)
@@ -313,7 +313,7 @@ __FORTIFY_INLINE char *strcat(char *p, c
 __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
 {
 	__kernel_size_t ret;
-	size_t p_size = __builtin_object_size(p, 0);
+	size_t p_size = __builtin_object_size(p, 1);
 
 	/* Work around gcc excess stack consumption issue */
 	if (p_size == (size_t)-1 ||
@@ -328,7 +328,7 @@ __FORTIFY_INLINE __kernel_size_t strlen(
 extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
 __FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
 {
-	size_t p_size = __builtin_object_size(p, 0);
+	size_t p_size = __builtin_object_size(p, 1);
 	__kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
 	if (p_size <= ret && maxlen != ret)
 		fortify_panic(__func__);
@@ -340,8 +340,8 @@ extern size_t __real_strlcpy(char *, con
 __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
 {
 	size_t ret;
-	size_t p_size = __builtin_object_size(p, 0);
-	size_t q_size = __builtin_object_size(q, 0);
+	size_t p_size = __builtin_object_size(p, 1);
+	size_t q_size = __builtin_object_size(q, 1);
 	if (p_size == (size_t)-1 && q_size == (size_t)-1)
 		return __real_strlcpy(p, q, size);
 	ret = strlen(q);
@@ -361,8 +361,8 @@ __FORTIFY_INLINE size_t strlcpy(char *p,
 __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
 {
 	size_t p_len, copy_len;
-	size_t p_size = __builtin_object_size(p, 0);
-	size_t q_size = __builtin_object_size(q, 0);
+	size_t p_size = __builtin_object_size(p, 1);
+	size_t q_size = __builtin_object_size(q, 1);
 	if (p_size == (size_t)-1 && q_size == (size_t)-1)
 		return __underlying_strncat(p, q, count);
 	p_len = strlen(p);
@@ -475,11 +475,16 @@ __FORTIFY_INLINE void *kmemdup(const voi
 /* defined after fortified strlen and memcpy to reuse them */
 __FORTIFY_INLINE char *strcpy(char *p, const char *q)
 {
-	size_t p_size = __builtin_object_size(p, 0);
-	size_t q_size = __builtin_object_size(q, 0);
+	size_t p_size = __builtin_object_size(p, 1);
+	size_t q_size = __builtin_object_size(q, 1);
+	size_t size;
 	if (p_size == (size_t)-1 && q_size == (size_t)-1)
 		return __underlying_strcpy(p, q);
-	memcpy(p, q, strlen(q) + 1);
+	size = strlen(q) + 1;
+	/* test here to use the more stringent object size */
+	if (p_size < size)
+		fortify_panic(__func__);
+	memcpy(p, q, size);
 	return p;
 }
 
_

  parent reply	other threads:[~2020-12-16  4:44 UTC|newest]

Thread overview: 113+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-16  4:41 incoming Andrew Morton
2020-12-16  4:42 ` [patch 01/95] mm: fix a race on nr_swap_pages Andrew Morton
2020-12-16  4:42 ` [patch 02/95] mm/memory_hotplug: quieting offline operation Andrew Morton
2020-12-16  4:42 ` [patch 03/95] alpha: replace bogus in_interrupt() Andrew Morton
2020-12-16  4:42 ` [patch 04/95] procfs: delete duplicated words + other fixes Andrew Morton
2020-12-16  4:42 ` [patch 05/95] proc: provide details on indirect branch speculation Andrew Morton
2020-12-16  4:42 ` [patch 06/95] proc: fix lookup in /proc/net subdirectories after setns(2) Andrew Morton
2020-12-16  4:42 ` [patch 07/95] fs/proc: make pde_get() return nothing Andrew Morton
2020-12-16  4:42 ` [patch 08/95] asm-generic: force inlining of get_order() to work around gcc10 poor decision Andrew Morton
2020-12-16  4:42 ` [patch 09/95] kernel.h: split out mathematical helpers Andrew Morton
2020-12-16  4:42 ` [patch 10/95] kernel/acct.c: use #elif instead of #end and #elif Andrew Morton
2020-12-16  4:42 ` [patch 11/95] include/linux/bitmap.h: convert bitmap_empty() / bitmap_full() to return boolean Andrew Morton
2020-12-16  4:42 ` [patch 12/95] bitmap: remove unused function declaration Andrew Morton
2020-12-16  4:43 ` [patch 13/95] lib/test_free_pages.c: add basic progress indicators Andrew Morton
2020-12-16  4:43 ` [patch 14/95] lib/stackdepot.c: replace one-element array with flexible-array member Andrew Morton
2020-12-16  4:43 ` [patch 15/95] lib/stackdepot.c: use flex_array_size() helper in memcpy() Andrew Morton
2020-12-16  4:43 ` [patch 16/95] lib/stackdepot.c: use array_size() helper in jhash2() Andrew Morton
2020-12-16  4:43 ` [patch 17/95] lib/test_lockup.c: minimum fix to get it compiled on PREEMPT_RT Andrew Morton
2020-12-16  4:43 ` [patch 18/95] lib/list_kunit: follow new file name convention for KUnit tests Andrew Morton
2020-12-16  6:02   ` Linus Torvalds
2020-12-16  6:02     ` Linus Torvalds
2020-12-16  6:53     ` David Gow
2020-12-16  6:53       ` David Gow
2020-12-16  7:01       ` Linus Torvalds
2020-12-16  7:01         ` Linus Torvalds
2020-12-16 10:41       ` Andy Shevchenko
2020-12-17  9:21         ` David Gow
2020-12-17  9:21           ` David Gow
2020-12-17 12:02           ` Andy Shevchenko
2020-12-16  4:43 ` [patch 19/95] lib/linear_ranges_kunit: " Andrew Morton
2020-12-16  4:43 ` [patch 20/95] lib/bits_kunit: " Andrew Morton
2020-12-16  4:43 ` [patch 21/95] lib/cmdline: fix get_option() for strings starting with hyphen Andrew Morton
2020-12-16  4:43 ` [patch 22/95] lib/cmdline: allow NULL to be an output for get_option() Andrew Morton
2020-12-16  4:43 ` [patch 23/95] lib/cmdline_kunit: add a new test suite for cmdline API Andrew Morton
2020-12-16  4:43 ` [patch 24/95] ilog2: improve ilog2 for constant arguments Andrew Morton
2020-12-16  4:43 ` [patch 25/95] lib/string: remove unnecessary #undefs Andrew Morton
2020-12-16  4:43 ` Andrew Morton [this message]
2020-12-16  4:43 ` [patch 27/95] lkdtm: tests for FORTIFY_SOURCE Andrew Morton
2020-12-16  4:43 ` [patch 28/95] string.h: add FORTIFY coverage for strscpy() Andrew Morton
2020-12-16  7:26   ` Linus Torvalds
2020-12-16  7:26     ` Linus Torvalds
2020-12-16  4:43 ` [patch 29/95] drivers/misc/lkdtm: add new file in LKDTM to test fortified strscpy Andrew Morton
2020-12-16  4:43 ` [patch 30/95] drivers/misc/lkdtm/lkdtm.h: correct wrong filenames in comment Andrew Morton
2020-12-16  4:44 ` [patch 31/95] lib: cleanup kstrto*() usage Andrew Morton
2020-12-16  4:44 ` [patch 32/95] lib/lz4: explicitly support in-place decompression Andrew Morton
2020-12-16  4:44 ` [patch 33/95] bitops: introduce the for_each_set_clump macro Andrew Morton
2020-12-16  6:14   ` Linus Torvalds
2020-12-16  6:14     ` Linus Torvalds
2020-12-16  4:44 ` [patch 34/95] lib/test_bitmap.c: add for_each_set_clump test cases Andrew Morton
2020-12-16  4:44 ` [patch 35/95] gpio: thunderx: utilize for_each_set_clump macro Andrew Morton
2020-12-16  4:44 ` [patch 36/95] gpio: xilinx: utilize generic bitmap_get_value and _set_value Andrew Morton
2020-12-16  4:44 ` [patch 37/95] checkpatch: add new exception to repeated word check Andrew Morton
2020-12-16  4:44 ` [patch 38/95] checkpatch: fix false positives in REPEATED_WORD warning Andrew Morton
2020-12-16  4:44 ` [patch 39/95] checkpatch: ignore generated CamelCase defines and enum values Andrew Morton
2020-12-16  4:44 ` [patch 40/95] checkpatch: prefer static const declarations Andrew Morton
2020-12-16  4:44 ` [patch 41/95] checkpatch: allow --fix removal of unnecessary break statements Andrew Morton
2020-12-16  4:44 ` [patch 42/95] checkpatch: extend attributes check to handle more patterns Andrew Morton
2020-12-16  4:44 ` [patch 43/95] checkpatch: add a fixer for missing newline at eof Andrew Morton
2020-12-16  4:44 ` [patch 44/95] checkpatch: update __attribute__((section("name"))) quote removal Andrew Morton
2020-12-16  4:44 ` [patch 45/95] checkpatch: add fix option for GERRIT_CHANGE_ID Andrew Morton
2020-12-16  4:44 ` [patch 46/95] checkpatch: add __alias and __weak to suggested __attribute__ conversions Andrew Morton
2020-12-16  4:44 ` [patch 47/95] checkpatch: improve email parsing Andrew Morton
2020-12-16  4:44 ` [patch 48/95] checkpatch: fix spelling errors and remove repeated word Andrew Morton
2020-12-16  4:44 ` [patch 49/95] checkpatch: avoid COMMIT_LOG_LONG_LINE warning for signature tags Andrew Morton
2020-12-16  4:45 ` [patch 50/95] checkpatch: fix unescaped left brace Andrew Morton
2020-12-16  4:45 ` [patch 51/95] checkpatch: add fix option for ASSIGNMENT_CONTINUATIONS Andrew Morton
2020-12-16  4:45 ` [patch 52/95] checkpatch: add fix option for LOGICAL_CONTINUATIONS Andrew Morton
2020-12-16  4:45 ` [patch 53/95] checkpatch: add fix and improve warning msg for non-standard signature Andrew Morton
2020-12-16  4:45 ` [patch 54/95] checkpatch: add warning for unnecessary use of %h[xudi] and %hh[xudi] Andrew Morton
2020-12-16  4:45 ` [patch 55/95] checkpatch: add warning for lines starting with a '#' in commit log Andrew Morton
2020-12-16  4:45 ` [patch 56/95] checkpatch: fix TYPO_SPELLING check for words with apostrophe Andrew Morton
2020-12-16  4:45 ` [patch 57/95] checkpatch: add printk_once and printk_ratelimit to prefer pr_<level> warning Andrew Morton
2020-12-16  4:45 ` [patch 58/95] fs/nilfs2: remove some unused macros to tame gcc Andrew Morton
2020-12-16  4:45 ` [patch 59/95] kdump: append uts_namespace.name offset to VMCOREINFO Andrew Morton
2020-12-16  4:45 ` [patch 60/95] rapidio: remove unused rio_get_asm() and rio_get_device() Andrew Morton
2020-12-16  4:45 ` [patch 61/95] gcov: remove support for GCC < 4.9 Andrew Morton
2020-12-16  4:45 ` [patch 62/95] gcov: fix kernel-doc markup issue Andrew Morton
2020-12-16  4:45 ` [patch 63/95] bfs: don't use WARNING: string when it's just info Andrew Morton
2020-12-16  4:45 ` [patch 64/95] relay: remove unused buf_mapped and buf_unmapped callbacks Andrew Morton
2020-12-16  4:45 ` [patch 65/95] relay: require non-NULL callbacks in relay_open() Andrew Morton
2020-12-16  4:45 ` [patch 66/95] relay: make create_buf_file and remove_buf_file callbacks mandatory Andrew Morton
2020-12-16  4:45 ` [patch 67/95] relay: allow the use of const callback structs Andrew Morton
2020-12-16  4:46 ` [patch 68/95] drm/i915: make relay callbacks const Andrew Morton
2020-12-16  4:46 ` [patch 69/95] ath10k: " Andrew Morton
2020-12-16  4:46 ` [patch 70/95] ath11k: " Andrew Morton
2020-12-16  4:46 ` [patch 71/95] ath9k: " Andrew Morton
2020-12-16  4:46 ` [patch 72/95] blktrace: " Andrew Morton
2020-12-16  4:46 ` [patch 73/95] kernel/resource.c: fix kernel-doc markups Andrew Morton
2020-12-16  4:46 ` [patch 74/95] ubsan: remove redundant -Wno-maybe-uninitialized Andrew Morton
2020-12-16  4:46 ` [patch 75/95] ubsan: move cc-option tests into Kconfig Andrew Morton
2020-12-16  4:46 ` [patch 76/95] ubsan: disable object-size sanitizer under GCC Andrew Morton
2020-12-16  4:46 ` [patch 77/95] ubsan: disable UBSAN_TRAP for all*config Andrew Morton
2020-12-16  4:46 ` [patch 78/95] ubsan: enable for all*config builds Andrew Morton
2020-12-16  4:46 ` [patch 79/95] ubsan: remove UBSAN_MISC in favor of individual options Andrew Morton
2020-12-16  4:46 ` [patch 80/95] ubsan: expand tests and reporting Andrew Morton
2020-12-16  4:46 ` [patch 81/95] kcov: don't instrument with UBSAN Andrew Morton
2020-12-16  4:46 ` [patch 82/95] lib/ubsan.c: mark type_check_kinds with static keyword Andrew Morton
2020-12-16  4:46 ` [patch 83/95] reboot: refactor and comment the cpu selection code Andrew Morton
2020-12-16  4:46 ` [patch 84/95] reboot: allow to specify reboot mode via sysfs Andrew Morton
2020-12-16  4:47 ` [patch 85/95] reboot: remove cf9_safe from allowed types and rename cf9_force Andrew Morton
2020-12-16  4:47 ` [patch 86/95] reboot: allow to override reboot type if quirks are found Andrew Morton
2020-12-16  4:47 ` [patch 87/95] reboot: hide from sysfs not applicable settings Andrew Morton
2020-12-16  4:47 ` [patch 88/95] fault-injection: handle EI_ETYPE_TRUE Andrew Morton
2020-12-16  4:47 ` [patch 89/95] lib/lzo/lzo1x_compress.c: make lzogeneric1x_1_compress() static Andrew Morton
2020-12-16  4:47 ` [patch 90/95] apparmor: remove duplicate macro list_entry_is_head() Andrew Morton
2020-12-16  4:47 ` [patch 91/95] mm: unexport follow_pte_pmd Andrew Morton
2020-12-16  4:47 ` [patch 92/95] mm: simplify follow_pte{,pmd} Andrew Morton
2020-12-16  4:47 ` [patch 93/95] mm: fix some spelling mistakes in comments Andrew Morton
2020-12-16  4:47 ` [patch 94/95] mmap locking API: don't check locking if the mm isn't live yet Andrew Morton
2020-12-16  5:07   ` Jann Horn
2020-12-16  5:07     ` Jann Horn
2020-12-16 18:08     ` Jason Gunthorpe
2020-12-16  4:47 ` [patch 95/95] mm/gup: assert that the mmap lock is held in __get_user_pages() Andrew Morton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201216044344.Ht0m_MRFn%akpm@linux-foundation.org \
    --to=akpm@linux-foundation.org \
    --cc=danielmicay@gmail.com \
    --cc=dja@axtens.net \
    --cc=keescook@chromium.org \
    --cc=laniel_francis@privacyrequired.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mm-commits@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.