Skip to contentSkip to conversation

fortify: Add KUnit tests for runtime overflows

PATCH New Kees Cook 1 participant 6 messages Feb 17, 2024

Subsystems (6)
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

v3 cover fortify: Add KUnit tests for runtime overflows

Hi,

This series is the rest of the v2 series that was half landed last year, and finally introduces KUnit runtime testing of the CONFIG_FORTIFY_SOURCE APIs. Additionally FORTIFY failure messages are improved to give more context about read/write and sizes.

-Kees

v3
- rebase (goodbye strlcpy)
- avoid extra macros for replacing fortify_panic() (nick)
- generally clean up macro usage
- avoid build warnings when testing known overflow conditions
v2 https://lore.kernel.org/all/[email protected]/
v1 https://lore.kernel.org/lkml/[email protected]/

Kees Cook (5):
fortify: Split reporting and avoid passing string pointer
fortify: Allow KUnit test to build without FORTIFY
fortify: Provide KUnit counters for failure testing
fortify: Add KUnit tests for runtime overflows
fortify: Improve buffer overflow reporting

 arch/arm/boot/compressed/misc.c |   2 +-
 arch/x86/boot/compressed/misc.c |   2 +-
 include/linux/fortify-string.h  | 100 +++--
 lib/Kconfig.debug               |   2 +-
 lib/Makefile                    |   1 +
 lib/fortify_kunit.c             | 662 +++++++++++++++++++++++++++++++-
 lib/string_helpers.c            |  26 +-
 tools/objtool/noreturns.h       |   2 +-
 8 files changed, 761 insertions(+), 36 deletions(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH v3 1/5 fortify: Split reporting and avoid passing string pointer

In preparation for KUnit testing and further improvements in fortify failure reporting, split out the report and encode the function and access failure (read or write overflow) into a single u8 argument. This mainly ends up saving a tiny bit of space in the data segment. For a defconfig with FORTIFY_SOURCE enabled:

$ size gcc/vmlinux.before gcc/vmlinux.after
   text  	  data     bss     dec    	    hex filename
26132309        9760658 2195460 38088427        2452eeb gcc/vmlinux.before
26132386        9748382 2195460 38076228        244ff44 gcc/vmlinux.after

---
Cc: [email protected]
Cc: Andy Shevchenko <[email protected]>
Cc: Cezary Rojewski <[email protected]>
Cc: Puyou Lu <[email protected]>
Cc: Mark Brown <[email protected]>

---
 arch/arm/boot/compressed/misc.c |  2 +-
 arch/x86/boot/compressed/misc.c |  2 +-
 include/linux/fortify-string.h  | 81 ++++++++++++++++++++++++---------
 lib/string_helpers.c            | 23 ++++++++--
 tools/objtool/noreturns.h       |  2 +-
 5 files changed, 83 insertions(+), 27 deletions(-)
arch/arm/boot/compressed/misc.c +1 -1
index 6b4baa6a9a50..d93e2e466f6a 100644
@@ -154,7 +154,7 @@ decompress_kernel(unsigned long output_start, unsigned long free_mem_ptr_p,
 		putstr(" done, booting the kernel.\n");
 }
 
-void fortify_panic(const char *name)
+void __fortify_panic(const u8 reason)
 {
 	error("detected buffer overflow");
 }
arch/x86/boot/compressed/misc.c +1 -1
index b99e08e6815b..c9971b9dbb09 100644
@@ -496,7 +496,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
 	return output + entry_offset;
 }
 
-void fortify_panic(const char *name)
+void __fortify_panic(const u8 reason)
 {
 	error("detected buffer overflow");
 }
include/linux/fortify-string.h +60 -21

201 lines not on this page · raw message

lib/string_helpers.c +20 -3
index 606c3099013f..9291dc74ae01 100644
@@ -1008,10 +1008,27 @@ EXPORT_SYMBOL(__read_overflow2_field);
 void __write_overflow_field(size_t avail, size_t wanted) { }
 EXPORT_SYMBOL(__write_overflow_field);
 
-void fortify_panic(const char *name)
+static const char * const fortify_func_name[] = {
+#define MAKE_FORTIFY_FUNC_NAME(func)	[MAKE_FORTIFY_FUNC(func)] = #func
+	EACH_FORTIFY_FUNC(MAKE_FORTIFY_FUNC_NAME)
+#undef  MAKE_FORTIFY_FUNC_NAME
+};
+
+void __fortify_report(const u8 reason)
+{
+	const u8 func = FORTIFY_REASON_FUNC(reason);
+	const bool write = FORTIFY_REASON_DIR(reason);
+	const char *name;
+
+	name = fortify_func_name[umin(func, FORTIFY_FUNC_UNKNOWN)];
+	WARN(1, "%s: detected buffer %s overflow\n", name, str_read_write(!write));
+}
+EXPORT_SYMBOL(__fortify_report);
+
+void __fortify_panic(const u8 reason)
 {
-	pr_emerg("detected buffer overflow in %s\n", name);
+	__fortify_report(reason);
 	BUG();
 }
-EXPORT_SYMBOL(fortify_panic);
+EXPORT_SYMBOL(__fortify_panic);
 #endif /* CONFIG_FORTIFY_SOURCE */
tools/objtool/noreturns.h +1 -1
index 1685d7ea6a9f..3a301696f005 100644
@@ -6,6 +6,7 @@
  *
  * Yes, this is unfortunate.  A better solution is in the works.
  */
+NORETURN(__fortify_panic)
 NORETURN(__kunit_abort)
 NORETURN(__module_put_and_kthread_exit)
 NORETURN(__reiserfs_panic)
@@ -22,7 +23,6 @@ NORETURN(do_exit)
 NORETURN(do_group_exit)
 NORETURN(do_task_dead)
 NORETURN(ex_handler_msr_mce)
-NORETURN(fortify_panic)
 NORETURN(hlt_play_dead)
 NORETURN(hv_ghcb_terminate)
 NORETURN(kthread_complete_and_exit)
Signature · 1 line
2.34.1
author
#2 lore raw

PATCH v3 2/5 fortify: Allow KUnit test to build without FORTIFY

In reply to Kees Cook

In order for CI systems to notice all the skipped tests related to CONFIG_FORTIFY_SOURCE, allow the FORTIFY_SOURCE KUnit tests to build with or without CONFIG_FORTIFY_SOURCE.

---
Cc: [email protected]

---
 lib/Kconfig.debug   |  2 +-
 lib/fortify_kunit.c | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)
lib/Kconfig.debug +1 -1
index 975a07f9f1cc..4e2febe3b568 100644
@@ -2748,7 +2748,7 @@ config STACKINIT_KUNIT_TEST
 
 config FORTIFY_KUNIT_TEST
 	tristate "Test fortified str*() and mem*() function internals at runtime" if !KUNIT_ALL_TESTS
-	depends on KUNIT && FORTIFY_SOURCE
+	depends on KUNIT
 	default KUNIT_ALL_TESTS
 	help
 	  Builds unit tests for checking internals of FORTIFY_SOURCE as used
lib/fortify_kunit.c +14 -0
index 2e4fedc81621..7a88b5dd3d27 100644
@@ -22,6 +22,11 @@
 #include <linux/string.h>
 #include <linux/vmalloc.h>
 
+/* Handle being built without CONFIG_FORTIFY_SOURCE */
+#ifndef __compiletime_strlen
+# define __compiletime_strlen __builtin_strlen
+#endif
+
 static const char array_of_10[] = "this is 10";
 static const char *ptr_of_11 = "this is 11!";
 static char array_unknown[] = "compiler thinks I might change";
@@ -308,6 +313,14 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)
 } while (0)
 DEFINE_ALLOC_SIZE_TEST_PAIR(devm_kmalloc)
 
+static int fortify_test_init(struct kunit *test)
+{
+	if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE))
+		kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y");
+
+	return 0;
+}
+
 static struct kunit_case fortify_test_cases[] = {
 	KUNIT_CASE(known_sizes_test),
 	KUNIT_CASE(control_flow_split_test),
@@ -324,6 +337,7 @@ static struct kunit_case fortify_test_cases[] = {
 
 static struct kunit_suite fortify_test_suite = {
 	.name = "fortify",
+	.init = fortify_test_init,
 	.test_cases = fortify_test_cases,
 };
 
Signature · 1 line
2.34.1
author
#3 lore raw

PATCH v3 3/5 fortify: Provide KUnit counters for failure testing

In reply to Kees Cook

The standard C string APIs were not designed to have a failure mode; they were expected to always succeed without memory safety issues. Normally, CONFIG_FORTIFY_SOURCE will use fortify_panic() to stop processing, as truncating a read or write may provide an even worse system state. However, this creates a problem for testing under things like KUnit, which needs a way to survive failures.

When building with CONFIG_KUNIT, provide a failure path for all users of fortify_panic, and track whether the failure was a read overflow or a write overflow, for KUnit tests to examine. Inspired by similar logic in the slab tests.

---
Cc: [email protected]
Cc: Andy Shevchenko <[email protected]>
Cc: Brendan Higgins <[email protected]>
Cc: David Gow <[email protected]>
Cc: [email protected]

---
 include/linux/fortify-string.h | 43 ++++++++++++++++++----------------
 lib/fortify_kunit.c            | 41 ++++++++++++++++++++++++++++++++
 lib/string_helpers.c           |  2 ++
 3 files changed, 66 insertions(+), 20 deletions(-)
include/linux/fortify-string.h +23 -20
index 4f6767dcd933..fbfb90479b8f 100644
@@ -15,8 +15,10 @@
 #define FORTIFY_REASON(func, write)	(FIELD_PREP(BIT(0), write) | \
 					 FIELD_PREP(GENMASK(7, 1), func))
 
-#define fortify_panic(func, write)	\
-	__fortify_panic(FORTIFY_REASON(func, write))
+#ifndef fortify_panic
+# define fortify_panic(func, write, retfail)	\
+	 __fortify_panic(FORTIFY_REASON(func, write))
+#endif
 
 #define FORTIFY_READ		 0
 #define FORTIFY_WRITE		 1
@@ -181,7 +183,7 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__write_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_strncpy, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strncpy, FORTIFY_WRITE, p);
 	return __underlying_strncpy(p, q, size);
 }
 
@@ -212,7 +214,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
 	/* Do not check characters beyond the end of p. */
 	ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
 	if (p_size <= ret && maxlen != ret)
-		fortify_panic(FORTIFY_FUNC_strnlen, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_strnlen, FORTIFY_READ, ret);
 	return ret;
 }
 
@@ -248,7 +250,7 @@ __kernel_size_t __fortify_strlen(const char * const POS p)
 		return __underlying_strlen(p);
 	ret = strnlen(p, p_size);
 	if (p_size <= ret)
-		fortify_panic(FORTIFY_FUNC_strlen, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_strlen, FORTIFY_READ, ret);
 	return ret;
 }
 
@@ -299,7 +301,7 @@ __FORTIFY_INLINE ssize_t sized_strscpy(char * const POS p, const char * const PO
 	 * p_size.
 	 */
 	if (len > p_size)
-		fortify_panic(FORTIFY_FUNC_strscpy, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strscpy, FORTIFY_WRITE, -E2BIG);
 
 	/*
 	 * We can now safely call vanilla strscpy because we are protected from:
@@ -357,7 +359,7 @@ size_t strlcat(char * const POS p, const char * const POS q, size_t avail)
 
 	/* Give up if string is already overflowed. */
 	if (p_size <= p_len)
-		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_READ, wanted);
 
 	if (actual >= avail) {
 		copy_len = avail - p_len - 1;
@@ -366,7 +368,7 @@ size_t strlcat(char * const POS p, const char * const POS q, size_t avail)
 
 	/* Give up if copy will overflow. */
 	if (p_size <= actual)
-		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_WRITE, wanted);
 	__underlying_memcpy(p + p_len, q, copy_len);
 	p[actual] = '\0';
 
@@ -395,7 +397,7 @@ char *strcat(char * const POS p, const char *q)
 	const size_t p_size = __member_size(p);
 
 	if (strlcat(p, q, p_size) >= p_size)
-		fortify_panic(FORTIFY_FUNC_strcat, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strcat, FORTIFY_WRITE, p);
 	return p;
 }
 
@@ -431,13 +433,13 @@ char *strncat(char * const POS p, const char * const POS q, __kernel_size_t coun
 	p_len = strlen(p);
 	copy_len = strnlen(q, count);
 	if (p_size < p_len + copy_len + 1)
-		fortify_panic(FORTIFY_FUNC_strncat, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strncat, FORTIFY_WRITE, p);
 	__underlying_memcpy(p + p_len, q, copy_len);
 	p[p_len + copy_len] = '\0';
 	return p;
 }
 
-__FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
+__FORTIFY_INLINE bool fortify_memset_chk(__kernel_size_t size,
 					 const size_t p_size,
 					 const size_t p_size_field)
 {
@@ -472,7 +474,8 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
 	 * lengths are unknown.)
 	 */
 	if (p_size != SIZE_MAX && p_size < size)
-		fortify_panic(FORTIFY_FUNC_memset, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_memset, FORTIFY_WRITE, true);
+	return false;
 }
 
 #define __fortify_memset_chk(p, c, size, p_size, p_size_field) ({	\
@@ -571,9 +574,9 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
 	 * lengths are unknown.)
 	 */
 	if (p_size != SIZE_MAX && p_size < size)
-		fortify_panic(func, FORTIFY_WRITE);
+		fortify_panic(func, FORTIFY_WRITE, true);
 	else if (q_size != SIZE_MAX && q_size < size)
-		fortify_panic(func, FORTIFY_READ);
+		fortify_panic(func, FORTIFY_READ, true);
 
 	/*
 	 * Warn when writing beyond destination field size.
@@ -673,7 +676,7 @@ __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_memscan, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_memscan, FORTIFY_READ, NULL);
 	return __real_memscan(p, c, size);
 }
 
@@ -690,7 +693,7 @@ int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t
 			__read_overflow2();
 	}
 	if (p_size < size || q_size < size)
-		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ, INT_MIN);
 	return __underlying_memcmp(p, q, size);
 }
 
@@ -702,7 +705,7 @@ void *memchr(const void * const POS0 p, int c, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_memchr, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_memchr, FORTIFY_READ, NULL);
 	return __underlying_memchr(p, c, size);
 }
 
@@ -714,7 +717,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_memchr_inv, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_memchr_inv, FORTIFY_READ, NULL);
 	return __real_memchr_inv(p, c, size);
 }
 
@@ -727,7 +730,7 @@ __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_kmemdup, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_kmemdup, FORTIFY_READ, NULL);
 	return __real_kmemdup(p, size, gfp);
 }
 
@@ -764,7 +767,7 @@ char *strcpy(char * const POS p, const char * const POS q)
 		__write_overflow();
 	/* Run-time check for dynamic size overflow. */
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_strcpy, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strcpy, FORTIFY_WRITE, p);
 	__underlying_memcpy(p, q, size);
 	return p;
 }
lib/fortify_kunit.c +41 -0

65 lines not on this page · raw message

lib/string_helpers.c +2 -0
index 9291dc74ae01..5e53d42e32bb 100644
@@ -18,6 +18,8 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/string_helpers.h>
+#include <kunit/test.h>
+#include <kunit/test-bug.h>
 
 /**
  * string_get_size - get the size in the specified units
Signature · 1 line
2.34.1
author
#4 lore raw

PATCH v3 4/5 fortify: Add KUnit tests for runtime overflows

In reply to Kees Cook

With fortify overflows able to be redirected, we can use KUnit to exercise the overflow conditions. Add tests for every API covered by CONFIG_FORTIFY_SOURCE, except for memset() and memcpy(), which are special-cased for now.

Note that this makes the LKDTM FORTIFY_STR* tests obsolete, but those can be removed separately.

---
Cc: [email protected]
Cc: Brendan Higgins <[email protected]>
Cc: David Gow <[email protected]>
Cc: [email protected]

---
 lib/Makefile        |   1 +
 lib/fortify_kunit.c | 607 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 606 insertions(+), 2 deletions(-)
lib/Makefile +1 -0
index bc36a5c167db..9d0e2af8100a 100644
@@ -402,6 +402,7 @@ obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
 CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable)
 obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o
 CFLAGS_fortify_kunit.o += $(call cc-disable-warning, unsequenced)
+CFLAGS_fortify_kunit.o += $(call cc-disable-warning, stringop-overread)
 CFLAGS_fortify_kunit.o += $(DISABLE_STRUCTLEAK_PLUGIN)
 obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o
 obj-$(CONFIG_STRCAT_KUNIT_TEST) += strcat_kunit.o
lib/fortify_kunit.c +605 -2

624 lines not on this page · raw message

Signature · 1 line
2.34.1
author
#5 lore raw

PATCH v3 5/5 fortify: Improve buffer overflow reporting

In reply to Kees Cook

Improve the reporting of buffer overflows under CONFIG_FORTIFY_SOURCE to help accelerate debugging efforts. The calculations are all just sitting in registers anyway, so pass them along to the function to be reported.

For example, before:

detected buffer overflow in memcpy

and after:

memcpy: detected buffer overflow: 4096 byte read of buffer size 1

---
 arch/arm/boot/compressed/misc.c |  2 +-
 arch/x86/boot/compressed/misc.c |  2 +-
 include/linux/fortify-string.h  | 56 ++++++++++++++++++---------------
 lib/fortify_kunit.c             |  4 +--
 lib/string_helpers.c            |  9 +++---
 5 files changed, 39 insertions(+), 34 deletions(-)
arch/arm/boot/compressed/misc.c +1 -1
index d93e2e466f6a..6c41b270560e 100644
@@ -154,7 +154,7 @@ decompress_kernel(unsigned long output_start, unsigned long free_mem_ptr_p,
 		putstr(" done, booting the kernel.\n");
 }
 
-void __fortify_panic(const u8 reason)
+void __fortify_panic(const u8 reason, size_t avail, size_t size)
 {
 	error("detected buffer overflow");
 }
arch/x86/boot/compressed/misc.c +1 -1
index c9971b9dbb09..1844da203da9 100644
@@ -496,7 +496,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
 	return output + entry_offset;
 }
 
-void __fortify_panic(const u8 reason)
+void __fortify_panic(const u8 reason, size_t avail, size_t size)
 {
 	error("detected buffer overflow");
 }
include/linux/fortify-string.h +30 -26

171 lines not on this page · raw message

lib/fortify_kunit.c +2 -2
index f0accebeca02..493ec02dd5b3 100644
@@ -17,8 +17,8 @@
 
 /* Redefine fortify_panic() to track failures. */
 void fortify_add_kunit_error(int write);
-#define fortify_panic(func, write, retfail) do {			\
-	__fortify_report(FORTIFY_REASON(func, write));			\
+#define fortify_panic(func, write, avail, size, retfail) do {		\
+	__fortify_report(FORTIFY_REASON(func, write), avail, size);	\
 	fortify_add_kunit_error(write);					\
 	return (retfail);						\
 } while (0)
lib/string_helpers.c +5 -4
index 5e53d42e32bb..5419282e12bd 100644
@@ -1016,20 +1016,21 @@ static const char * const fortify_func_name[] = {
 #undef  MAKE_FORTIFY_FUNC_NAME
 };
 
-void __fortify_report(const u8 reason)
+void __fortify_report(const u8 reason, const size_t avail, const size_t size)
 {
 	const u8 func = FORTIFY_REASON_FUNC(reason);
 	const bool write = FORTIFY_REASON_DIR(reason);
 	const char *name;
 
 	name = fortify_func_name[umin(func, FORTIFY_FUNC_UNKNOWN)];
-	WARN(1, "%s: detected buffer %s overflow\n", name, str_read_write(!write));
+	WARN(1, "%s: detected buffer overflow: %zu byte %s of buffer size %zu\n",
+		 name, size, str_read_write(!write), avail);
 }
 EXPORT_SYMBOL(__fortify_report);
 
-void __fortify_panic(const u8 reason)
+void __fortify_panic(const u8 reason, const size_t avail, const size_t size)
 {
-	__fortify_report(reason);
+	__fortify_report(reason, avail, size);
 	BUG();
 }
 EXPORT_SYMBOL(__fortify_panic);
Signature · 1 line
2.34.1