* [PATCH v2 1/2] string.h: detect intra-object overflow in fortified string functions
2020-01-20 4:54 [PATCH v2 0/2] FORTIFY_SOURCE: detect intra-object overflow in string functions Daniel Axtens
@ 2020-01-20 4:54 ` Daniel Axtens
2020-01-20 4:54 ` [PATCH v2 2/2] lkdtm: tests for FORTIFY_SOURCE Daniel Axtens
1 sibling, 0 replies; 3+ messages in thread
From: Daniel Axtens @ 2020-01-20 4:54 UTC (permalink / raw)
To: kernel-hardening, akpm, keescook
Cc: linux-kernel, Daniel Axtens, Daniel Micay
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.
Cc: Daniel Micay <danielmicay@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
include/linux/string.h | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index 3b8e8b12dd37..e7f34c3113f8 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -319,7 +319,7 @@ void __write_overflow(void) __compiletime_error("detected write beyond size of o
#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
__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)
@@ -329,7 +329,7 @@ __FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
__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 __builtin_strcat(p, q);
if (strlcat(p, q, p_size) >= p_size)
@@ -340,7 +340,7 @@ __FORTIFY_INLINE char *strcat(char *p, const char *q)
__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 ||
@@ -355,7 +355,7 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
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__);
@@ -367,8 +367,8 @@ extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
__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);
@@ -388,8 +388,8 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
__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 __builtin_strncat(p, q, count);
p_len = strlen(p);
@@ -502,11 +502,16 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)
/* 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 __builtin_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;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH v2 2/2] lkdtm: tests for FORTIFY_SOURCE
2020-01-20 4:54 [PATCH v2 0/2] FORTIFY_SOURCE: detect intra-object overflow in string functions Daniel Axtens
2020-01-20 4:54 ` [PATCH v2 1/2] string.h: detect intra-object overflow in fortified " Daniel Axtens
@ 2020-01-20 4:54 ` Daniel Axtens
1 sibling, 0 replies; 3+ messages in thread
From: Daniel Axtens @ 2020-01-20 4:54 UTC (permalink / raw)
To: kernel-hardening, akpm, keescook; +Cc: linux-kernel, Daniel Axtens
Add code to test both:
- runtime detection of the overrun of a structure. This covers the
__builtin_object_size(x, 0) case. This test is called FORTIFY_OBJECT.
- runtime detection of the overrun of a char array within a structure.
This covers the __builtin_object_size(x, 1) case which can be used
for some string functions. This test is called FORTIFY_SUBOBJECT.
Suggested-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
drivers/misc/lkdtm/bugs.c | 51 ++++++++++++++++++++++++++++++++++++++
drivers/misc/lkdtm/core.c | 2 ++
drivers/misc/lkdtm/lkdtm.h | 2 ++
3 files changed, 55 insertions(+)
diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
index a4fdad04809a..77bf01ce7e0c 100644
--- a/drivers/misc/lkdtm/bugs.c
+++ b/drivers/misc/lkdtm/bugs.c
@@ -11,6 +11,7 @@
#include <linux/sched/signal.h>
#include <linux/sched/task_stack.h>
#include <linux/uaccess.h>
+#include <linux/slab.h>
#ifdef CONFIG_X86_32
#include <asm/desc.h>
@@ -376,3 +377,53 @@ void lkdtm_DOUBLE_FAULT(void)
panic("tried to double fault but didn't die\n");
}
#endif
+
+void lkdtm_FORTIFY_OBJECT(void)
+{
+ struct target {
+ char a[10];
+ } target[2] = {};
+ int result;
+
+ /*
+ * Using volatile prevents the compiler from determining the value of
+ * 'size' at compile time. Without that, we would get a compile error
+ * rather than a runtime error.
+ */
+ volatile int size = 11;
+
+ pr_info("trying to read past the end of a struct\n");
+
+ result = memcmp(&target[0], &target[1], size);
+
+ /* Print result to prevent the code from being eliminated */
+ pr_err("FAIL: fortify did not catch an object overread!\n"
+ "\"%d\" was the memcmp result.\n", result);
+}
+
+void lkdtm_FORTIFY_SUBOBJECT(void)
+{
+ struct target {
+ char a[10];
+ char b[10];
+ } target;
+ char *src;
+
+ src = kmalloc(20, GFP_KERNEL);
+ strscpy(src, "over ten bytes", 20);
+
+ pr_info("trying to strcpy past the end of a member of a struct\n");
+
+ /*
+ * strncpy(target.a, src, 20); will hit a compile error because the
+ * compiler knows at build time that target.a < 20 bytes. Use strcpy()
+ * to force a runtime error.
+ */
+ strcpy(target.a, src);
+
+ /* Use target.a to prevent the code from being eliminated */
+ pr_err("FAIL: fortify did not catch an sub-object overrun!\n"
+ "\"%s\" was copied.\n", target.a);
+
+ kfree(src);
+}
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index ee0d6e721441..78d22a23b4f9 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -117,6 +117,8 @@ static const struct crashtype crashtypes[] = {
CRASHTYPE(STACK_GUARD_PAGE_TRAILING),
CRASHTYPE(UNSET_SMEP),
CRASHTYPE(UNALIGNED_LOAD_STORE_WRITE),
+ CRASHTYPE(FORTIFY_OBJECT),
+ CRASHTYPE(FORTIFY_SUBOBJECT),
CRASHTYPE(OVERWRITE_ALLOCATION),
CRASHTYPE(WRITE_AFTER_FREE),
CRASHTYPE(READ_AFTER_FREE),
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index c56d23e37643..13f13421dc19 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -31,6 +31,8 @@ void lkdtm_UNSET_SMEP(void);
#ifdef CONFIG_X86_32
void lkdtm_DOUBLE_FAULT(void);
#endif
+void lkdtm_FORTIFY_OBJECT(void);
+void lkdtm_FORTIFY_SUBOBJECT(void);
/* lkdtm_heap.c */
void __init lkdtm_heap_init(void);
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread