From: Francis Laniel <[email protected]>
Hi.
I hope your families, friends and yourselves are fine.
This patch set answers to this issue:
https://github.com/KSPP/linux/issues/46
I based my modifications on top of two patches from Daniel Axtens which modify
calls to __builtin_object_size to ensure the true size of char * are returned
and not the surrounding structure size.
To sum up, in my first patch I implemented a fortified version of strscpy.
This 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.
The second patch brings a new file in LKDTM driver to test this new version.
The test ensures the fortified version still returns the same value as the
vanilla one while panic'ing when there is a write overflow.
The third just corrects some typos in LKDTM related file.
If you see any problem or way to improve the code, feel free to share it.
Best regards.
Daniel Axtens (2):
string.h: detect intra-object overflow in fortified string functions
lkdtm: tests for FORTIFY_SOURCE
Francis Laniel (3):
string.h: Add FORTIFY coverage for strscpy()
Add new file in LKDTM to test fortified strscpy.
Correct wrong filenames in comment.
drivers/misc/lkdtm/Makefile | 1 +
drivers/misc/lkdtm/bugs.c | 50 +++++++++++++++
drivers/misc/lkdtm/core.c | 3 +
drivers/misc/lkdtm/fortify.c | 82 +++++++++++++++++++++++++
drivers/misc/lkdtm/lkdtm.h | 19 +++---
include/linux/string.h | 75 ++++++++++++++++++----
tools/testing/selftests/lkdtm/tests.txt | 1 +
7 files changed, 213 insertions(+), 18 deletions(-)
create mode 100644 drivers/misc/lkdtm/fortify.c
--
2.20.1
From: Francis Laniel <[email protected]>
In lkdtm.h, files targeted in comments are named "lkdtm_file.c" while there are
named "file.c" in directory.
Signed-off-by: Francis Laniel <[email protected]>
Acked-by: Kees Cook <[email protected]>
---
drivers/misc/lkdtm/lkdtm.h | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index 138f06254b61..6aa6d6a1a839 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -6,7 +6,7 @@
#include <linux/kernel.h>
-/* lkdtm_bugs.c */
+/* bugs.c */
void __init lkdtm_bugs_init(int *recur_param);
void lkdtm_PANIC(void);
void lkdtm_BUG(void);
@@ -35,7 +35,7 @@ void lkdtm_CORRUPT_PAC(void);
void lkdtm_FORTIFY_OBJECT(void);
void lkdtm_FORTIFY_SUBOBJECT(void);
-/* lkdtm_heap.c */
+/* heap.c */
void __init lkdtm_heap_init(void);
void __exit lkdtm_heap_exit(void);
void lkdtm_OVERWRITE_ALLOCATION(void);
@@ -47,7 +47,7 @@ void lkdtm_SLAB_FREE_DOUBLE(void);
void lkdtm_SLAB_FREE_CROSS(void);
void lkdtm_SLAB_FREE_PAGE(void);
-/* lkdtm_perms.c */
+/* perms.c */
void __init lkdtm_perms_init(void);
void lkdtm_WRITE_RO(void);
void lkdtm_WRITE_RO_AFTER_INIT(void);
@@ -62,7 +62,7 @@ void lkdtm_EXEC_NULL(void);
void lkdtm_ACCESS_USERSPACE(void);
void lkdtm_ACCESS_NULL(void);
-/* lkdtm_refcount.c */
+/* refcount.c */
void lkdtm_REFCOUNT_INC_OVERFLOW(void);
void lkdtm_REFCOUNT_ADD_OVERFLOW(void);
void lkdtm_REFCOUNT_INC_NOT_ZERO_OVERFLOW(void);
@@ -83,10 +83,10 @@ void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void);
void lkdtm_REFCOUNT_TIMING(void);
void lkdtm_ATOMIC_TIMING(void);
-/* lkdtm_rodata.c */
+/* rodata.c */
void lkdtm_rodata_do_nothing(void);
-/* lkdtm_usercopy.c */
+/* usercopy.c */
void __init lkdtm_usercopy_init(void);
void __exit lkdtm_usercopy_exit(void);
void lkdtm_USERCOPY_HEAP_SIZE_TO(void);
@@ -98,7 +98,7 @@ void lkdtm_USERCOPY_STACK_FRAME_FROM(void);
void lkdtm_USERCOPY_STACK_BEYOND(void);
void lkdtm_USERCOPY_KERNEL(void);
-/* lkdtm_stackleak.c */
+/* stackleak.c */
void lkdtm_STACKLEAK_ERASING(void);
/* cfi.c */
--
2.20.1
From: Francis Laniel <[email protected]>
The fortified version of strscpy ensures the following before vanilla strscpy
is called:
1. There is no read overflow because we 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.
Signed-off-by: Francis Laniel <[email protected]>
Acked-by: Kees Cook <[email protected]>
---
include/linux/string.h | 48 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index 46e91d684c47..1cd63a8a23ab 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -6,6 +6,7 @@
#include <linux/compiler.h> /* for inline */
#include <linux/types.h> /* for size_t */
#include <linux/stddef.h> /* for NULL */
+#include <linux/errno.h> /* for E2BIG */
#include <stdarg.h>
#include <uapi/linux/string.h>
@@ -357,6 +358,53 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
return ret;
}
+/* defined after fortified strnlen to reuse it */
+extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
+__FORTIFY_INLINE ssize_t strscpy(char *p, const char *q, size_t size)
+{
+ size_t len;
+ /* Use string size rather than possible enclosing struct size. */
+ size_t p_size = __builtin_object_size(p, 1);
+ size_t q_size = __builtin_object_size(q, 1);
+
+ /* If we cannot get size of p and q default to call strscpy. */
+ if (p_size == (size_t) -1 && q_size == (size_t) -1)
+ return __real_strscpy(p, q, size);
+
+ /*
+ * If size can be known at compile time and is greater than
+ * p_size, generate a compile time write overflow error.
+ */
+ if (__builtin_constant_p(size) && size > p_size)
+ __write_overflow();
+
+ /*
+ * This call protects from read overflow, because len will default to q
+ * length if it smaller than size.
+ */
+ len = strnlen(q, size);
+ /*
+ * If len equals size, we will copy only size bytes which leads to
+ * -E2BIG being returned.
+ * Otherwise we will copy len + 1 because of the final '\O'.
+ */
+ len = len == size ? size : len + 1;
+
+ /*
+ * Generate a runtime write overflow error if len is greater than
+ * p_size.
+ */
+ if (len > p_size)
+ fortify_panic(__func__);
+
+ /*
+ * We can now safely call vanilla strscpy because we are protected from:
+ * 1. Read overflow thanks to call to strnlen().
+ * 2. Write overflow thanks to above ifs.
+ */
+ return __real_strscpy(p, q, len);
+}
+
/* defined after fortified strlen and strnlen to reuse them */
__FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
{
--
2.20.1
From: Daniel Axtens <[email protected]>
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 <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Signed-off-by: Daniel Axtens <[email protected]>
---
drivers/misc/lkdtm/bugs.c | 50 ++++++++++++++++++++++++++++++++++++++
drivers/misc/lkdtm/core.c | 2 ++
drivers/misc/lkdtm/lkdtm.h | 2 ++
3 files changed, 54 insertions(+)
diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
index a0675d4154d2..110f5a8538e9 100644
--- a/drivers/misc/lkdtm/bugs.c
+++ b/drivers/misc/lkdtm/bugs.c
@@ -482,3 +482,53 @@ noinline void lkdtm_CORRUPT_PAC(void)
pr_err("XFAIL: this test is arm64-only\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 97803f213d9d..b8c51a633fcc 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -117,6 +117,8 @@ static const struct crashtype crashtypes[] = {
CRASHTYPE(UNSET_SMEP),
CRASHTYPE(CORRUPT_PAC),
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 6dec4c9b442f..49e6b945feb7 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -32,6 +32,8 @@ void lkdtm_STACK_GUARD_PAGE_TRAILING(void);
void lkdtm_UNSET_SMEP(void);
void lkdtm_DOUBLE_FAULT(void);
void lkdtm_CORRUPT_PAC(void);
+void lkdtm_FORTIFY_OBJECT(void);
+void lkdtm_FORTIFY_SUBOBJECT(void);
/* lkdtm_heap.c */
void __init lkdtm_heap_init(void);
--
2.20.1