Re: [PATCH 34/64] fortify: Detect struct member overflows in memcpy() at compile-time
From: Nick Desaulniers <ndesaulniers@google.com>
Date: 2021-07-27 22:43:44
Also in:
dri-devel, linux-block, linux-hardening, linux-kbuild, linux-staging, lkml, netdev
On Tue, Jul 27, 2021 at 2:17 PM Kees Cook [off-list ref] wrote:
To accelerate the review of potential run-time false positives, it's also worth noting that it is possible to partially automate checking by examining memcpy() buffer argument fields to see if they have a neighboring. It is reasonable to expect that the vast majority of
a neighboring...field?
quoted hunk ↗ jump to hunk
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index 7e67d02764db..5e79e626172b 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h@@ -2,13 +2,17 @@ #ifndef _LINUX_FORTIFY_STRING_H_ #define _LINUX_FORTIFY_STRING_H_ +#include <linux/bug.h>
What are you using from linux/bug.h here?
quoted hunk ↗ jump to hunk
+ #define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline)) #define __RENAME(x) __asm__(#x) void fortify_panic(const char *name) __noreturn __cold; void __read_overflow(void) __compiletime_error("detected read beyond size of object (1st parameter)"); void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)"); +void __read_overflow2_field(void) __compiletime_warning("detected read beyond size of field (2nd parameter); maybe use struct_group()?"); void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)"); +void __write_overflow_field(void) __compiletime_warning("detected write beyond size of field (1st parameter); maybe use struct_group()?"); #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS) extern void *__underlying_memchr(const void *p, int c, __kernel_size_t size) __RENAME(memchr);@@ -182,22 +186,105 @@ __FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size) return __underlying_memset(p, c, size); } -__FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size) +/* + * To make sure the compiler can enforce protection against buffer overflows, + * memcpy(), memmove(), and memset() must not be used beyond individual + * struct members. If you need to copy across multiple members, please use + * struct_group() to create a named mirror of an anonymous struct union. + * (e.g. see struct sk_buff.) + * + * Mitigation coverage + * Bounds checking at: + * +-------+-------+-------+-------+ + * | Compile time | Run time | + * memcpy() argument sizes: | write | read | write | read | + * +-------+-------+-------+-------+ + * memcpy(known, known, constant) | y | y | n/a | n/a | + * memcpy(unknown, known, constant) | n | y | V | n/a | + * memcpy(known, unknown, constant) | y | n | n/a | V | + * memcpy(unknown, unknown, constant) | n | n | V | V | + * memcpy(known, known, dynamic) | n | n | b | B | + * memcpy(unknown, known, dynamic) | n | n | V | B | + * memcpy(known, unknown, dynamic) | n | n | b | V | + * memcpy(unknown, unknown, dynamic) | n | n | V | V | + * +-------+-------+-------+-------+ + * + * y = deterministic compile-time bounds checking + * n = cannot do deterministic compile-time bounds checking + * n/a = no run-time bounds checking needed since compile-time deterministic + * b = perform run-time bounds checking + * B = can perform run-time bounds checking, but current unenforced + * V = vulnerable to run-time overflow + * + */ +__FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size, + const size_t p_size, + const size_t q_size, + const size_t p_size_field, + const size_t q_size_field, + const char *func) { - size_t p_size = __builtin_object_size(p, 0); - size_t q_size = __builtin_object_size(q, 0); - if (__builtin_constant_p(size)) { - if (p_size < size) + /* + * Length argument is a constant expression, so we + * can perform compile-time bounds checking where + * buffer sizes are known. + */ + + /* Error when size is larger than enclosing struct. */ + if (p_size > p_size_field && p_size < size) __write_overflow(); - if (q_size < size) + if (q_size > q_size_field && q_size < size) __read_overflow2(); + + /* Warn when write size argument larger than dest field. */ + if (p_size_field < size) + __write_overflow_field(); + /* + * Warn for source field over-read when building with W=1 + * or when an over-write happened, so both can be fixed at + * the same time. + */ + if ((IS_ENABLED(KBUILD_EXTRA_WARN1) || p_size_field < size) && + q_size_field < size) + __read_overflow2_field(); } - if (p_size < size || q_size < size) - fortify_panic(__func__); - return __underlying_memcpy(p, q, size); + /* + * At this point, length argument may not be a constant expression, + * so run-time bounds checking can be done where buffer sizes are + * known. (This is not an "else" because the above checks may only + * be compile-time warnings, and we want to still warn for run-time + * overflows.) + */ + + /* + * Always stop accesses beyond the struct that contains the + * field, when the buffer's remaining size is known. + * (The -1 test is to optimize away checks where the buffer + * lengths are unknown.) + */ + if ((p_size != (size_t)(-1) && p_size < size) || + (q_size != (size_t)(-1) && q_size < size)) + fortify_panic(func); } +#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); \ + __underlying_##op(p, q, __fortify_size); \ +})
Are there other macro expansion sites for `__fortify_memcpy_chk`, perhaps later in this series? I don't understand why `memcpy` is passed as `func` to `fortify_panic()` rather than continuing to use `__func__`?
quoted hunk ↗ jump to hunk
+ +/* + * __builtin_object_size() must be captured here to avoid evaluating argument + * side-effects further into the macro layers. + */ +#define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \ + __builtin_object_size(p, 0), __builtin_object_size(q, 0), \ + __builtin_object_size(p, 1), __builtin_object_size(q, 1), \ + memcpy) + __FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size) { size_t p_size = __builtin_object_size(p, 0);@@ -277,27 +364,27 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp) return __real_kmemdup(p, size, gfp); } -/* defined after fortified strlen and memcpy to reuse them */ +/* Defined after fortified strlen to reuse it. */ __FORTIFY_INLINE char *strcpy(char *p, const char *q) { size_t p_size = __builtin_object_size(p, 1); size_t q_size = __builtin_object_size(q, 1); size_t size; + /* If neither buffer size is known, immediately give up. */ if (p_size == (size_t)-1 && q_size == (size_t)-1) return __underlying_strcpy(p, q); size = strlen(q) + 1; /* test here to use the more stringent object size */ if (p_size < size) fortify_panic(__func__); - memcpy(p, q, size); + __underlying_memcpy(p, q, size); return p; } /* Don't use these outside the FORITFY_SOURCE implementation */ #undef __underlying_memchr #undef __underlying_memcmp -#undef __underlying_memcpy #undef __underlying_memmove #undef __underlying_memset #undef __underlying_strcatdiff --git a/include/linux/string.h b/include/linux/string.h index 9473f81b9db2..cbe889e404e2 100644 --- a/include/linux/string.h +++ b/include/linux/string.h@@ -261,8 +261,9 @@ static inline const char *kbasename(const char *path) * @count: The number of bytes to copy * @pad: Character to use for padding if space is left in destination. */ -static inline void memcpy_and_pad(void *dest, size_t dest_len, - const void *src, size_t count, int pad) +static __always_inline void memcpy_and_pad(void *dest, size_t dest_len, + const void *src, size_t count, + int pad)
Why __always_inline here?
quoted hunk ↗ jump to hunk
{ if (dest_len > count) { memcpy(dest, src, count);diff --git a/lib/Makefile b/lib/Makefile index 083a19336e20..74523fd394bd 100644 --- a/lib/Makefile +++ b/lib/Makefile@@ -370,7 +370,8 @@ TEST_FORTIFY_LOG = test_fortify.log quiet_cmd_test_fortify = TEST $@ cmd_test_fortify = $(CONFIG_SHELL) $(srctree)/scripts/test_fortify.sh \ $< $@ "$(NM)" $(CC) $(c_flags) \ - $(call cc-disable-warning,fortify-source) + $(call cc-disable-warning,fortify-source) \ + -DKBUILD_EXTRA_WARN1 targets += $(TEST_FORTIFY_LOGS) clean-files += $(TEST_FORTIFY_LOGS)diff --git a/lib/string_helpers.c b/lib/string_helpers.c index faa9d8e4e2c5..4d205bf5993c 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c@@ -884,6 +884,12 @@ char *strreplace(char *s, char old, char new) EXPORT_SYMBOL(strreplace); #ifdef CONFIG_FORTIFY_SOURCE +/* These are placeholders for fortify compile-time warnings. */ +void __read_overflow2_field(void) { } +EXPORT_SYMBOL(__read_overflow2_field); +void __write_overflow_field(void) { } +EXPORT_SYMBOL(__write_overflow_field); +
Don't we rely on these being undefined for Clang to produce a linkage failure (until https://reviews.llvm.org/D106030 has landed)? By providing a symbol definition we can link against, I don't think __compiletime_{warning|error} will warn at all with Clang?
quoted hunk ↗ jump to hunk
void fortify_panic(const char *name) { pr_emerg("detected buffer overflow in %s\n", name);diff --git a/lib/test_fortify/read_overflow2_field-memcpy.c b/lib/test_fortify/read_overflow2_field-memcpy.c new file mode 100644 index 000000000000..de9569266223 --- /dev/null +++ b/lib/test_fortify/read_overflow2_field-memcpy.c@@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memcpy(large, instance.buf, sizeof(instance.buf) + 1) + +#include "test_fortify.h"diff --git a/lib/test_fortify/write_overflow_field-memcpy.c b/lib/test_fortify/write_overflow_field-memcpy.c new file mode 100644 index 000000000000..28cc81058dd3 --- /dev/null +++ b/lib/test_fortify/write_overflow_field-memcpy.c@@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memcpy(instance.buf, large, sizeof(instance.buf) + 1) + +#include "test_fortify.h" --
I haven't read the whole series yet, but I assume test_fortify.h was provided earlier in the series? -- Thanks, ~Nick Desaulniers