--- v2
+++ v1
@@ -28,12 +28,10 @@
char array[6]; /* 6 bytes */
u64 scalar2; /* 8 bytes */
u32 scalar3; /* 4 bytes */
- u32 scalar4; /* 4 bytes */
} instance;
-__builtin_object_size(instance.array, 0) == 22, since the remaining size
-of the enclosing structure starting from "array" is 22 bytes (6 + 8 +
-4 + 4).
+__builtin_object_size(instance.array, 0) == 18, since the remaining size
+of the enclosing structure starting from "array" is 18 bytes (6 + 8 + 4).
__builtin_object_size(instance.array, 1) == 6, since the remaining size
of the specific field "array" is 6 bytes.
@@ -41,10 +39,10 @@
The initial implementation of FORTIFY_SOURCE used mode 0 because there
were many cases of both strcpy() and memcpy() functions being used to
write (or read) across multiple fields in a structure. For example,
-it would catch this, which is writing 2 bytes beyond the end of
+this would catch this, which is writing 2 bytes beyond the end of
"instance":
- memcpy(&instance.array, data, 24);
+ memcpy(&instance.array, data, 20);
While this didn't protect against overwriting adjacent fields in a given
structure, it would at least stop overflows from reaching beyond the
@@ -165,37 +163,34 @@
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 the memcpy() buffer argument to check for the destination
-struct member having a neighboring array member. It is reasonable to
-expect that the vast majority of run-time false positives would look like
-the already evaluated and fixed compile-time false positives, where the
-most common pattern is neighboring arrays. (And, FWIW, several of the
-compile-time fixes were actual bugs.)
+by examining memcpy() buffer argument fields to see if they have
+a neighboring. It is reasonable to expect that the vast majority of
+run-time false positives would look like the already evaluated and fixed
+compile-time false positives, where the most common pattern is neighboring
+arrays. (And, FWIW, several of the compile-time fixes were actual bugs.)
Implementation:
-Tighten the memcpy() destination buffer size checking to use the actual
-("mode 1") target buffer size as the bounds check instead of their
-enclosing structure's ("mode 0") size. Use a common inline for memcpy()
-(and memmove() in a following patch), since all the tests are the
-same. All new cross-field memcpy() uses must use the struct_group() macro
-or similar to target a specific range of fields, so that FORTIFY_SOURCE
-can reason about the size and safety of the copy.
+Tighten the memcpy() buffer size checking to use the actual ("mode 1")
+target buffer size as the bounds check instead of their enclosing
+structure's ("mode 0") size. Use a common inline for memcpy() (and
+memmove() in a following patch), since all the tests are the same. All new
+cross-field memcpy() uses must use the struct_group() macro or similar
+to target a specific range of fields, so that FORTIFY_SOURCE can reason
+about the size and safety of the copy.
+
+For run-time, the "mode 0" size checking and mitigation is left unchanged,
+with "mode 1" added only to writes, and only performing a WARN() for
+now. This way any missed run-time false positives can be flushed out over
+the coming several development cycles, but system builders who have tested
+their workloads to be WARN()-free can enable the panic_on_warn=1 sysctl
+to immediately gain a mitigation against this class of buffer overflows.
For now, cross-member "mode 1" read detection at compile-time will be
limited to W=1 builds, since it is, unfortunately, very common. As the
-priority is solving write overflows, read overflows can be part of the
-next phase.
-
-For run-time, the "mode 0" size checking and mitigation is left unchanged,
-with "mode 1" to be added in stages. In this patch, no new run-time
-checks are added. Future patches will first bounds-check writes,
-and only perform a WARN() for now. This way any missed run-time false
-positives can be flushed out over the coming several development cycles,
-but system builders who have tested their workloads to be WARN()-free
-can enable the panic_on_warn=1 sysctl to immediately gain a mitigation
-against this class of buffer overflows. Once that is under way, run-time
-bounds-checking of reads can be similarly added.
+priority is solving write overflows, read overflows can be the next
+phase. Similarly, run-time cross-member "mode 1" read detection will be
+added at a later time, once write false-positives have been handled.
Related classes of flaws that remain unmitigated:
@@ -215,31 +210,39 @@
Signed-off-by: Kees Cook <keescook@chromium.org>
---
- include/linux/fortify-string.h | 109 ++++++++++++++++--
+ include/linux/fortify-string.h | 111 ++++++++++++++++--
include/linux/string.h | 5 +-
lib/Makefile | 3 +-
lib/string_helpers.c | 6 +
.../read_overflow2_field-memcpy.c | 5 +
.../write_overflow_field-memcpy.c | 5 +
- 6 files changed, 118 insertions(+), 15 deletions(-)
+ 6 files changed, 120 insertions(+), 15 deletions(-)
create mode 100644 lib/test_fortify/read_overflow2_field-memcpy.c
create mode 100644 lib/test_fortify/write_overflow_field-memcpy.c
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
-index e232a63fd826..25943442f532 100644
+index 7e67d02764db..5e79e626172b 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
-@@ -8,7 +8,9 @@
+@@ -2,13 +2,17 @@
+ #ifndef _LINUX_FORTIFY_STRING_H_
+ #define _LINUX_FORTIFY_STRING_H_
+
++#include <linux/bug.h>
++
+ #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(size_t avail, size_t wanted) __compiletime_warning("detected read beyond size of field (2nd parameter); maybe use struct_group()?");
++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(size_t avail, size_t wanted) __compiletime_warning("detected write beyond size of field (1st parameter); maybe use struct_group()?");
-
- #define __compiletime_strlen(p) ({ \
- size_t ret = (size_t)-1; \
-@@ -207,22 +209,105 @@ __FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size)
++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);
}
@@ -302,7 +305,7 @@
+
+ /* Warn when write size argument larger than dest field. */
+ if (p_size_field < size)
-+ __write_overflow_field(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
@@ -310,7 +313,7 @@
+ */
+ if ((IS_ENABLED(KBUILD_EXTRA_WARN1) || p_size_field < size) &&
+ q_size_field < size)
-+ __read_overflow2_field(q_size_field, size);
++ __read_overflow2_field();
}
- if (p_size < size || q_size < size)
- fortify_panic(__func__);
@@ -354,7 +357,7 @@
__FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size)
{
size_t p_size = __builtin_object_size(p, 0);
-@@ -302,13 +387,14 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)
+@@ -277,27 +364,27 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)
return __real_kmemdup(p, size, gfp);
}
@@ -370,8 +373,7 @@
if (p_size == (size_t)-1 && q_size == (size_t)-1)
return __underlying_strcpy(p, q);
size = strlen(q) + 1;
-@@ -318,14 +404,13 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
- /* Run-time check for dynamic size overflow. */
+ /* test here to use the more stringent object size */
if (p_size < size)
fortify_panic(__func__);
- memcpy(p, q, size);
@@ -403,7 +405,7 @@
if (dest_len > count) {
memcpy(dest, src, count);
diff --git a/lib/Makefile b/lib/Makefile
-index 8a4c8bdb38a2..ff80f09947c2 100644
+index 083a19336e20..74523fd394bd 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -370,7 +370,8 @@ TEST_FORTIFY_LOG = test_fortify.log
@@ -417,7 +419,7 @@
targets += $(TEST_FORTIFY_LOGS)
clean-files += $(TEST_FORTIFY_LOGS)
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
-index faa9d8e4e2c5..961636c120b1 100644
+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)
@@ -425,9 +427,9 @@
#ifdef CONFIG_FORTIFY_SOURCE
+/* These are placeholders for fortify compile-time warnings. */
-+void __read_overflow2_field(size_t avail, size_t wanted) { }
++void __read_overflow2_field(void) { }
+EXPORT_SYMBOL(__read_overflow2_field);
-+void __write_overflow_field(size_t avail, size_t wanted) { }
++void __write_overflow_field(void) { }
+EXPORT_SYMBOL(__write_overflow_field);
+
void fortify_panic(const char *name)