[04/32] fortify: Add run-time WARN for cross-field memcpy()

Commit Message

Kees Cook May 4, 2022, 1:44 a.m. UTC

Enable run-time checking of dynamic memcpy() and memmove() lengths,
issuing a WARN when a write would exceed the size of the target struct
member, when built with CONFIG_FORTIFY_SOURCE=y. This would have caught
all of the memcpy()-based buffer overflows from 2018 through 2020,
specifically covering all the cases where the destination buffer size
is known at compile time.

This change ONLY adds a run-time warning. As false positives are currently
still expected, this will not block the overflow. The new warnings will
look like this:

  memcpy: detected field-spanning write (size N) of single field "var->dest" (size M)
  WARNING: CPU: n PID: pppp at source/file/path.c:nr function+0xXX/0xXX [module]

The false positives are most likely where intentional field-spanning
writes are happening. These need to be addressed similarly to how the
compile-time cases were addressed: add a struct_group(), split the
memcpy(), use a flex_array.h helper, or some other refactoring.

In order to make identifying/investigating instances of added runtime
checks easier, each instance includes the destination variable name as a
WARN argument, prefixed with 'field "'. Therefore, on any given build,
it is trivial to inspect the artifacts to find instances. For example
on an x86_64 defconfig build, there are 78 new run-time memcpy() bounds
checks added:

  $ for i in vmlinux $(find . -name '*.ko'); do \
      strings "$i" | grep '^field "'; done | wc -l
  78

Currently, the common case where a destination buffer is known to be a
dynamic size (i.e. has a trailing flexible array) does not generate a
WARN. For example:

    struct normal_flex_array {
	void *a;
	int b;
	size_t array_size;
	u32 c;
	u8 flex_array[];
    };

    struct normal_flex_array *instance;
    ...
    /* These cases will be ignored for run-time bounds checking. */
    memcpy(instance, src, len);
    memcpy(instance->flex_array, src, len);

This code pattern will need to be addressed separately, likely by
migrating to one of the flex_array.h family of helpers.

Note that one of the dynamic-sized destination cases is irritatingly
unable to be detected by the compiler: when using memcpy() to target
a composite struct member which contains a trailing flexible array
struct. For example:

    struct wrapper {
	int foo;
	char bar;
	struct normal_flex_array embedded;
    };

    struct wrapper *instance;
    ...
    /* This will incorrectly WARN when len > sizeof(instance->embedded) */
    memcpy(&instance->embedded, src, len);

These cases end up appearing to the compiler to be sized as if the
flexible array had 0 elements. :( For more details see:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
https://godbolt.org/z/vW6x8vh4P

Regardless, all cases of copying to/from flexible array structures
should be migrated to using the new flex*()-family of helpers to gain
their added safety checking, but priority will need to be given to the
"composite flexible array structure destination" cases noted above.

As mentioned, none of these bounds checks block any overflows
currently. For users that have tested their workloads, do not encounter
any warnings, and wish to make these checks stop any overflows, they
can use a big hammer and set the sysctl panic_on_warn=1.

Cc: Nathan Chancellor <[e-mail]>
Cc: Nick Desaulniers <[e-mail]>
Cc: Tom Rix <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Kees Cook <[e-mail]>
---
 include/linux/fortify-string.h | 70 ++++++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 3 deletions(-)

Patch

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 295637a66c46..9f65527fff40 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -3,6 +3,7 @@ 
 #define _LINUX_FORTIFY_STRING_H_
 
 #include <linux/const.h>
+#include <linux/bug.h>
 
 #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
 #define __RENAME(x) __asm__(#x)
@@ -303,7 +304,7 @@  __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
  * V = vulnerable to run-time overflow (will need refactoring to solve)
  *
  */
-__FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,
+__FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
 					 const size_t p_size,
 					 const size_t q_size,
 					 const size_t p_size_field,
@@ -352,16 +353,79 @@  __FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,
 	if ((p_size != (size_t)(-1) && p_size < size) ||
 	    (q_size != (size_t)(-1) && q_size < size))
 		fortify_panic(func);
+
+	/*
+	 * Warn when writing beyond destination field size.
+	 *
+	 * We must ignore p_size_field == 0 and -1 for existing
+	 * 0-element and flexible arrays, until they are all converted
+	 * to flexible arrays and use the flex()-family of helpers.
+	 *
+	 * The implementation of __builtin_object_size() behaves
+	 * like sizeof() when not directly referencing a flexible
+	 * array member, which means there will be many bounds checks
+	 * that will appear at run-time, without a way for them to be
+	 * detected at compile-time (as can be done when the destination
+	 * is specifically the flexible array member).
+	 * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
+	 */
+	if (p_size_field != 0 && p_size_field != (size_t)(-1) &&
+	    p_size != p_size_field && p_size_field < size)
+		return true;
+
+	return false;
 }
 
 #define __fortify_memcpy_chk(p, q, size, p_size, q_size,		\
 			     p_size_field, q_size_field, op) ({		\
 	size_t __fortify_size = (size_t)(size);				\
-	fortify_memcpy_chk(__fortify_size, p_size, q_size,		\
-			   p_size_field, q_size_field, #op);		\
+	WARN_ONCE(fortify_memcpy_chk(__fortify_size, p_size, q_size,	\
+				     p_size_field, q_size_field, #op),	\
+		  #op ": detected field-spanning write (size %zu) of single %s (size %zu)\n", \
+		  __fortify_size,					\
+		  "field \"" #p "\" at " __FILE__ ":" __stringify(__LINE__), \
+		  p_size_field);					\
 	__underlying_##op(p, q, __fortify_size);			\
 })
 
+/*
+ * Notes about compile-time buffer size detection:
+ *
+ * With these types...
+ *
+ *	struct middle {
+ *		u16 a;
+ *		u8 middle_buf[16];
+ *		int b;
+ *	};
+ *	struct end {
+ *		u16 a;
+ *		u8 end_buf[16];
+ *	};
+ *	struct flex {
+ *		int a;
+ *		u8 flex_buf[];
+ *	};
+ *
+ *	void func(TYPE *ptr) { ... }
+ *
+ * Cases where destination size cannot be currently detected:
+ * - the size of ptr's object (seemingly by design, gcc & clang fail):
+ *	__builtin_object_size(ptr, 1) == -1
+ * - the size of flexible arrays in ptr's obj (by design, dynamic size):
+ *      __builtin_object_size(ptr->flex_buf, 1) == -1
+ * - the size of ANY array at the end of ptr's obj (gcc and clang bug):
+ *	__builtin_object_size(ptr->end_buf, 1) == -1
+ *	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836
+ *
+ * Cases where destination size is currently detected:
+ * - the size of non-array members within ptr's object:
+ *	__builtin_object_size(ptr->a, 1) == 2
+ * - the size of non-flexible-array in the middle of ptr's obj:
+ *	__builtin_object_size(ptr->middle_buf, 1) == 16
+ *
+ */
+
 /*
  * __builtin_object_size() must be captured here to avoid evaluating argument
  * side-effects further into the macro layers.