From addbeea6f50b5ac344331652dd7f35faf760969e Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 26 Aug 2022 09:21:15 -0700 Subject: [PATCH 01/46] testing/selftests: Add tests for the is_signed_type() macro Although not documented, is_signed_type() must support the 'bool' and pointer types next to scalar and enumeration types. Add a selftest that verifies that this macro handles all supported types correctly. Cc: Andrew Morton Cc: Arnd Bergmann Cc: Dan Williams Cc: Eric Dumazet Cc: Ingo Molnar Cc: Isabella Basso Cc: "Jason A. Donenfeld" Cc: Josh Poimboeuf Cc: Luc Van Oostenryck Cc: Masami Hiramatsu Cc: Nathan Chancellor Cc: Peter Zijlstra Cc: Rasmus Villemoes Cc: Sander Vanheule Cc: Steven Rostedt Cc: Vlastimil Babka Cc: Yury Norov Signed-off-by: Bart Van Assche Tested-by: Isabella Basso Acked-by: Rasmus Villemoes Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220826162116.1050972-2-bvanassche@acm.org --- lib/Kconfig.debug | 12 ++++++++++ lib/Makefile | 1 + lib/is_signed_type_kunit.c | 48 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+) create mode 100644 lib/is_signed_type_kunit.c diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 072e4b289c13e..36455953d3069 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2506,6 +2506,18 @@ config MEMCPY_KUNIT_TEST If unsure, say N. +config IS_SIGNED_TYPE_KUNIT_TEST + tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS + depends on KUNIT + default KUNIT_ALL_TESTS + help + Builds unit tests for the is_signed_type() macro. + + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + config OVERFLOW_KUNIT_TEST tristate "Test check_*_overflow() functions at runtime" if !KUNIT_ALL_TESTS depends on KUNIT diff --git a/lib/Makefile b/lib/Makefile index 5927d7fa08063..f545140ed9e74 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -377,6 +377,7 @@ obj-$(CONFIG_BITS_TEST) += test_bits.o obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o +obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_kunit.o 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 diff --git a/lib/is_signed_type_kunit.c b/lib/is_signed_type_kunit.c new file mode 100644 index 0000000000000..f2eedb1f09356 --- /dev/null +++ b/lib/is_signed_type_kunit.c @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +/* + * ./tools/testing/kunit/kunit.py run is_signed_type [--raw_output] + */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include +#include + +enum unsigned_enum { + constant_a = 3, +}; + +enum signed_enum { + constant_b = -1, + constant_c = 2, +}; + +static void is_signed_type_test(struct kunit *test) +{ + KUNIT_EXPECT_EQ(test, is_signed_type(bool), false); + KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true); + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false); + KUNIT_EXPECT_EQ(test, is_signed_type(int), true); + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned int), false); + KUNIT_EXPECT_EQ(test, is_signed_type(long), true); + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long), false); + KUNIT_EXPECT_EQ(test, is_signed_type(long long), true); + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long long), false); + KUNIT_EXPECT_EQ(test, is_signed_type(enum unsigned_enum), false); + KUNIT_EXPECT_EQ(test, is_signed_type(enum signed_enum), true); + KUNIT_EXPECT_EQ(test, is_signed_type(void *), false); + KUNIT_EXPECT_EQ(test, is_signed_type(const char *), false); +} + +static struct kunit_case is_signed_type_test_cases[] = { + KUNIT_CASE(is_signed_type_test), + {} +}; + +static struct kunit_suite is_signed_type_test_suite = { + .name = "is_signed_type", + .test_cases = is_signed_type_test_cases, +}; + +kunit_test_suite(is_signed_type_test_suite); + +MODULE_LICENSE("Dual MIT/GPL"); From 92d23c6e94157739b997cacce151586a0d07bb8a Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 26 Aug 2022 09:21:16 -0700 Subject: [PATCH 02/46] overflow, tracing: Define the is_signed_type() macro once There are two definitions of the is_signed_type() macro: one in and a second definition in . As suggested by Linus Torvalds, move the definition of the is_signed_type() macro into the header file. Change the definition of the is_signed_type() macro to make sure that it does not trigger any sparse warnings with future versions of sparse for bitwise types. See also: https://lore.kernel.org/all/CAHk-=whjH6p+qzwUdx5SOVVHjS3WvzJQr6mDUwhEyTf6pJWzaQ@mail.gmail.com/ https://lore.kernel.org/all/CAHk-=wjQGnVfb4jehFR0XyZikdQvCZouE96xR_nnf5kqaM5qqQ@mail.gmail.com/ Cc: Andrew Morton Cc: Arnd Bergmann Cc: Dan Williams Cc: Eric Dumazet Cc: Ingo Molnar Cc: Isabella Basso Cc: "Jason A. Donenfeld" Cc: Josh Poimboeuf Cc: Luc Van Oostenryck Cc: Masami Hiramatsu Cc: Nathan Chancellor Cc: Peter Zijlstra Cc: Rasmus Villemoes Cc: Sander Vanheule Cc: Steven Rostedt Cc: Vlastimil Babka Cc: Yury Norov Signed-off-by: Bart Van Assche Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220826162116.1050972-3-bvanassche@acm.org --- include/linux/compiler.h | 6 ++++++ include/linux/overflow.h | 1 - include/linux/trace_events.h | 2 -- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 01ce94b58b423..7713d7bcdaea9 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -239,6 +239,12 @@ static inline void *offset_to_ptr(const int *off) /* &a[0] degrades to a pointer: a different type from an array */ #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) +/* + * Whether 'type' is a signed type or an unsigned type. Supports scalar types, + * bool and also pointer types. + */ +#define is_signed_type(type) (((type)(-1)) < (__force type)1) + /* * This is needed in functions which generate the stack canary, see * arch/x86/kernel/smpboot.c::start_secondary() for an example. diff --git a/include/linux/overflow.h b/include/linux/overflow.h index f1221d11f8e57..0eb3b192f07ab 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -30,7 +30,6 @@ * https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html - * credit to Christian Biere. */ -#define is_signed_type(type) (((type)(-1)) < (type)1) #define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type))) #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T))) #define type_min(T) ((T)((T)-type_max(T)-(T)1)) diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h index b18759a673c66..8401dec93c155 100644 --- a/include/linux/trace_events.h +++ b/include/linux/trace_events.h @@ -814,8 +814,6 @@ extern int trace_add_event_call(struct trace_event_call *call); extern int trace_remove_event_call(struct trace_event_call *call); extern int trace_event_get_offsets(struct trace_event_call *call); -#define is_signed_type(type) (((type)(-1)) < (type)1) - int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set); int trace_set_clr_event(const char *system, const char *event, int set); int trace_array_set_clr_event(struct trace_array *tr, const char *system, From d219d2a9a92e39aa92799efe8f2aa21259b6dd82 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 29 Aug 2022 13:37:17 -0700 Subject: [PATCH 03/46] overflow: Allow mixed type arguments When the check_[op]_overflow() helpers were introduced, all arguments were required to be the same type to make the fallback macros simpler. However, now that the fallback macros have been removed[1], it is fine to allow mixed types, which makes using the helpers much more useful, as they can be used to test for type-based overflows (e.g. adding two large ints but storing into a u8), as would be handy in the drm core[2]. Remove the restriction, and add additional self-tests that exercise some of the mixed-type overflow cases, and double-check for accidental macro side-effects. [1] https://git.kernel.org/linus/4eb6bd55cfb22ffc20652732340c4962f3ac9a91 [2] https://lore.kernel.org/lkml/20220824084514.2261614-2-gwan-gyeong.mun@intel.com Cc: Rasmus Villemoes Cc: Gwan-gyeong Mun Cc: "Gustavo A. R. Silva" Cc: Nick Desaulniers Cc: linux-hardening@vger.kernel.org Reviewed-by: Andrzej Hajda Reviewed-by: Gwan-gyeong Mun Tested-by: Gwan-gyeong Mun Signed-off-by: Kees Cook --- include/linux/overflow.h | 72 ++++++++++++++++------------ lib/overflow_kunit.c | 101 ++++++++++++++++++++++++++++----------- 2 files changed, 113 insertions(+), 60 deletions(-) diff --git a/include/linux/overflow.h b/include/linux/overflow.h index 0eb3b192f07ab..19dfdd74835ec 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -51,40 +51,50 @@ static inline bool __must_check __must_check_overflow(bool overflow) return unlikely(overflow); } -/* - * For simplicity and code hygiene, the fallback code below insists on - * a, b and *d having the same type (similar to the min() and max() - * macros), whereas gcc's type-generic overflow checkers accept - * different types. Hence we don't just make check_add_overflow an - * alias for __builtin_add_overflow, but add type checks similar to - * below. +/** check_add_overflow() - Calculate addition with overflow checking + * + * @a: first addend + * @b: second addend + * @d: pointer to store sum + * + * Returns 0 on success. + * + * *@d holds the results of the attempted addition, but is not considered + * "safe for use" on a non-zero return value, which indicates that the + * sum has overflowed or been truncated. */ -#define check_add_overflow(a, b, d) __must_check_overflow(({ \ - typeof(a) __a = (a); \ - typeof(b) __b = (b); \ - typeof(d) __d = (d); \ - (void) (&__a == &__b); \ - (void) (&__a == __d); \ - __builtin_add_overflow(__a, __b, __d); \ -})) +#define check_add_overflow(a, b, d) \ + __must_check_overflow(__builtin_add_overflow(a, b, d)) -#define check_sub_overflow(a, b, d) __must_check_overflow(({ \ - typeof(a) __a = (a); \ - typeof(b) __b = (b); \ - typeof(d) __d = (d); \ - (void) (&__a == &__b); \ - (void) (&__a == __d); \ - __builtin_sub_overflow(__a, __b, __d); \ -})) +/** check_sub_overflow() - Calculate subtraction with overflow checking + * + * @a: minuend; value to subtract from + * @b: subtrahend; value to subtract from @a + * @d: pointer to store difference + * + * Returns 0 on success. + * + * *@d holds the results of the attempted subtraction, but is not considered + * "safe for use" on a non-zero return value, which indicates that the + * difference has underflowed or been truncated. + */ +#define check_sub_overflow(a, b, d) \ + __must_check_overflow(__builtin_sub_overflow(a, b, d)) -#define check_mul_overflow(a, b, d) __must_check_overflow(({ \ - typeof(a) __a = (a); \ - typeof(b) __b = (b); \ - typeof(d) __d = (d); \ - (void) (&__a == &__b); \ - (void) (&__a == __d); \ - __builtin_mul_overflow(__a, __b, __d); \ -})) +/** check_mul_overflow() - Calculate multiplication with overflow checking + * + * @a: first factor + * @b: second factor + * @d: pointer to store product + * + * Returns 0 on success. + * + * *@d holds the results of the attempted multiplication, but is not + * considered "safe for use" on a non-zero return value, which indicates + * that the product has overflowed or been truncated. + */ +#define check_mul_overflow(a, b, d) \ + __must_check_overflow(__builtin_mul_overflow(a, b, d)) /** check_shl_overflow() - Calculate a left-shifted value and check overflow * diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c index 7e3e43679b73c..0d98c9bc75dae 100644 --- a/lib/overflow_kunit.c +++ b/lib/overflow_kunit.c @@ -16,12 +16,15 @@ #include #include -#define DEFINE_TEST_ARRAY(t) \ - static const struct test_ ## t { \ - t a, b; \ - t sum, diff, prod; \ - bool s_of, d_of, p_of; \ - } t ## _tests[] +#define DEFINE_TEST_ARRAY_TYPED(t1, t2, t) \ + static const struct test_ ## t1 ## _ ## t2 ## __ ## t { \ + t1 a; \ + t2 b; \ + t sum, diff, prod; \ + bool s_of, d_of, p_of; \ + } t1 ## _ ## t2 ## __ ## t ## _tests[] + +#define DEFINE_TEST_ARRAY(t) DEFINE_TEST_ARRAY_TYPED(t, t, t) DEFINE_TEST_ARRAY(u8) = { {0, 0, 0, 0, 0, false, false, false}, @@ -222,21 +225,27 @@ DEFINE_TEST_ARRAY(s64) = { }; #endif -#define check_one_op(t, fmt, op, sym, a, b, r, of) do { \ - t _r; \ - bool _of; \ - \ - _of = check_ ## op ## _overflow(a, b, &_r); \ - KUNIT_EXPECT_EQ_MSG(test, _of, of, \ +#define check_one_op(t, fmt, op, sym, a, b, r, of) do { \ + int _a_orig = a, _a_bump = a + 1; \ + int _b_orig = b, _b_bump = b + 1; \ + bool _of; \ + t _r; \ + \ + _of = check_ ## op ## _overflow(a, b, &_r); \ + KUNIT_EXPECT_EQ_MSG(test, _of, of, \ "expected "fmt" "sym" "fmt" to%s overflow (type %s)\n", \ - a, b, of ? "" : " not", #t); \ - KUNIT_EXPECT_EQ_MSG(test, _r, r, \ + a, b, of ? "" : " not", #t); \ + KUNIT_EXPECT_EQ_MSG(test, _r, r, \ "expected "fmt" "sym" "fmt" == "fmt", got "fmt" (type %s)\n", \ - a, b, r, _r, #t); \ + a, b, r, _r, #t); \ + /* Check for internal macro side-effects. */ \ + _of = check_ ## op ## _overflow(_a_orig++, _b_orig++, &_r); \ + KUNIT_EXPECT_EQ_MSG(test, _a_orig, _a_bump, "Unexpected " #op " macro side-effect!\n"); \ + KUNIT_EXPECT_EQ_MSG(test, _b_orig, _b_bump, "Unexpected " #op " macro side-effect!\n"); \ } while (0) -#define DEFINE_TEST_FUNC(t, fmt) \ -static void do_test_ ## t(struct kunit *test, const struct test_ ## t *p) \ +#define DEFINE_TEST_FUNC_TYPED(n, t, fmt) \ +static void do_test_ ## n(struct kunit *test, const struct test_ ## n *p) \ { \ check_one_op(t, fmt, add, "+", p->a, p->b, p->sum, p->s_of); \ check_one_op(t, fmt, add, "+", p->b, p->a, p->sum, p->s_of); \ @@ -245,15 +254,18 @@ static void do_test_ ## t(struct kunit *test, const struct test_ ## t *p) \ check_one_op(t, fmt, mul, "*", p->b, p->a, p->prod, p->p_of); \ } \ \ -static void t ## _overflow_test(struct kunit *test) { \ +static void n ## _overflow_test(struct kunit *test) { \ unsigned i; \ \ - for (i = 0; i < ARRAY_SIZE(t ## _tests); ++i) \ - do_test_ ## t(test, &t ## _tests[i]); \ + for (i = 0; i < ARRAY_SIZE(n ## _tests); ++i) \ + do_test_ ## n(test, &n ## _tests[i]); \ kunit_info(test, "%zu %s arithmetic tests finished\n", \ - ARRAY_SIZE(t ## _tests), #t); \ + ARRAY_SIZE(n ## _tests), #n); \ } +#define DEFINE_TEST_FUNC(t, fmt) \ + DEFINE_TEST_FUNC_TYPED(t ## _ ## t ## __ ## t, t, fmt) + DEFINE_TEST_FUNC(u8, "%d"); DEFINE_TEST_FUNC(s8, "%d"); DEFINE_TEST_FUNC(u16, "%d"); @@ -265,6 +277,33 @@ DEFINE_TEST_FUNC(u64, "%llu"); DEFINE_TEST_FUNC(s64, "%lld"); #endif +DEFINE_TEST_ARRAY_TYPED(u32, u32, u8) = { + {0, 0, 0, 0, 0, false, false, false}, + {U8_MAX, 2, 1, U8_MAX - 2, U8_MAX - 1, true, false, true}, + {U8_MAX + 1, 0, 0, 0, 0, true, true, false}, +}; +DEFINE_TEST_FUNC_TYPED(u32_u32__u8, u8, "%d"); + +DEFINE_TEST_ARRAY_TYPED(u32, u32, int) = { + {0, 0, 0, 0, 0, false, false, false}, + {U32_MAX, 0, -1, -1, 0, true, true, false}, +}; +DEFINE_TEST_FUNC_TYPED(u32_u32__int, int, "%d"); + +DEFINE_TEST_ARRAY_TYPED(u8, u8, int) = { + {0, 0, 0, 0, 0, false, false, false}, + {U8_MAX, U8_MAX, 2 * U8_MAX, 0, U8_MAX * U8_MAX, false, false, false}, + {1, 2, 3, -1, 2, false, false, false}, +}; +DEFINE_TEST_FUNC_TYPED(u8_u8__int, int, "%d"); + +DEFINE_TEST_ARRAY_TYPED(int, int, u8) = { + {0, 0, 0, 0, 0, false, false, false}, + {1, 2, 3, U8_MAX, 2, false, true, false}, + {-1, 0, U8_MAX, U8_MAX, 0, true, true, false}, +}; +DEFINE_TEST_FUNC_TYPED(int_int__u8, u8, "%d"); + static void overflow_shift_test(struct kunit *test) { int count = 0; @@ -649,17 +688,21 @@ static void overflow_size_helpers_test(struct kunit *test) } static struct kunit_case overflow_test_cases[] = { - KUNIT_CASE(u8_overflow_test), - KUNIT_CASE(s8_overflow_test), - KUNIT_CASE(u16_overflow_test), - KUNIT_CASE(s16_overflow_test), - KUNIT_CASE(u32_overflow_test), - KUNIT_CASE(s32_overflow_test), + KUNIT_CASE(u8_u8__u8_overflow_test), + KUNIT_CASE(s8_s8__s8_overflow_test), + KUNIT_CASE(u16_u16__u16_overflow_test), + KUNIT_CASE(s16_s16__s16_overflow_test), + KUNIT_CASE(u32_u32__u32_overflow_test), + KUNIT_CASE(s32_s32__s32_overflow_test), /* Clang 13 and earlier generate unwanted libcalls on 32-bit. */ #if BITS_PER_LONG == 64 - KUNIT_CASE(u64_overflow_test), - KUNIT_CASE(s64_overflow_test), + KUNIT_CASE(u64_u64__u64_overflow_test), + KUNIT_CASE(s64_s64__s64_overflow_test), #endif + KUNIT_CASE(u32_u32__u8_overflow_test), + KUNIT_CASE(u32_u32__int_overflow_test), + KUNIT_CASE(u8_u8__int_overflow_test), + KUNIT_CASE(int_int__u8_overflow_test), KUNIT_CASE(overflow_shift_test), KUNIT_CASE(overflow_allocation_test), KUNIT_CASE(overflow_size_helpers_test), From 779742255cb464e9e833fed2a8d352eb12936dae Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 31 Aug 2022 11:09:13 -0700 Subject: [PATCH 04/46] overflow: Split up kunit tests for smaller stack frames Under some pathological 32-bit configs, the shift overflow KUnit tests create huge stack frames. Split up the function to avoid this, separating by rough shift overflow cases. Cc: Rasmus Villemoes Cc: Daniel Latypov Cc: Vitor Massaru Iha Cc: "Gustavo A. R. Silva" Cc: Nick Desaulniers Reported-by: kernel test robot Link: https://lore.kernel.org/lkml/202208301850.iuv9VwA8-lkp@intel.com Acked-by: Daniel Latypov Signed-off-by: Kees Cook --- lib/overflow_kunit.c | 78 +++++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 27 deletions(-) diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c index 0d98c9bc75dae..f385ca652b747 100644 --- a/lib/overflow_kunit.c +++ b/lib/overflow_kunit.c @@ -304,10 +304,6 @@ DEFINE_TEST_ARRAY_TYPED(int, int, u8) = { }; DEFINE_TEST_FUNC_TYPED(int_int__u8, u8, "%d"); -static void overflow_shift_test(struct kunit *test) -{ - int count = 0; - /* Args are: value, shift, type, expected result, overflow expected */ #define TEST_ONE_SHIFT(a, s, t, expect, of) do { \ typeof(a) __a = (a); \ @@ -331,6 +327,10 @@ static void overflow_shift_test(struct kunit *test) count++; \ } while (0) +static void shift_sane_test(struct kunit *test) +{ + int count = 0; + /* Sane shifts. */ TEST_ONE_SHIFT(1, 0, u8, 1 << 0, false); TEST_ONE_SHIFT(1, 4, u8, 1 << 4, false); @@ -373,6 +373,13 @@ static void overflow_shift_test(struct kunit *test) TEST_ONE_SHIFT(0, 30, s32, 0, false); TEST_ONE_SHIFT(0, 62, s64, 0, false); + kunit_info(test, "%d sane shift tests finished\n", count); +} + +static void shift_overflow_test(struct kunit *test) +{ + int count = 0; + /* Overflow: shifted the bit off the end. */ TEST_ONE_SHIFT(1, 8, u8, 0, true); TEST_ONE_SHIFT(1, 16, u16, 0, true); @@ -420,6 +427,13 @@ static void overflow_shift_test(struct kunit *test) /* 0100000100001000001000000010000001000010000001000100010001001011 */ TEST_ONE_SHIFT(4686030735197619275LL, 2, s64, 0, true); + kunit_info(test, "%d overflow shift tests finished\n", count); +} + +static void shift_truncate_test(struct kunit *test) +{ + int count = 0; + /* Overflow: values larger than destination type. */ TEST_ONE_SHIFT(0x100, 0, u8, 0, true); TEST_ONE_SHIFT(0xFF, 0, s8, 0, true); @@ -431,6 +445,33 @@ static void overflow_shift_test(struct kunit *test) TEST_ONE_SHIFT(0xFFFFFFFFUL, 0, int, 0, true); TEST_ONE_SHIFT(0xFFFFFFFFFFFFFFFFULL, 0, s64, 0, true); + /* Overflow: shifted at or beyond entire type's bit width. */ + TEST_ONE_SHIFT(0, 8, u8, 0, true); + TEST_ONE_SHIFT(0, 9, u8, 0, true); + TEST_ONE_SHIFT(0, 8, s8, 0, true); + TEST_ONE_SHIFT(0, 9, s8, 0, true); + TEST_ONE_SHIFT(0, 16, u16, 0, true); + TEST_ONE_SHIFT(0, 17, u16, 0, true); + TEST_ONE_SHIFT(0, 16, s16, 0, true); + TEST_ONE_SHIFT(0, 17, s16, 0, true); + TEST_ONE_SHIFT(0, 32, u32, 0, true); + TEST_ONE_SHIFT(0, 33, u32, 0, true); + TEST_ONE_SHIFT(0, 32, int, 0, true); + TEST_ONE_SHIFT(0, 33, int, 0, true); + TEST_ONE_SHIFT(0, 32, s32, 0, true); + TEST_ONE_SHIFT(0, 33, s32, 0, true); + TEST_ONE_SHIFT(0, 64, u64, 0, true); + TEST_ONE_SHIFT(0, 65, u64, 0, true); + TEST_ONE_SHIFT(0, 64, s64, 0, true); + TEST_ONE_SHIFT(0, 65, s64, 0, true); + + kunit_info(test, "%d truncate shift tests finished\n", count); +} + +static void shift_nonsense_test(struct kunit *test) +{ + int count = 0; + /* Nonsense: negative initial value. */ TEST_ONE_SHIFT(-1, 0, s8, 0, true); TEST_ONE_SHIFT(-1, 0, u8, 0, true); @@ -455,26 +496,6 @@ static void overflow_shift_test(struct kunit *test) TEST_ONE_SHIFT(0, -30, s64, 0, true); TEST_ONE_SHIFT(0, -30, u64, 0, true); - /* Overflow: shifted at or beyond entire type's bit width. */ - TEST_ONE_SHIFT(0, 8, u8, 0, true); - TEST_ONE_SHIFT(0, 9, u8, 0, true); - TEST_ONE_SHIFT(0, 8, s8, 0, true); - TEST_ONE_SHIFT(0, 9, s8, 0, true); - TEST_ONE_SHIFT(0, 16, u16, 0, true); - TEST_ONE_SHIFT(0, 17, u16, 0, true); - TEST_ONE_SHIFT(0, 16, s16, 0, true); - TEST_ONE_SHIFT(0, 17, s16, 0, true); - TEST_ONE_SHIFT(0, 32, u32, 0, true); - TEST_ONE_SHIFT(0, 33, u32, 0, true); - TEST_ONE_SHIFT(0, 32, int, 0, true); - TEST_ONE_SHIFT(0, 33, int, 0, true); - TEST_ONE_SHIFT(0, 32, s32, 0, true); - TEST_ONE_SHIFT(0, 33, s32, 0, true); - TEST_ONE_SHIFT(0, 64, u64, 0, true); - TEST_ONE_SHIFT(0, 65, u64, 0, true); - TEST_ONE_SHIFT(0, 64, s64, 0, true); - TEST_ONE_SHIFT(0, 65, s64, 0, true); - /* * Corner case: for unsigned types, we fail when we've shifted * through the entire width of bits. For signed types, we might @@ -490,9 +511,9 @@ static void overflow_shift_test(struct kunit *test) TEST_ONE_SHIFT(0, 31, s32, 0, false); TEST_ONE_SHIFT(0, 63, s64, 0, false); - kunit_info(test, "%d shift tests finished\n", count); -#undef TEST_ONE_SHIFT + kunit_info(test, "%d nonsense shift tests finished\n", count); } +#undef TEST_ONE_SHIFT /* * Deal with the various forms of allocator arguments. See comments above @@ -703,7 +724,10 @@ static struct kunit_case overflow_test_cases[] = { KUNIT_CASE(u32_u32__int_overflow_test), KUNIT_CASE(u8_u8__int_overflow_test), KUNIT_CASE(int_int__u8_overflow_test), - KUNIT_CASE(overflow_shift_test), + KUNIT_CASE(shift_sane_test), + KUNIT_CASE(shift_overflow_test), + KUNIT_CASE(shift_truncate_test), + KUNIT_CASE(shift_nonsense_test), KUNIT_CASE(overflow_allocation_test), KUNIT_CASE(overflow_size_helpers_test), {} From dfbafa70bde26c40615f8c538ce68dac82a64fb4 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 26 Aug 2022 11:04:43 -0700 Subject: [PATCH 05/46] string: Introduce strtomem() and strtomem_pad() One of the "legitimate" uses of strncpy() is copying a NUL-terminated string into a fixed-size non-NUL-terminated character array. To avoid the weaknesses and ambiguity of intent when using strncpy(), provide replacement functions that explicitly distinguish between trailing padding and not, and require the destination buffer size be discoverable by the compiler. For example: struct obj { int foo; char small[4] __nonstring; char big[8] __nonstring; int bar; }; struct obj p; /* This will truncate to 4 chars with no trailing NUL */ strncpy(p.small, "hello", sizeof(p.small)); /* p.small contains 'h', 'e', 'l', 'l' */ /* This will NUL pad to 8 chars. */ strncpy(p.big, "hello", sizeof(p.big)); /* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */ When the "__nonstring" attributes are missing, the intent of the programmer becomes ambiguous for whether the lack of a trailing NUL in the p.small copy is a bug. Additionally, it's not clear whether the trailing padding in the p.big copy is _needed_. Both cases become unambiguous with: strtomem(p.small, "hello"); strtomem_pad(p.big, "hello", 0); See also https://github.com/KSPP/linux/issues/90 Expand the memcpy KUnit tests to include these functions. Cc: Wolfram Sang Cc: Nick Desaulniers Cc: Geert Uytterhoeven Cc: Guenter Roeck Signed-off-by: Kees Cook --- Documentation/process/deprecated.rst | 11 ++++-- include/linux/fortify-string.h | 32 +++++++++++++++ include/linux/string.h | 43 ++++++++++++++++++++ lib/memcpy_kunit.c | 59 ++++++++++++++++++++++++++-- 4 files changed, 137 insertions(+), 8 deletions(-) diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst index a6e36d9c3d14b..c8fd53a11a207 100644 --- a/Documentation/process/deprecated.rst +++ b/Documentation/process/deprecated.rst @@ -138,17 +138,20 @@ be NUL terminated. This can lead to various linear read overflows and other misbehavior due to the missing termination. It also NUL-pads the destination buffer if the source contents are shorter than the destination buffer size, which may be a needless performance penalty -for callers using only NUL-terminated strings. The safe replacement is +for callers using only NUL-terminated strings. + +When the destination is required to be NUL-terminated, the replacement is strscpy(), though care must be given to any cases where the return value of strncpy() was used, since strscpy() does not return a pointer to the destination, but rather a count of non-NUL bytes copied (or negative errno when it truncates). Any cases still needing NUL-padding should instead use strscpy_pad(). -If a caller is using non-NUL-terminated strings, strncpy() can -still be used, but destinations should be marked with the `__nonstring +If a caller is using non-NUL-terminated strings, strtomem() should be +used, and the destinations should be marked with the `__nonstring `_ -attribute to avoid future compiler warnings. +attribute to avoid future compiler warnings. For cases still needing +NUL-padding, strtomem_pad() can be used. strlcpy() --------- diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index 3b401fa0f3746..8e8c2c87b1d50 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -77,6 +77,38 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) #define POS __pass_object_size(1) #define POS0 __pass_object_size(0) +/** + * strncpy - Copy a string to memory with non-guaranteed NUL padding + * + * @p: pointer to destination of copy + * @q: pointer to NUL-terminated source string to copy + * @size: bytes to write at @p + * + * If strlen(@q) >= @size, the copy of @q will stop after @size bytes, + * and @p will NOT be NUL-terminated + * + * If strlen(@q) < @size, following the copy of @q, trailing NUL bytes + * will be written to @p until @size total bytes have been written. + * + * Do not use this function. While FORTIFY_SOURCE tries to avoid + * over-reads of @q, it cannot defend against writing unterminated + * results to @p. Using strncpy() remains ambiguous and fragile. + * Instead, please choose an alternative, so that the expectation + * of @p's contents is unambiguous: + * + * +--------------------+-----------------+------------+ + * | @p needs to be: | padded to @size | not padded | + * +====================+=================+============+ + * | NUL-terminated | strscpy_pad() | strscpy() | + * +--------------------+-----------------+------------+ + * | not NUL-terminated | strtomem_pad() | strtomem() | + * +--------------------+-----------------+------------+ + * + * Note strscpy*()'s differing return values for detecting truncation, + * and strtomem*()'s expectation that the destination is marked with + * __nonstring when it is a character array. + * + */ __FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3) char *strncpy(char * const POS p, const char *q, __kernel_size_t size) { diff --git a/include/linux/string.h b/include/linux/string.h index 61ec7e4f6311a..cf7607b321027 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -260,6 +260,49 @@ static inline const char *kbasename(const char *path) void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count, int pad); +/** + * strtomem_pad - Copy NUL-terminated string to non-NUL-terminated buffer + * + * @dest: Pointer of destination character array (marked as __nonstring) + * @src: Pointer to NUL-terminated string + * @pad: Padding character to fill any remaining bytes of @dest after copy + * + * This is a replacement for strncpy() uses where the destination is not + * a NUL-terminated string, but with bounds checking on the source size, and + * an explicit padding character. If padding is not required, use strtomem(). + * + * Note that the size of @dest is not an argument, as the length of @dest + * must be discoverable by the compiler. + */ +#define strtomem_pad(dest, src, pad) do { \ + const size_t _dest_len = __builtin_object_size(dest, 1); \ + \ + BUILD_BUG_ON(!__builtin_constant_p(_dest_len) || \ + _dest_len == (size_t)-1); \ + memcpy_and_pad(dest, _dest_len, src, strnlen(src, _dest_len), pad); \ +} while (0) + +/** + * strtomem - Copy NUL-terminated string to non-NUL-terminated buffer + * + * @dest: Pointer of destination character array (marked as __nonstring) + * @src: Pointer to NUL-terminated string + * + * This is a replacement for strncpy() uses where the destination is not + * a NUL-terminated string, but with bounds checking on the source size, and + * without trailing padding. If padding is required, use strtomem_pad(). + * + * Note that the size of @dest is not an argument, as the length of @dest + * must be discoverable by the compiler. + */ +#define strtomem(dest, src) do { \ + const size_t _dest_len = __builtin_object_size(dest, 1); \ + \ + BUILD_BUG_ON(!__builtin_constant_p(_dest_len) || \ + _dest_len == (size_t)-1); \ + memcpy(dest, src, min(_dest_len, strnlen(src, _dest_len))); \ +} while (0) + /** * memset_after - Set a value after a struct member to the end of a struct * diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c index 62f8ffcbbaa35..d22fa3838ee97 100644 --- a/lib/memcpy_kunit.c +++ b/lib/memcpy_kunit.c @@ -29,9 +29,8 @@ struct some_bytes { }; #define check(instance, v) do { \ - int i; \ BUILD_BUG_ON(sizeof(instance.data) != 32); \ - for (i = 0; i < sizeof(instance.data); i++) { \ + for (size_t i = 0; i < sizeof(instance.data); i++) { \ KUNIT_ASSERT_EQ_MSG(test, instance.data[i], v, \ "line %d: '%s' not initialized to 0x%02x @ %d (saw 0x%02x)\n", \ __LINE__, #instance, v, i, instance.data[i]); \ @@ -39,9 +38,8 @@ struct some_bytes { } while (0) #define compare(name, one, two) do { \ - int i; \ BUILD_BUG_ON(sizeof(one) != sizeof(two)); \ - for (i = 0; i < sizeof(one); i++) { \ + for (size_t i = 0; i < sizeof(one); i++) { \ KUNIT_EXPECT_EQ_MSG(test, one.data[i], two.data[i], \ "line %d: %s.data[%d] (0x%02x) != %s.data[%d] (0x%02x)\n", \ __LINE__, #one, i, one.data[i], #two, i, two.data[i]); \ @@ -272,10 +270,63 @@ static void memset_test(struct kunit *test) #undef TEST_OP } +static void strtomem_test(struct kunit *test) +{ + static const char input[] = "hi"; + static const char truncate[] = "this is too long"; + struct { + unsigned long canary1; + unsigned char output[sizeof(unsigned long)] __nonstring; + unsigned long canary2; + } wrap; + + memset(&wrap, 0xFF, sizeof(wrap)); + KUNIT_EXPECT_EQ_MSG(test, wrap.canary1, ULONG_MAX, + "bad initial canary value"); + KUNIT_EXPECT_EQ_MSG(test, wrap.canary2, ULONG_MAX, + "bad initial canary value"); + + /* Check unpadded copy leaves surroundings untouched. */ + strtomem(wrap.output, input); + KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX); + KUNIT_EXPECT_EQ(test, wrap.output[0], input[0]); + KUNIT_EXPECT_EQ(test, wrap.output[1], input[1]); + for (size_t i = 2; i < sizeof(wrap.output); i++) + KUNIT_EXPECT_EQ(test, wrap.output[i], 0xFF); + KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX); + + /* Check truncated copy leaves surroundings untouched. */ + memset(&wrap, 0xFF, sizeof(wrap)); + strtomem(wrap.output, truncate); + KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX); + for (size_t i = 0; i < sizeof(wrap.output); i++) + KUNIT_EXPECT_EQ(test, wrap.output[i], truncate[i]); + KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX); + + /* Check padded copy leaves only string padded. */ + memset(&wrap, 0xFF, sizeof(wrap)); + strtomem_pad(wrap.output, input, 0xAA); + KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX); + KUNIT_EXPECT_EQ(test, wrap.output[0], input[0]); + KUNIT_EXPECT_EQ(test, wrap.output[1], input[1]); + for (size_t i = 2; i < sizeof(wrap.output); i++) + KUNIT_EXPECT_EQ(test, wrap.output[i], 0xAA); + KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX); + + /* Check truncated padded copy has no padding. */ + memset(&wrap, 0xFF, sizeof(wrap)); + strtomem(wrap.output, truncate); + KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX); + for (size_t i = 0; i < sizeof(wrap.output); i++) + KUNIT_EXPECT_EQ(test, wrap.output[i], truncate[i]); + KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX); +} + static struct kunit_case memcpy_test_cases[] = { KUNIT_CASE(memset_test), KUNIT_CASE(memcpy_test), KUNIT_CASE(memmove_test), + KUNIT_CASE(strtomem_test), {} }; From d07c0acb4f41cc42a0d97530946965b3e4fa68c1 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 2 Sep 2022 13:02:26 -0700 Subject: [PATCH 06/46] fortify: Fix __compiletime_strlen() under UBSAN_BOUNDS_LOCAL With CONFIG_FORTIFY=y and CONFIG_UBSAN_LOCAL_BOUNDS=y enabled, we observe a runtime panic while running Android's Compatibility Test Suite's (CTS) android.hardware.input.cts.tests. This is stemming from a strlen() call in hidinput_allocate(). __compiletime_strlen() is implemented in terms of __builtin_object_size(), then does an array access to check for NUL-termination. A quirk of __builtin_object_size() is that for strings whose values are runtime dependent, __builtin_object_size(str, 1 or 0) returns the maximum size of possible values when those sizes are determinable at compile time. Example: static const char *v = "FOO BAR"; static const char *y = "FOO BA"; unsigned long x (int z) { // Returns 8, which is: // max(__builtin_object_size(v, 1), __builtin_object_size(y, 1)) return __builtin_object_size(z ? v : y, 1); } So when FORTIFY_SOURCE is enabled, the current implementation of __compiletime_strlen() will try to access beyond the end of y at runtime using the size of v. Mixed with UBSAN_LOCAL_BOUNDS we get a fault. hidinput_allocate() has a local C string whose value is control flow dependent on a switch statement, so __builtin_object_size(str, 1) evaluates to the maximum string length, making all other cases fault on the last character check. hidinput_allocate() could be cleaned up to avoid runtime calls to strlen() since the local variable can only have literal values, so there's no benefit to trying to fortify the strlen call site there. Perform a __builtin_constant_p() check against index 0 earlier in the macro to filter out the control-flow-dependant case. Add a KUnit test for checking the expected behavioral characteristics of FORTIFY_SOURCE internals. Cc: Nathan Chancellor Cc: Tom Rix Cc: Andrew Morton Cc: Vlastimil Babka Cc: "Steven Rostedt (Google)" Cc: David Gow Cc: Yury Norov Cc: Masami Hiramatsu Cc: Sander Vanheule Cc: linux-hardening@vger.kernel.org Cc: llvm@lists.linux.dev Reviewed-by: Nick Desaulniers Tested-by: Android Treehugger Robot Link: https://android-review.googlesource.com/c/kernel/common/+/2206839 Co-developed-by: Nick Desaulniers Signed-off-by: Nick Desaulniers Signed-off-by: Kees Cook --- include/linux/fortify-string.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index 8e8c2c87b1d50..be264091f7a7c 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -19,7 +19,8 @@ void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning(" unsigned char *__p = (unsigned char *)(p); \ size_t __ret = (size_t)-1; \ size_t __p_size = __builtin_object_size(p, 1); \ - if (__p_size != (size_t)-1) { \ + if (__p_size != (size_t)-1 && \ + __builtin_constant_p(*__p)) { \ size_t __p_len = __p_size - 1; \ if (__builtin_constant_p(__p[__p_len]) && \ __p[__p_len] == '\0') \ From 875bfd5276f31d09e811d31fca638b9f4d1205e8 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 2 Sep 2022 13:02:26 -0700 Subject: [PATCH 07/46] fortify: Add KUnit test for FORTIFY_SOURCE internals Add lib/fortify_kunit.c KUnit test for checking the expected behavioral characteristics of FORTIFY_SOURCE internals. Cc: Nick Desaulniers Cc: Nathan Chancellor Cc: Tom Rix Cc: Andrew Morton Cc: Vlastimil Babka Cc: "Steven Rostedt (Google)" Cc: Yury Norov Cc: Masami Hiramatsu Cc: Sander Vanheule Cc: linux-hardening@vger.kernel.org Cc: llvm@lists.linux.dev Reviewed-by: David Gow Signed-off-by: Kees Cook --- MAINTAINERS | 1 + lib/Kconfig.debug | 9 ++++++ lib/Makefile | 1 + lib/fortify_kunit.c | 77 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+) create mode 100644 lib/fortify_kunit.c diff --git a/MAINTAINERS b/MAINTAINERS index 9d7f64dc0efe8..640115472199e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8002,6 +8002,7 @@ L: linux-hardening@vger.kernel.org S: Supported T: git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/hardening F: include/linux/fortify-string.h +F: lib/fortify_kunit.c F: lib/test_fortify/* F: scripts/test_fortify.sh K: \b__NO_FORTIFY\b diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 36455953d3069..1f267c0ddffdf 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2542,6 +2542,15 @@ config STACKINIT_KUNIT_TEST CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF, or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. +config FORTIFY_KUNIT_TEST + tristate "Test fortified str*() and mem*() function internals at runtime" if !KUNIT_ALL_TESTS + depends on KUNIT && FORTIFY_SOURCE + default KUNIT_ALL_TESTS + help + Builds unit tests for checking internals of FORTIFY_SOURCE as used + by the str*() and mem*() family of functions. For testing runtime + traps of FORTIFY_SOURCE, see LKDTM's "FORTIFY_*" tests. + config TEST_UDELAY tristate "udelay test driver" help diff --git a/lib/Makefile b/lib/Makefile index f545140ed9e74..4ee1ceae945ac 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -381,6 +381,7 @@ obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_kunit.o 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 +obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c new file mode 100644 index 0000000000000..99bc0ea60d27a --- /dev/null +++ b/lib/fortify_kunit.c @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Runtime test cases for CONFIG_FORTIFY_SOURCE that aren't expected to + * Oops the kernel on success. (For those, see drivers/misc/lkdtm/fortify.c) + * + * For corner cases with UBSAN, try testing with: + * + * ./tools/testing/kunit/kunit.py run --arch=x86_64 \ + * --kconfig_add CONFIG_FORTIFY_SOURCE=y \ + * --kconfig_add CONFIG_UBSAN=y \ + * --kconfig_add CONFIG_UBSAN_TRAP=y \ + * --kconfig_add CONFIG_UBSAN_BOUNDS=y \ + * --kconfig_add CONFIG_UBSAN_LOCAL_BOUNDS=y \ + * --make_options LLVM=1 fortify + */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include +#include +#include + +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"; + +static void known_sizes_test(struct kunit *test) +{ + KUNIT_EXPECT_EQ(test, __compiletime_strlen("88888888"), 8); + KUNIT_EXPECT_EQ(test, __compiletime_strlen(array_of_10), 10); + KUNIT_EXPECT_EQ(test, __compiletime_strlen(ptr_of_11), 11); + + KUNIT_EXPECT_EQ(test, __compiletime_strlen(array_unknown), SIZE_MAX); + /* Externally defined and dynamically sized string pointer: */ + KUNIT_EXPECT_EQ(test, __compiletime_strlen(saved_command_line), SIZE_MAX); +} + +/* This is volatile so the optimizer can't perform DCE below. */ +static volatile int pick; + +/* Not inline to keep optimizer from figuring out which string we want. */ +static noinline size_t want_minus_one(int pick) +{ + const char *str; + + switch (pick) { + case 1: + str = "4444"; + break; + case 2: + str = "333"; + break; + default: + str = "1"; + break; + } + return __compiletime_strlen(str); +} + +static void control_flow_split_test(struct kunit *test) +{ + KUNIT_EXPECT_EQ(test, want_minus_one(pick), SIZE_MAX); +} + +static struct kunit_case fortify_test_cases[] = { + KUNIT_CASE(known_sizes_test), + KUNIT_CASE(control_flow_split_test), + {} +}; + +static struct kunit_suite fortify_test_suite = { + .name = "fortify", + .test_cases = fortify_test_cases, +}; + +kunit_test_suite(fortify_test_suite); + +MODULE_LICENSE("GPL"); From 311fb40aa0569abacc430b0d66ee41470803111f Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 2 Sep 2022 13:23:06 -0700 Subject: [PATCH 08/46] fortify: Use SIZE_MAX instead of (size_t)-1 Clean up uses of "(size_t)-1" in favor of SIZE_MAX. Cc: linux-hardening@vger.kernel.org Suggested-by: Nick Desaulniers Signed-off-by: Kees Cook --- include/linux/fortify-string.h | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index be264091f7a7c..e46af17d23d0d 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -3,6 +3,7 @@ #define _LINUX_FORTIFY_STRING_H_ #include +#include #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable #define __RENAME(x) __asm__(#x) @@ -17,9 +18,9 @@ void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning(" #define __compiletime_strlen(p) \ ({ \ unsigned char *__p = (unsigned char *)(p); \ - size_t __ret = (size_t)-1; \ + size_t __ret = SIZE_MAX; \ size_t __p_size = __builtin_object_size(p, 1); \ - if (__p_size != (size_t)-1 && \ + if (__p_size != SIZE_MAX && \ __builtin_constant_p(*__p)) { \ size_t __p_len = __p_size - 1; \ if (__builtin_constant_p(__p[__p_len]) && \ @@ -127,7 +128,7 @@ char *strcat(char * const POS p, const char *q) { size_t p_size = __builtin_object_size(p, 1); - if (p_size == (size_t)-1) + if (p_size == SIZE_MAX) return __underlying_strcat(p, q); if (strlcat(p, q, p_size) >= p_size) fortify_panic(__func__); @@ -142,7 +143,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size size_t ret; /* We can take compile-time actions when maxlen is const. */ - if (__builtin_constant_p(maxlen) && p_len != (size_t)-1) { + if (__builtin_constant_p(maxlen) && p_len != SIZE_MAX) { /* If p is const, we can use its compile-time-known len. */ if (maxlen >= p_size) return p_len; @@ -170,7 +171,7 @@ __kernel_size_t __fortify_strlen(const char * const POS p) size_t p_size = __builtin_object_size(p, 1); /* Give up if we don't know how large p is. */ - if (p_size == (size_t)-1) + if (p_size == SIZE_MAX) return __underlying_strlen(p); ret = strnlen(p, p_size); if (p_size <= ret) @@ -187,7 +188,7 @@ __FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, si size_t q_len; /* Full count of source string length. */ size_t len; /* Count of characters going into destination. */ - if (p_size == (size_t)-1 && q_size == (size_t)-1) + if (p_size == SIZE_MAX && q_size == SIZE_MAX) return __real_strlcpy(p, q, size); q_len = strlen(q); len = (q_len >= size) ? size - 1 : q_len; @@ -215,7 +216,7 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s 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) + if (p_size == SIZE_MAX && q_size == SIZE_MAX) return __real_strscpy(p, q, size); /* @@ -260,7 +261,7 @@ char *strncat(char * const POS p, const char * const POS q, __kernel_size_t coun size_t p_size = __builtin_object_size(p, 1); size_t q_size = __builtin_object_size(q, 1); - if (p_size == (size_t)-1 && q_size == (size_t)-1) + if (p_size == SIZE_MAX && q_size == SIZE_MAX) return __underlying_strncat(p, q, count); p_len = strlen(p); copy_len = strnlen(q, count); @@ -301,10 +302,10 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size, /* * 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 + * (The SIZE_MAX test is to optimize away checks where the buffer * lengths are unknown.) */ - if (p_size != (size_t)(-1) && p_size < size) + if (p_size != SIZE_MAX && p_size < size) fortify_panic("memset"); } @@ -395,11 +396,11 @@ __FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size, /* * 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 + * (The SIZE_MAX 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)) + if ((p_size != SIZE_MAX && p_size < size) || + (q_size != SIZE_MAX && q_size < size)) fortify_panic(func); } @@ -498,7 +499,7 @@ char *strcpy(char * const POS p, const char * const POS q) size_t size; /* If neither buffer size is known, immediately give up. */ - if (p_size == (size_t)-1 && q_size == (size_t)-1) + if (p_size == SIZE_MAX && q_size == SIZE_MAX) return __underlying_strcpy(p, q); size = strlen(q) + 1; /* Compile-time check for const size overflow. */ From 54d9469bc515dc5fcbc20eecbe19cea868b70d68 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 24 Jun 2021 15:39:26 -0700 Subject: [PATCH 09/46] fortify: Add run-time WARN for cross-field memcpy() 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 in the last 3 years, 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] There may be false positives in the kernel 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(), or some other refactoring. In order to make counting/investigating instances of added runtime checks easier, each instance includes the destination variable name as a WARN argument, prefixed with 'field "'. Therefore, on an x86_64 defconfig build, it is trivial to inspect the build 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 Simple cases where a destination buffer is known to be a dynamic size do not generate a WARN. For example: struct normal_flex_array { void *a; int b; u32 c; size_t array_size; u8 flex_array[]; }; struct normal_flex_array *instance; ... /* These will be ignored for run-time bounds checking. */ memcpy(instance, src, len); memcpy(instance->flex_array, src, len); However, 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 These "composite flexible array structure destination" cases will be need to be flushed out and addressed on a case-by-case basis. Regardless, for the general case of using memcpy() on flexible array destinations, future APIs will be created to handle common cases. Those can be used to migrate away from open-coded memcpy() so that proper error handling (instead of trapping) can be used. 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. Signed-off-by: Kees Cook --- include/linux/fortify-string.h | 70 ++++++++++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 3 deletions(-) diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index e46af17d23d0d..ff879efe94ed5 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -2,6 +2,7 @@ #ifndef _LINUX_FORTIFY_STRING_H_ #define _LINUX_FORTIFY_STRING_H_ +#include #include #include @@ -353,7 +354,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, @@ -402,16 +403,79 @@ __FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size, if ((p_size != SIZE_MAX && p_size < size) || (q_size != SIZE_MAX && q_size < size)) fortify_panic(func); + + /* + * Warn when writing beyond destination field size. + * + * We must ignore p_size_field == 0 for existing 0-element + * fake flexible arrays, until they are all converted to + * proper flexible arrays. + * + * 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_MAX && + 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) == SIZE_MAX + * - the size of flexible arrays in ptr's obj (by design, dynamic size): + * __builtin_object_size(ptr->flex_buf, 1) == SIZE_MAX + * - the size of ANY array at the end of ptr's obj (gcc and clang bug): + * __builtin_object_size(ptr->end_buf, 1) == SIZE_MAX + * 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. From 325bf6d84bad3fc641b94fad6e69c70e960fdf2e Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 31 Aug 2022 23:05:01 -0700 Subject: [PATCH 10/46] lkdtm: Update tests for memcpy() run-time warnings Clarify the LKDTM FORTIFY tests, and add tests for the mem*() family of functions, now that run-time checking is distinct. Cc: Arnd Bergmann Cc: Greg Kroah-Hartman Cc: Shuah Khan Cc: linux-kselftest@vger.kernel.org Signed-off-by: Kees Cook --- drivers/misc/lkdtm/fortify.c | 96 +++++++++++++++++++++---- tools/testing/selftests/lkdtm/tests.txt | 8 ++- 2 files changed, 88 insertions(+), 16 deletions(-) diff --git a/drivers/misc/lkdtm/fortify.c b/drivers/misc/lkdtm/fortify.c index 080293fa3c521..0159276656780 100644 --- a/drivers/misc/lkdtm/fortify.c +++ b/drivers/misc/lkdtm/fortify.c @@ -10,28 +10,31 @@ static volatile int fortify_scratch_space; -static void lkdtm_FORTIFIED_OBJECT(void) +static void lkdtm_FORTIFY_STR_OBJECT(void) { struct target { char a[10]; - } target[2] = {}; + int foo; + } target[3] = {}; /* * 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; + volatile int size = 20; + + pr_info("trying to strcmp() past the end of a struct\n"); - pr_info("trying to read past the end of a struct\n"); + strncpy(target[0].a, target[1].a, size); /* Store result to global to prevent the code from being eliminated */ - fortify_scratch_space = memcmp(&target[0], &target[1], size); + fortify_scratch_space = target[0].a[3]; - pr_err("FAIL: fortify did not block an object overread!\n"); + pr_err("FAIL: fortify did not block a strncpy() object write overflow!\n"); pr_expected_config(CONFIG_FORTIFY_SOURCE); } -static void lkdtm_FORTIFIED_SUBOBJECT(void) +static void lkdtm_FORTIFY_STR_MEMBER(void) { struct target { char a[10]; @@ -44,7 +47,7 @@ static void lkdtm_FORTIFIED_SUBOBJECT(void) strscpy(src, "over ten bytes", size); size = strlen(src) + 1; - pr_info("trying to strncpy past the end of a member of a struct\n"); + pr_info("trying to strncpy() past the end of a struct member...\n"); /* * strncpy(target.a, src, 20); will hit a compile error because the @@ -56,7 +59,72 @@ static void lkdtm_FORTIFIED_SUBOBJECT(void) /* Store result to global to prevent the code from being eliminated */ fortify_scratch_space = target.a[3]; - pr_err("FAIL: fortify did not block an sub-object overrun!\n"); + pr_err("FAIL: fortify did not block a strncpy() struct member write overflow!\n"); + pr_expected_config(CONFIG_FORTIFY_SOURCE); + + kfree(src); +} + +static void lkdtm_FORTIFY_MEM_OBJECT(void) +{ + int before[10]; + struct target { + char a[10]; + int foo; + } target = {}; + int after[10]; + /* + * 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 = 20; + + memset(before, 0, sizeof(before)); + memset(after, 0, sizeof(after)); + fortify_scratch_space = before[5]; + fortify_scratch_space = after[5]; + + pr_info("trying to memcpy() past the end of a struct\n"); + + pr_info("0: %zu\n", __builtin_object_size(&target, 0)); + pr_info("1: %zu\n", __builtin_object_size(&target, 1)); + pr_info("s: %d\n", size); + memcpy(&target, &before, size); + + /* Store result to global to prevent the code from being eliminated */ + fortify_scratch_space = target.a[3]; + + pr_err("FAIL: fortify did not block a memcpy() object write overflow!\n"); + pr_expected_config(CONFIG_FORTIFY_SOURCE); +} + +static void lkdtm_FORTIFY_MEM_MEMBER(void) +{ + struct target { + char a[10]; + char b[10]; + } target; + volatile int size = 20; + char *src; + + src = kmalloc(size, GFP_KERNEL); + strscpy(src, "over ten bytes", size); + size = strlen(src) + 1; + + pr_info("trying to memcpy() past the end of a struct member...\n"); + + /* + * strncpy(target.a, src, 20); will hit a compile error because the + * compiler knows at build time that target.a < 20 bytes. Use a + * volatile to force a runtime error. + */ + memcpy(target.a, src, size); + + /* Store result to global to prevent the code from being eliminated */ + fortify_scratch_space = target.a[3]; + + pr_err("FAIL: fortify did not block a memcpy() struct member write overflow!\n"); pr_expected_config(CONFIG_FORTIFY_SOURCE); kfree(src); @@ -67,7 +135,7 @@ static void lkdtm_FORTIFIED_SUBOBJECT(void) * strscpy and generate a panic because there is a write overflow (i.e. src * length is greater than dst length). */ -static void lkdtm_FORTIFIED_STRSCPY(void) +static void lkdtm_FORTIFY_STRSCPY(void) { char *src; char dst[5]; @@ -136,9 +204,11 @@ static void lkdtm_FORTIFIED_STRSCPY(void) } static struct crashtype crashtypes[] = { - CRASHTYPE(FORTIFIED_OBJECT), - CRASHTYPE(FORTIFIED_SUBOBJECT), - CRASHTYPE(FORTIFIED_STRSCPY), + CRASHTYPE(FORTIFY_STR_OBJECT), + CRASHTYPE(FORTIFY_STR_MEMBER), + CRASHTYPE(FORTIFY_MEM_OBJECT), + CRASHTYPE(FORTIFY_MEM_MEMBER), + CRASHTYPE(FORTIFY_STRSCPY), }; struct crashtype_category fortify_crashtypes = { diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 65e53eb0840ba..607b8d7e3ea34 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -75,7 +75,9 @@ USERCOPY_KERNEL STACKLEAK_ERASING OK: the rest of the thread stack is properly erased CFI_FORWARD_PROTO CFI_BACKWARD call trace:|ok: control flow unchanged -FORTIFIED_STRSCPY -FORTIFIED_OBJECT -FORTIFIED_SUBOBJECT +FORTIFY_STRSCPY detected buffer overflow +FORTIFY_STR_OBJECT detected buffer overflow +FORTIFY_STR_MEMBER detected buffer overflow +FORTIFY_MEM_OBJECT detected buffer overflow +FORTIFY_MEM_MEMBER detected field-spanning write PPC_SLB_MULTIHIT Recovered From ba38961a069b0d8d03b53218a6c29d737577d448 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 9 Feb 2022 16:32:24 -0800 Subject: [PATCH 11/46] um: Enable FORTIFY_SOURCE Enable FORTIFY_SOURCE so running Kunit tests can test fortified functions. Signed-off-by: Kees Cook Tested-by: David Gow Link: https://lore.kernel.org/r/20220210003224.773957-1-keescook@chromium.org --- arch/um/Kconfig | 1 + arch/um/os-Linux/user_syms.c | 1 + 2 files changed, 2 insertions(+) diff --git a/arch/um/Kconfig b/arch/um/Kconfig index 78de31ac1da7c..ad4ff3b0e91e5 100644 --- a/arch/um/Kconfig +++ b/arch/um/Kconfig @@ -6,6 +6,7 @@ config UML bool default y select ARCH_EPHEMERAL_INODES + select ARCH_HAS_FORTIFY_SOURCE select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_KCOV select ARCH_HAS_STRNCPY_FROM_USER diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c index cb667c9225abd..fd575ecbcaece 100644 --- a/arch/um/os-Linux/user_syms.c +++ b/arch/um/os-Linux/user_syms.c @@ -1,4 +1,5 @@ // SPDX-License-Identifier: GPL-2.0 +#define __NO_FORTIFY #include #include From aafc203bbad4bf6cf394a34ea698c2b0b8affae0 Mon Sep 17 00:00:00 2001 From: Matthias Kaehlcke Date: Mon, 29 Aug 2022 17:46:10 -0700 Subject: [PATCH 12/46] LoadPin: Fix Kconfig doc about format of file with verity digests The doc for CONFIG_SECURITY_LOADPIN_VERITY says that the file with verity digests must contain a comma separated list of digests. That was the case at some stage of the development, but was changed during the review process to one digest per line. Update the Kconfig doc accordingly. Reported-by: Jae Hoon Kim Signed-off-by: Matthias Kaehlcke Fixes: 3f805f8cc23b ("LoadPin: Enable loading from trusted dm-verity devices") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220829174557.1.I5d202d1344212a3800d9828f936df6511eb2d0d1@changeid --- security/loadpin/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/loadpin/Kconfig b/security/loadpin/Kconfig index 70e7985b2561c..994c1d9376e6b 100644 --- a/security/loadpin/Kconfig +++ b/security/loadpin/Kconfig @@ -33,4 +33,4 @@ config SECURITY_LOADPIN_VERITY on the LoadPin securityfs entry 'dm-verity'. The ioctl expects a file descriptor of a file with verity digests as parameter. The file must be located on the pinned root and - contain a comma separated list of digests. + contain one digest per line. From 916ef6232cc4b84db7082b4c3d3cf1753d9462ba Mon Sep 17 00:00:00 2001 From: Matthias Kaehlcke Date: Wed, 7 Sep 2022 13:30:58 -0700 Subject: [PATCH 13/46] dm: verity-loadpin: Only trust verity targets with enforcement Verity targets can be configured to ignore corrupted data blocks. LoadPin must only trust verity targets that are configured to perform some kind of enforcement when data corruption is detected, like returning an error, restarting the system or triggering a panic. Fixes: b6c1c5745ccc ("dm: Add verity helpers for LoadPin") Reported-by: Sarthak Kukreti Signed-off-by: Matthias Kaehlcke Reviewed-by: Sarthak Kukreti Cc: stable@vger.kernel.org Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220907133055.1.Ic8a1dafe960dc0f8302e189642bc88ebb785d274@changeid --- drivers/md/dm-verity-loadpin.c | 8 ++++++++ drivers/md/dm-verity-target.c | 16 ++++++++++++++++ drivers/md/dm-verity.h | 1 + 3 files changed, 25 insertions(+) diff --git a/drivers/md/dm-verity-loadpin.c b/drivers/md/dm-verity-loadpin.c index 387ec43aef728..4f78cc55c2514 100644 --- a/drivers/md/dm-verity-loadpin.c +++ b/drivers/md/dm-verity-loadpin.c @@ -14,6 +14,7 @@ LIST_HEAD(dm_verity_loadpin_trusted_root_digests); static bool is_trusted_verity_target(struct dm_target *ti) { + int verity_mode; u8 *root_digest; unsigned int digest_size; struct dm_verity_loadpin_trusted_root_digest *trd; @@ -22,6 +23,13 @@ static bool is_trusted_verity_target(struct dm_target *ti) if (!dm_is_verity_target(ti)) return false; + verity_mode = dm_verity_get_mode(ti); + + if ((verity_mode != DM_VERITY_MODE_EIO) && + (verity_mode != DM_VERITY_MODE_RESTART) && + (verity_mode != DM_VERITY_MODE_PANIC)) + return false; + if (dm_verity_get_root_digest(ti, &root_digest, &digest_size)) return false; diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c index 94b6cb599db4f..8a00cc42e4985 100644 --- a/drivers/md/dm-verity-target.c +++ b/drivers/md/dm-verity-target.c @@ -1446,6 +1446,22 @@ bool dm_is_verity_target(struct dm_target *ti) return ti->type->module == THIS_MODULE; } +/* + * Get the verity mode (error behavior) of a verity target. + * + * Returns the verity mode of the target, or -EINVAL if 'ti' is not a verity + * target. + */ +int dm_verity_get_mode(struct dm_target *ti) +{ + struct dm_verity *v = ti->private; + + if (!dm_is_verity_target(ti)) + return -EINVAL; + + return v->mode; +} + /* * Get the root digest of a verity target. * diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h index 45455de1b4bc5..98f306ec6a33d 100644 --- a/drivers/md/dm-verity.h +++ b/drivers/md/dm-verity.h @@ -134,6 +134,7 @@ extern int verity_hash_for_block(struct dm_verity *v, struct dm_verity_io *io, sector_t block, u8 *digest, bool *is_zero); extern bool dm_is_verity_target(struct dm_target *ti); +extern int dm_verity_get_mode(struct dm_target *ti); extern int dm_verity_get_root_digest(struct dm_target *ti, u8 **root_digest, unsigned int *digest_size); From 6e42aec7c75947e0d6b38400628f171364eb8231 Mon Sep 17 00:00:00 2001 From: Matthias Kaehlcke Date: Tue, 6 Sep 2022 18:18:12 -0700 Subject: [PATCH 14/46] LoadPin: Require file with verity root digests to have a header LoadPin expects the file with trusted verity root digests to be an ASCII file with one digest (hex value) per line. A pinned root could contain files that meet these format requirements, even though the hex values don't represent trusted root digests. Add a new requirement to the file format which consists in the first line containing a fixed string. This prevents attackers from feeding files with an otherwise valid format to LoadPin. Suggested-by: Sarthak Kukreti Signed-off-by: Matthias Kaehlcke Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220906181725.1.I3f51d1bb0014e5a5951be4ad3c5ad7c7ca1dfc32@changeid --- security/loadpin/Kconfig | 7 ++++++- security/loadpin/loadpin.c | 16 +++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/security/loadpin/Kconfig b/security/loadpin/Kconfig index 994c1d9376e6b..6724eaba3d364 100644 --- a/security/loadpin/Kconfig +++ b/security/loadpin/Kconfig @@ -33,4 +33,9 @@ config SECURITY_LOADPIN_VERITY on the LoadPin securityfs entry 'dm-verity'. The ioctl expects a file descriptor of a file with verity digests as parameter. The file must be located on the pinned root and - contain one digest per line. + start with the line: + + # LOADPIN_TRUSTED_VERITY_ROOT_DIGESTS + + This is followed by the verity digests, with one digest per + line. diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c index 44521582dcba2..de41621f4998e 100644 --- a/security/loadpin/loadpin.c +++ b/security/loadpin/loadpin.c @@ -21,6 +21,8 @@ #include #include +#define VERITY_DIGEST_FILE_HEADER "# LOADPIN_TRUSTED_VERITY_ROOT_DIGESTS" + static void report_load(const char *origin, struct file *file, char *operation) { char *cmdline, *pathname; @@ -292,9 +294,21 @@ static int read_trusted_verity_root_digests(unsigned int fd) p = strim(data); while ((d = strsep(&p, "\n")) != NULL) { - int len = strlen(d); + int len; struct dm_verity_loadpin_trusted_root_digest *trd; + if (d == data) { + /* first line, validate header */ + if (strcmp(d, VERITY_DIGEST_FILE_HEADER)) { + rc = -EPROTO; + goto err; + } + + continue; + } + + len = strlen(d); + if (len % 2) { rc = -EPROTO; goto err; From 98388bda6a99d76309f81584f2bc0d773bdf8b35 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Wed, 7 Sep 2022 11:03:29 -0700 Subject: [PATCH 15/46] lib: Improve the is_signed_type() kunit test Since the definition of is_signed_type() has been moved from to , include the latter header file instead of the former. Additionally, add a test for the type 'char'. Cc: Isabella Basso Cc: Rasmus Villemoes Signed-off-by: Bart Van Assche Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220907180329.3825417-1-bvanassche@acm.org --- lib/is_signed_type_kunit.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/is_signed_type_kunit.c b/lib/is_signed_type_kunit.c index f2eedb1f09356..207207522925e 100644 --- a/lib/is_signed_type_kunit.c +++ b/lib/is_signed_type_kunit.c @@ -5,7 +5,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include -#include +#include enum unsigned_enum { constant_a = 3, @@ -21,6 +21,11 @@ static void is_signed_type_test(struct kunit *test) KUNIT_EXPECT_EQ(test, is_signed_type(bool), false); KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true); KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false); +#ifdef __CHAR_UNSIGNED__ + KUNIT_EXPECT_EQ(test, is_signed_type(char), false); +#else + KUNIT_EXPECT_EQ(test, is_signed_type(char), true); +#endif KUNIT_EXPECT_EQ(test, is_signed_type(int), true); KUNIT_EXPECT_EQ(test, is_signed_type(unsigned int), false); KUNIT_EXPECT_EQ(test, is_signed_type(long), true); From 66cb2a36a96f6facbcb4ef1db967b8e9ea6910fe Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 7 Sep 2022 16:27:06 -0700 Subject: [PATCH 16/46] kunit/memcpy: Avoid pathological compile-time string size The memcpy() KUnit tests are trying to sanity-check run-time behaviors, but tripped compile-time warnings about a pathological condition of a too-small buffer being used for input. Avoid this by explicitly resizing the buffer, but leaving the string short. Avoid the following warning: lib/memcpy_kunit.c: In function 'strtomem_test': include/linux/string.h:303:42: warning: 'strnlen' specified bound 4 exceeds source size 3 [-Wstringop-overread] 303 | memcpy(dest, src, min(_dest_len, strnlen(src, _dest_len))); \ include/linux/minmax.h:32:39: note: in definition of macro '__cmp_once' 32 | typeof(y) unique_y = (y); \ | ^ include/linux/minmax.h:45:25: note: in expansion of macro '__careful_cmp' 45 | #define min(x, y) __careful_cmp(x, y, <) | ^~~~~~~~~~~~~ include/linux/string.h:303:27: note: in expansion of macro 'min' 303 | memcpy(dest, src, min(_dest_len, strnlen(src, _dest_len))); \ | ^~~ lib/memcpy_kunit.c:290:9: note: in expansion of macro 'strtomem' 290 | strtomem(wrap.output, input); | ^~~~~~~~ lib/memcpy_kunit.c:275:27: note: source object allocated here 275 | static const char input[] = "hi"; | ^~~~~ Reported-by: kernel test robot Link: https://lore.kernel.org/linux-mm/202209070728.o3stvgVt-lkp@intel.com Fixes: dfbafa70bde2 ("string: Introduce strtomem() and strtomem_pad()") Signed-off-by: Kees Cook --- lib/memcpy_kunit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c index d22fa3838ee97..2b5cc70ac53fc 100644 --- a/lib/memcpy_kunit.c +++ b/lib/memcpy_kunit.c @@ -272,7 +272,7 @@ static void memset_test(struct kunit *test) static void strtomem_test(struct kunit *test) { - static const char input[] = "hi"; + static const char input[sizeof(unsigned long)] = "hi"; static const char truncate[] = "this is too long"; struct { unsigned long canary1; From c5783af354688b24abd359f7086c282ec74de993 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 7 Sep 2022 16:40:44 -0700 Subject: [PATCH 17/46] sh: machvec: Use char[] for section boundaries As done for other sections, define the extern as a character array, which relaxes many of the compiler-time object size checks, which would otherwise assume it's a single long. Solves the following build error: arch/sh/kernel/machvec.c: error: array subscript 'struct sh_machine_vector[0]' is partly outside array bounds of 'long int[1]' [-Werror=array-bounds]: => 105:33 Cc: Yoshinori Sato Cc: Rich Felker Cc: linux-sh@vger.kernel.org Reported-by: Geert Uytterhoeven Link: https://lore.kernel.org/lkml/alpine.DEB.2.22.394.2209050944290.964530@ramsan.of.borg/ Fixes: 9655ad03af2d ("sh: Fixup machvec support.") Reviewed-by: Geert Uytterhoeven Reviewed-by: Gustavo A. R. Silva Acked-by: Rich Felker Signed-off-by: Kees Cook --- arch/sh/include/asm/sections.h | 2 +- arch/sh/kernel/machvec.c | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/sh/include/asm/sections.h b/arch/sh/include/asm/sections.h index 8edb824049b9e..0cb0ca149ac34 100644 --- a/arch/sh/include/asm/sections.h +++ b/arch/sh/include/asm/sections.h @@ -4,7 +4,7 @@ #include -extern long __machvec_start, __machvec_end; +extern char __machvec_start[], __machvec_end[]; extern char __uncached_start, __uncached_end; extern char __start_eh_frame[], __stop_eh_frame[]; diff --git a/arch/sh/kernel/machvec.c b/arch/sh/kernel/machvec.c index d606679a211e1..57efaf5b82ae0 100644 --- a/arch/sh/kernel/machvec.c +++ b/arch/sh/kernel/machvec.c @@ -20,8 +20,8 @@ #define MV_NAME_SIZE 32 #define for_each_mv(mv) \ - for ((mv) = (struct sh_machine_vector *)&__machvec_start; \ - (mv) && (unsigned long)(mv) < (unsigned long)&__machvec_end; \ + for ((mv) = (struct sh_machine_vector *)__machvec_start; \ + (mv) && (unsigned long)(mv) < (unsigned long)__machvec_end; \ (mv)++) static struct sh_machine_vector * __init get_mv_byname(const char *name) @@ -87,8 +87,8 @@ void __init sh_mv_setup(void) if (!machvec_selected) { unsigned long machvec_size; - machvec_size = ((unsigned long)&__machvec_end - - (unsigned long)&__machvec_start); + machvec_size = ((unsigned long)__machvec_end - + (unsigned long)__machvec_start); /* * Sanity check for machvec section alignment. Ensure @@ -102,7 +102,7 @@ void __init sh_mv_setup(void) * vector (usually the only one) from .machvec.init. */ if (machvec_size >= sizeof(struct sh_machine_vector)) - sh_mv = *(struct sh_machine_vector *)&__machvec_start; + sh_mv = *(struct sh_machine_vector *)__machvec_start; } pr_notice("Booting machvec: %s\n", get_system_type()); From 06c1c49d0cd1d6cec5b78963109ba728e49e0063 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 13 Sep 2022 10:28:56 -0700 Subject: [PATCH 18/46] fortify: Adjust KUnit test for modular build A much better "unknown size" string pointer is available directly from struct test, so use that instead of a global that isn't shared with modules. Reported-by: Nathan Chancellor Link: https://lore.kernel.org/lkml/YyCOHOchVuE/E7vS@dev-arch.thelio-3990X Fixes: 875bfd5276f3 ("fortify: Add KUnit test for FORTIFY_SOURCE internals") Cc: linux-hardening@vger.kernel.org Build-tested-by: Nathan Chancellor Reviewed-by: David Gow Signed-off-by: Kees Cook --- lib/fortify_kunit.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c index 99bc0ea60d27a..409af07f340af 100644 --- a/lib/fortify_kunit.c +++ b/lib/fortify_kunit.c @@ -17,7 +17,6 @@ #include #include -#include static const char array_of_10[] = "this is 10"; static const char *ptr_of_11 = "this is 11!"; @@ -31,7 +30,7 @@ static void known_sizes_test(struct kunit *test) KUNIT_EXPECT_EQ(test, __compiletime_strlen(array_unknown), SIZE_MAX); /* Externally defined and dynamically sized string pointer: */ - KUNIT_EXPECT_EQ(test, __compiletime_strlen(saved_command_line), SIZE_MAX); + KUNIT_EXPECT_EQ(test, __compiletime_strlen(test->name), SIZE_MAX); } /* This is volatile so the optimizer can't perform DCE below. */ From 1b64daf413acd86c2c13f5443f6b4ef3690c8061 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 7 Sep 2022 15:41:03 -0700 Subject: [PATCH 19/46] ARM: decompressor: Include .data.rel.ro.local The .data.rel.ro.local section has the same semantics as .data.rel.ro here, so include it in the .rodata section of the decompressor. Additionally since the .printk_index section isn't usable outside of the core kernel, discard it in the decompressor. Avoids these warnings: arm-linux-gnueabi-ld: warning: orphan section `.data.rel.ro.local' from `arch/arm/boot/compressed/fdt_rw.o' being placed in section `.data.rel.ro.local' arm-linux-gnueabi-ld: warning: orphan section `.printk_index' from `arch/arm/boot/compressed/fdt_rw.o' being placed in section `.printk_index' Reported-by: kernel test robot Link: https://lore.kernel.org/linux-mm/202209080545.qMIVj7YM-lkp@intel.com Cc: Russell King Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Kees Cook --- arch/arm/boot/compressed/vmlinux.lds.S | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S index 1bcb68ac4b011..3fcb3e62dc569 100644 --- a/arch/arm/boot/compressed/vmlinux.lds.S +++ b/arch/arm/boot/compressed/vmlinux.lds.S @@ -23,6 +23,7 @@ SECTIONS *(.ARM.extab*) *(.note.*) *(.rel.*) + *(.printk_index) /* * Discard any r/w data - this produces a link error if we have any, * which is required for PIC decompression. Local data generates @@ -57,6 +58,7 @@ SECTIONS *(.rodata) *(.rodata.*) *(.data.rel.ro) + *(.data.rel.ro.*) } .piggydata : { *(.piggydata) From f143ff397a3f991e8b48542f77aad900845f436e Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:43 -0700 Subject: [PATCH 20/46] treewide: Filter out CC_FLAGS_CFI In preparation for removing CC_FLAGS_CFI from CC_FLAGS_LTO, explicitly filter out CC_FLAGS_CFI in all the makefiles where we currently filter out CC_FLAGS_LTO. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-2-samitolvanen@google.com --- arch/arm64/kernel/vdso/Makefile | 3 ++- arch/x86/entry/vdso/Makefile | 3 ++- drivers/firmware/efi/libstub/Makefile | 2 ++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile index bafbf78fab772..619e2dc7ee14c 100644 --- a/arch/arm64/kernel/vdso/Makefile +++ b/arch/arm64/kernel/vdso/Makefile @@ -40,7 +40,8 @@ ccflags-y += -DDISABLE_BRANCH_PROFILING -DBUILD_VDSO # kernel with CONFIG_WERROR enabled. CFLAGS_REMOVE_vgettimeofday.o = $(CC_FLAGS_FTRACE) -Os $(CC_FLAGS_SCS) \ $(RANDSTRUCT_CFLAGS) $(GCC_PLUGINS_CFLAGS) \ - $(CC_FLAGS_LTO) -Wmissing-prototypes -Wmissing-declarations + $(CC_FLAGS_LTO) $(CC_FLAGS_CFI) \ + -Wmissing-prototypes -Wmissing-declarations KASAN_SANITIZE := n KCSAN_SANITIZE := n UBSAN_SANITIZE := n diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile index 12f6c4d714cd6..381d3333b9960 100644 --- a/arch/x86/entry/vdso/Makefile +++ b/arch/x86/entry/vdso/Makefile @@ -91,7 +91,7 @@ ifneq ($(RETPOLINE_VDSO_CFLAGS),) endif endif -$(vobjs): KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO) $(RANDSTRUCT_CFLAGS) $(GCC_PLUGINS_CFLAGS) $(RETPOLINE_CFLAGS),$(KBUILD_CFLAGS)) $(CFL) +$(vobjs): KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO) $(CC_FLAGS_CFI) $(RANDSTRUCT_CFLAGS) $(GCC_PLUGINS_CFLAGS) $(RETPOLINE_CFLAGS),$(KBUILD_CFLAGS)) $(CFL) $(vobjs): KBUILD_AFLAGS += -DBUILD_VDSO # @@ -153,6 +153,7 @@ KBUILD_CFLAGS_32 := $(filter-out $(RANDSTRUCT_CFLAGS),$(KBUILD_CFLAGS_32)) KBUILD_CFLAGS_32 := $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS_32)) KBUILD_CFLAGS_32 := $(filter-out $(RETPOLINE_CFLAGS),$(KBUILD_CFLAGS_32)) KBUILD_CFLAGS_32 := $(filter-out $(CC_FLAGS_LTO),$(KBUILD_CFLAGS_32)) +KBUILD_CFLAGS_32 := $(filter-out $(CC_FLAGS_CFI),$(KBUILD_CFLAGS_32)) KBUILD_CFLAGS_32 += -m32 -msoft-float -mregparm=0 -fpic KBUILD_CFLAGS_32 += -fno-stack-protector KBUILD_CFLAGS_32 += $(call cc-option, -foptimize-sibling-calls) diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile index d0537573501e9..234fb29106222 100644 --- a/drivers/firmware/efi/libstub/Makefile +++ b/drivers/firmware/efi/libstub/Makefile @@ -39,6 +39,8 @@ KBUILD_CFLAGS := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \ # remove SCS flags from all objects in this directory KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_SCS), $(KBUILD_CFLAGS)) +# disable CFI +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_CFI), $(KBUILD_CFLAGS)) # disable LTO KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS)) From d0f9562ee43a135b941715d9e5e607de88898aca Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:44 -0700 Subject: [PATCH 21/46] scripts/kallsyms: Ignore __kcfi_typeid_ The compiler generates __kcfi_typeid_ symbols for annotating assembly functions with type information. These are constants that can be referenced in assembly code and are resolved by the linker. Ignore them in kallsyms. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-3-samitolvanen@google.com --- scripts/kallsyms.c | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c index f18e6dfc68c58..ccdf0c897f31c 100644 --- a/scripts/kallsyms.c +++ b/scripts/kallsyms.c @@ -119,6 +119,7 @@ static bool is_ignored_symbol(const char *name, char type) "__ThumbV7PILongThunk_", "__LA25Thunk_", /* mips lld */ "__microLA25Thunk_", + "__kcfi_typeid_", /* CFI type identifiers */ NULL }; From 9fca7115827b2e5f48d84e50bceb4edfd4cb6375 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:45 -0700 Subject: [PATCH 22/46] cfi: Remove CONFIG_CFI_CLANG_SHADOW In preparation to switching to -fsanitize=kcfi, remove support for the CFI module shadow that will no longer be needed. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-4-samitolvanen@google.com --- arch/Kconfig | 10 -- include/linux/cfi.h | 12 --- kernel/cfi.c | 236 +------------------------------------------ kernel/module/main.c | 15 --- 4 files changed, 1 insertion(+), 272 deletions(-) diff --git a/arch/Kconfig b/arch/Kconfig index 5dbf11a5ba4e8..5fd875e18c999 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -754,16 +754,6 @@ config CFI_CLANG https://clang.llvm.org/docs/ControlFlowIntegrity.html -config CFI_CLANG_SHADOW - bool "Use CFI shadow to speed up cross-module checks" - default y - depends on CFI_CLANG && MODULES - help - If you select this option, the kernel builds a fast look-up table of - CFI check functions in loaded modules to reduce performance overhead. - - If unsure, say Y. - config CFI_PERMISSIVE bool "Use CFI in permissive mode" depends on CFI_CLANG diff --git a/include/linux/cfi.h b/include/linux/cfi.h index c6dfc1ed06261..4ab51c0670074 100644 --- a/include/linux/cfi.h +++ b/include/linux/cfi.h @@ -20,18 +20,6 @@ extern void __cfi_check(uint64_t id, void *ptr, void *diag); #define __CFI_ADDRESSABLE(fn, __attr) \ const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn -#ifdef CONFIG_CFI_CLANG_SHADOW - -extern void cfi_module_add(struct module *mod, unsigned long base_addr); -extern void cfi_module_remove(struct module *mod, unsigned long base_addr); - -#else - -static inline void cfi_module_add(struct module *mod, unsigned long base_addr) {} -static inline void cfi_module_remove(struct module *mod, unsigned long base_addr) {} - -#endif /* CONFIG_CFI_CLANG_SHADOW */ - #else /* !CONFIG_CFI_CLANG */ #ifdef CONFIG_X86_KERNEL_IBT diff --git a/kernel/cfi.c b/kernel/cfi.c index 2046276ee2348..e8bc1b370edc3 100644 --- a/kernel/cfi.c +++ b/kernel/cfi.c @@ -32,237 +32,6 @@ static inline void handle_cfi_failure(void *ptr) } #ifdef CONFIG_MODULES -#ifdef CONFIG_CFI_CLANG_SHADOW -/* - * Index type. A 16-bit index can address at most (2^16)-2 pages (taking - * into account SHADOW_INVALID), i.e. ~256M with 4k pages. - */ -typedef u16 shadow_t; -#define SHADOW_INVALID ((shadow_t)~0UL) - -struct cfi_shadow { - /* Page index for the beginning of the shadow */ - unsigned long base; - /* An array of __cfi_check locations (as indices to the shadow) */ - shadow_t shadow[1]; -} __packed; - -/* - * The shadow covers ~128M from the beginning of the module region. If - * the region is larger, we fall back to __module_address for the rest. - */ -#define __SHADOW_RANGE (_UL(SZ_128M) >> PAGE_SHIFT) - -/* The in-memory size of struct cfi_shadow, always at least one page */ -#define __SHADOW_PAGES ((__SHADOW_RANGE * sizeof(shadow_t)) >> PAGE_SHIFT) -#define SHADOW_PAGES max(1UL, __SHADOW_PAGES) -#define SHADOW_SIZE (SHADOW_PAGES << PAGE_SHIFT) - -/* The actual size of the shadow array, minus metadata */ -#define SHADOW_ARR_SIZE (SHADOW_SIZE - offsetof(struct cfi_shadow, shadow)) -#define SHADOW_ARR_SLOTS (SHADOW_ARR_SIZE / sizeof(shadow_t)) - -static DEFINE_MUTEX(shadow_update_lock); -static struct cfi_shadow __rcu *cfi_shadow __read_mostly; - -/* Returns the index in the shadow for the given address */ -static inline int ptr_to_shadow(const struct cfi_shadow *s, unsigned long ptr) -{ - unsigned long index; - unsigned long page = ptr >> PAGE_SHIFT; - - if (unlikely(page < s->base)) - return -1; /* Outside of module area */ - - index = page - s->base; - - if (index >= SHADOW_ARR_SLOTS) - return -1; /* Cannot be addressed with shadow */ - - return (int)index; -} - -/* Returns the page address for an index in the shadow */ -static inline unsigned long shadow_to_ptr(const struct cfi_shadow *s, - int index) -{ - if (unlikely(index < 0 || index >= SHADOW_ARR_SLOTS)) - return 0; - - return (s->base + index) << PAGE_SHIFT; -} - -/* Returns the __cfi_check function address for the given shadow location */ -static inline unsigned long shadow_to_check_fn(const struct cfi_shadow *s, - int index) -{ - if (unlikely(index < 0 || index >= SHADOW_ARR_SLOTS)) - return 0; - - if (unlikely(s->shadow[index] == SHADOW_INVALID)) - return 0; - - /* __cfi_check is always page aligned */ - return (s->base + s->shadow[index]) << PAGE_SHIFT; -} - -static void prepare_next_shadow(const struct cfi_shadow __rcu *prev, - struct cfi_shadow *next) -{ - int i, index, check; - - /* Mark everything invalid */ - memset(next->shadow, 0xFF, SHADOW_ARR_SIZE); - - if (!prev) - return; /* No previous shadow */ - - /* If the base address didn't change, an update is not needed */ - if (prev->base == next->base) { - memcpy(next->shadow, prev->shadow, SHADOW_ARR_SIZE); - return; - } - - /* Convert the previous shadow to the new address range */ - for (i = 0; i < SHADOW_ARR_SLOTS; ++i) { - if (prev->shadow[i] == SHADOW_INVALID) - continue; - - index = ptr_to_shadow(next, shadow_to_ptr(prev, i)); - if (index < 0) - continue; - - check = ptr_to_shadow(next, - shadow_to_check_fn(prev, prev->shadow[i])); - if (check < 0) - continue; - - next->shadow[index] = (shadow_t)check; - } -} - -static void add_module_to_shadow(struct cfi_shadow *s, struct module *mod, - unsigned long min_addr, unsigned long max_addr) -{ - int check_index; - unsigned long check = (unsigned long)mod->cfi_check; - unsigned long ptr; - - if (unlikely(!PAGE_ALIGNED(check))) { - pr_warn("cfi: not using shadow for module %s\n", mod->name); - return; - } - - check_index = ptr_to_shadow(s, check); - if (check_index < 0) - return; /* Module not addressable with shadow */ - - /* For each page, store the check function index in the shadow */ - for (ptr = min_addr; ptr <= max_addr; ptr += PAGE_SIZE) { - int index = ptr_to_shadow(s, ptr); - - if (index >= 0) { - /* Each page must only contain one module */ - WARN_ON_ONCE(s->shadow[index] != SHADOW_INVALID); - s->shadow[index] = (shadow_t)check_index; - } - } -} - -static void remove_module_from_shadow(struct cfi_shadow *s, struct module *mod, - unsigned long min_addr, unsigned long max_addr) -{ - unsigned long ptr; - - for (ptr = min_addr; ptr <= max_addr; ptr += PAGE_SIZE) { - int index = ptr_to_shadow(s, ptr); - - if (index >= 0) - s->shadow[index] = SHADOW_INVALID; - } -} - -typedef void (*update_shadow_fn)(struct cfi_shadow *, struct module *, - unsigned long min_addr, unsigned long max_addr); - -static void update_shadow(struct module *mod, unsigned long base_addr, - update_shadow_fn fn) -{ - struct cfi_shadow *prev; - struct cfi_shadow *next; - unsigned long min_addr, max_addr; - - next = vmalloc(SHADOW_SIZE); - - mutex_lock(&shadow_update_lock); - prev = rcu_dereference_protected(cfi_shadow, - mutex_is_locked(&shadow_update_lock)); - - if (next) { - next->base = base_addr >> PAGE_SHIFT; - prepare_next_shadow(prev, next); - - min_addr = (unsigned long)mod->core_layout.base; - max_addr = min_addr + mod->core_layout.text_size; - fn(next, mod, min_addr & PAGE_MASK, max_addr & PAGE_MASK); - - set_memory_ro((unsigned long)next, SHADOW_PAGES); - } - - rcu_assign_pointer(cfi_shadow, next); - mutex_unlock(&shadow_update_lock); - synchronize_rcu(); - - if (prev) { - set_memory_rw((unsigned long)prev, SHADOW_PAGES); - vfree(prev); - } -} - -void cfi_module_add(struct module *mod, unsigned long base_addr) -{ - update_shadow(mod, base_addr, add_module_to_shadow); -} - -void cfi_module_remove(struct module *mod, unsigned long base_addr) -{ - update_shadow(mod, base_addr, remove_module_from_shadow); -} - -static inline cfi_check_fn ptr_to_check_fn(const struct cfi_shadow __rcu *s, - unsigned long ptr) -{ - int index; - - if (unlikely(!s)) - return NULL; /* No shadow available */ - - index = ptr_to_shadow(s, ptr); - if (index < 0) - return NULL; /* Cannot be addressed with shadow */ - - return (cfi_check_fn)shadow_to_check_fn(s, index); -} - -static inline cfi_check_fn find_shadow_check_fn(unsigned long ptr) -{ - cfi_check_fn fn; - - rcu_read_lock_sched_notrace(); - fn = ptr_to_check_fn(rcu_dereference_sched(cfi_shadow), ptr); - rcu_read_unlock_sched_notrace(); - - return fn; -} - -#else /* !CONFIG_CFI_CLANG_SHADOW */ - -static inline cfi_check_fn find_shadow_check_fn(unsigned long ptr) -{ - return NULL; -} - -#endif /* CONFIG_CFI_CLANG_SHADOW */ static inline cfi_check_fn find_module_check_fn(unsigned long ptr) { @@ -298,10 +67,7 @@ static inline cfi_check_fn find_check_fn(unsigned long ptr) ct_irq_enter(); } - if (IS_ENABLED(CONFIG_CFI_CLANG_SHADOW)) - fn = find_shadow_check_fn(ptr); - if (!fn) - fn = find_module_check_fn(ptr); + fn = find_module_check_fn(ptr); if (rcu_idle) { ct_irq_exit(); diff --git a/kernel/module/main.c b/kernel/module/main.c index a4e4d84b6f4e0..0228f44b58e5b 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -1144,8 +1144,6 @@ void __weak module_arch_freeing_init(struct module *mod) { } -static void cfi_cleanup(struct module *mod); - /* Free a module, remove from lists, etc. */ static void free_module(struct module *mod) { @@ -1190,9 +1188,6 @@ static void free_module(struct module *mod) mod->name); mutex_unlock(&module_mutex); - /* Clean up CFI for the module. */ - cfi_cleanup(mod); - /* This may be empty, but that's OK */ module_arch_freeing_init(mod); module_memfree(mod->init_layout.base); @@ -2875,7 +2870,6 @@ static int load_module(struct load_info *info, const char __user *uargs, synchronize_rcu(); kfree(mod->args); free_arch_cleanup: - cfi_cleanup(mod); module_arch_cleanup(mod); free_modinfo: free_modinfo(mod); @@ -2984,15 +2978,6 @@ static void cfi_init(struct module *mod) mod->exit = *exit; #endif rcu_read_unlock_sched(); - - cfi_module_add(mod, mod_tree.addr_min); -#endif -} - -static void cfi_cleanup(struct module *mod) -{ -#ifdef CONFIG_CFI_CLANG - cfi_module_remove(mod, mod_tree.addr_min); #endif } From 92efda8eb15295a07f450828b2db14485bfc09c2 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:46 -0700 Subject: [PATCH 23/46] cfi: Drop __CFI_ADDRESSABLE The __CFI_ADDRESSABLE macro is used for init_module and cleanup_module to ensure we have the address of the CFI jump table, and with CONFIG_X86_KERNEL_IBT to ensure LTO won't optimize away the symbols. As __CFI_ADDRESSABLE is no longer necessary with -fsanitize=kcfi, add a more flexible version of the __ADDRESSABLE macro and always ensure these symbols won't be dropped. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-5-samitolvanen@google.com --- include/linux/cfi.h | 20 -------------------- include/linux/compiler.h | 6 ++++-- include/linux/module.h | 4 ++-- 3 files changed, 6 insertions(+), 24 deletions(-) diff --git a/include/linux/cfi.h b/include/linux/cfi.h index 4ab51c0670074..2cdbc0fbd0ab6 100644 --- a/include/linux/cfi.h +++ b/include/linux/cfi.h @@ -13,26 +13,6 @@ typedef void (*cfi_check_fn)(uint64_t id, void *ptr, void *diag); /* Compiler-generated function in each module, and the kernel */ extern void __cfi_check(uint64_t id, void *ptr, void *diag); -/* - * Force the compiler to generate a CFI jump table entry for a function - * and store the jump table address to __cfi_jt_. - */ -#define __CFI_ADDRESSABLE(fn, __attr) \ - const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn - -#else /* !CONFIG_CFI_CLANG */ - -#ifdef CONFIG_X86_KERNEL_IBT - -#define __CFI_ADDRESSABLE(fn, __attr) \ - const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn - -#endif /* CONFIG_X86_KERNEL_IBT */ - #endif /* CONFIG_CFI_CLANG */ -#ifndef __CFI_ADDRESSABLE -#define __CFI_ADDRESSABLE(fn, __attr) -#endif - #endif /* _LINUX_CFI_H */ diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 7713d7bcdaea9..7bfafc69172af 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -221,9 +221,11 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, * otherwise, or eliminated entirely due to lack of references that are * visible to the compiler. */ -#define __ADDRESSABLE(sym) \ - static void * __section(".discard.addressable") __used \ +#define ___ADDRESSABLE(sym, __attrs) \ + static void * __used __attrs \ __UNIQUE_ID(__PASTE(__addressable_,sym)) = (void *)&sym; +#define __ADDRESSABLE(sym) \ + ___ADDRESSABLE(sym, __section(".discard.addressable")) /** * offset_to_ptr - convert a relative memory offset to an absolute pointer diff --git a/include/linux/module.h b/include/linux/module.h index 518296ea7f73a..8937b020ec042 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -132,7 +132,7 @@ extern void cleanup_module(void); { return initfn; } \ int init_module(void) __copy(initfn) \ __attribute__((alias(#initfn))); \ - __CFI_ADDRESSABLE(init_module, __initdata); + ___ADDRESSABLE(init_module, __initdata); /* This is only required if you want to be unloadable. */ #define module_exit(exitfn) \ @@ -140,7 +140,7 @@ extern void cleanup_module(void); { return exitfn; } \ void cleanup_module(void) __copy(exitfn) \ __attribute__((alias(#exitfn))); \ - __CFI_ADDRESSABLE(cleanup_module, __exitdata); + ___ADDRESSABLE(cleanup_module, __exitdata); #endif From 89245600941e4e0f87d77f60ee269b5e61ef4e49 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:47 -0700 Subject: [PATCH 24/46] cfi: Switch to -fsanitize=kcfi Switch from Clang's original forward-edge control-flow integrity implementation to -fsanitize=kcfi, which is better suited for the kernel, as it doesn't require LTO, doesn't use a jump table that requires altering function references, and won't break cross-module function address equality. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-6-samitolvanen@google.com --- Makefile | 13 +-- arch/Kconfig | 8 +- include/asm-generic/vmlinux.lds.h | 37 ++++---- include/linux/cfi.h | 29 +++++- include/linux/compiler-clang.h | 14 +-- include/linux/module.h | 6 +- kernel/cfi.c | 144 +++++++++++++++--------------- kernel/module/main.c | 35 +------- scripts/module.lds.S | 23 +---- 9 files changed, 133 insertions(+), 176 deletions(-) diff --git a/Makefile b/Makefile index a4f71076cacb8..43e08c9f95e98 100644 --- a/Makefile +++ b/Makefile @@ -921,18 +921,7 @@ export CC_FLAGS_LTO endif ifdef CONFIG_CFI_CLANG -CC_FLAGS_CFI := -fsanitize=cfi \ - -fsanitize-cfi-cross-dso \ - -fno-sanitize-cfi-canonical-jump-tables \ - -fno-sanitize-trap=cfi \ - -fno-sanitize-blacklist - -ifdef CONFIG_CFI_PERMISSIVE -CC_FLAGS_CFI += -fsanitize-recover=cfi -endif - -# If LTO flags are filtered out, we must also filter out CFI. -CC_FLAGS_LTO += $(CC_FLAGS_CFI) +CC_FLAGS_CFI := -fsanitize=kcfi KBUILD_CFLAGS += $(CC_FLAGS_CFI) export CC_FLAGS_CFI endif diff --git a/arch/Kconfig b/arch/Kconfig index 5fd875e18c999..1c1eca0c0019d 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -738,11 +738,13 @@ config ARCH_SUPPORTS_CFI_CLANG An architecture should select this option if it can support Clang's Control-Flow Integrity (CFI) checking. +config ARCH_USES_CFI_TRAPS + bool + config CFI_CLANG bool "Use Clang's Control Flow Integrity (CFI)" - depends on LTO_CLANG && ARCH_SUPPORTS_CFI_CLANG - depends on CLANG_VERSION >= 140000 - select KALLSYMS + depends on ARCH_SUPPORTS_CFI_CLANG + depends on $(cc-option,-fsanitize=kcfi) help This option enables Clang’s forward-edge Control Flow Integrity (CFI) checking, where the compiler injects a runtime check to each diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 7515a465ec03a..7501edfce11ea 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -421,6 +421,22 @@ __end_ro_after_init = .; #endif +/* + * .kcfi_traps contains a list KCFI trap locations. + */ +#ifndef KCFI_TRAPS +#ifdef CONFIG_ARCH_USES_CFI_TRAPS +#define KCFI_TRAPS \ + __kcfi_traps : AT(ADDR(__kcfi_traps) - LOAD_OFFSET) { \ + __start___kcfi_traps = .; \ + KEEP(*(.kcfi_traps)) \ + __stop___kcfi_traps = .; \ + } +#else +#define KCFI_TRAPS +#endif +#endif + /* * Read only Data */ @@ -529,6 +545,8 @@ __stop___modver = .; \ } \ \ + KCFI_TRAPS \ + \ RO_EXCEPTION_TABLE \ NOTES \ BTF \ @@ -537,21 +555,6 @@ __end_rodata = .; -/* - * .text..L.cfi.jumptable.* contain Control-Flow Integrity (CFI) - * jump table entries. - */ -#ifdef CONFIG_CFI_CLANG -#define TEXT_CFI_JT \ - . = ALIGN(PMD_SIZE); \ - __cfi_jt_start = .; \ - *(.text..L.cfi.jumptable .text..L.cfi.jumptable.*) \ - . = ALIGN(PMD_SIZE); \ - __cfi_jt_end = .; -#else -#define TEXT_CFI_JT -#endif - /* * Non-instrumentable text section */ @@ -579,7 +582,6 @@ *(.text..refcount) \ *(.ref.text) \ *(.text.asan.* .text.tsan.*) \ - TEXT_CFI_JT \ MEM_KEEP(init.text*) \ MEM_KEEP(exit.text*) \ @@ -1008,8 +1010,7 @@ * keep any .init_array.* sections. * https://bugs.llvm.org/show_bug.cgi?id=46478 */ -#if defined(CONFIG_GCOV_KERNEL) || defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KCSAN) || \ - defined(CONFIG_CFI_CLANG) +#if defined(CONFIG_GCOV_KERNEL) || defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KCSAN) # ifdef CONFIG_CONSTRUCTORS # define SANITIZER_DISCARDS \ *(.eh_frame) diff --git a/include/linux/cfi.h b/include/linux/cfi.h index 2cdbc0fbd0ab6..5e134f4ce8b75 100644 --- a/include/linux/cfi.h +++ b/include/linux/cfi.h @@ -2,17 +2,38 @@ /* * Clang Control Flow Integrity (CFI) support. * - * Copyright (C) 2021 Google LLC + * Copyright (C) 2022 Google LLC */ #ifndef _LINUX_CFI_H #define _LINUX_CFI_H +#include +#include + #ifdef CONFIG_CFI_CLANG -typedef void (*cfi_check_fn)(uint64_t id, void *ptr, void *diag); +enum bug_trap_type report_cfi_failure(struct pt_regs *regs, unsigned long addr, + unsigned long *target, u32 type); -/* Compiler-generated function in each module, and the kernel */ -extern void __cfi_check(uint64_t id, void *ptr, void *diag); +static inline enum bug_trap_type report_cfi_failure_noaddr(struct pt_regs *regs, + unsigned long addr) +{ + return report_cfi_failure(regs, addr, NULL, 0); +} +#ifdef CONFIG_ARCH_USES_CFI_TRAPS +bool is_cfi_trap(unsigned long addr); +#endif #endif /* CONFIG_CFI_CLANG */ +#ifdef CONFIG_MODULES +#ifdef CONFIG_ARCH_USES_CFI_TRAPS +void module_cfi_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs, + struct module *mod); +#else +static inline void module_cfi_finalize(const Elf_Ehdr *hdr, + const Elf_Shdr *sechdrs, + struct module *mod) {} +#endif /* CONFIG_ARCH_USES_CFI_TRAPS */ +#endif /* CONFIG_MODULES */ + #endif /* _LINUX_CFI_H */ diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h index c84fec767445d..42e55579d649b 100644 --- a/include/linux/compiler-clang.h +++ b/include/linux/compiler-clang.h @@ -66,17 +66,9 @@ # define __noscs __attribute__((__no_sanitize__("shadow-call-stack"))) #endif -#define __nocfi __attribute__((__no_sanitize__("cfi"))) -#define __cficanonical __attribute__((__cfi_canonical_jump_table__)) - -#if defined(CONFIG_CFI_CLANG) -/* - * With CONFIG_CFI_CLANG, the compiler replaces function address - * references with the address of the function's CFI jump table - * entry. The function_nocfi macro always returns the address of the - * actual function instead. - */ -#define function_nocfi(x) __builtin_function_start(x) +#if __has_feature(kcfi) +/* Disable CFI checking inside a function. */ +#define __nocfi __attribute__((__no_sanitize__("kcfi"))) #endif /* diff --git a/include/linux/module.h b/include/linux/module.h index 8937b020ec042..ec61fb53979a9 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -27,7 +27,6 @@ #include #include #include -#include #include #include @@ -387,8 +386,9 @@ struct module { const s32 *crcs; unsigned int num_syms; -#ifdef CONFIG_CFI_CLANG - cfi_check_fn cfi_check; +#ifdef CONFIG_ARCH_USES_CFI_TRAPS + s32 *kcfi_traps; + s32 *kcfi_traps_end; #endif /* Kernel parameters. */ diff --git a/kernel/cfi.c b/kernel/cfi.c index e8bc1b370edc3..08caad7767176 100644 --- a/kernel/cfi.c +++ b/kernel/cfi.c @@ -1,105 +1,101 @@ // SPDX-License-Identifier: GPL-2.0 /* - * Clang Control Flow Integrity (CFI) error and slowpath handling. + * Clang Control Flow Integrity (CFI) error handling. * - * Copyright (C) 2021 Google LLC + * Copyright (C) 2022 Google LLC */ -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -/* Compiler-defined handler names */ -#ifdef CONFIG_CFI_PERMISSIVE -#define cfi_failure_handler __ubsan_handle_cfi_check_fail -#else -#define cfi_failure_handler __ubsan_handle_cfi_check_fail_abort -#endif - -static inline void handle_cfi_failure(void *ptr) +#include + +enum bug_trap_type report_cfi_failure(struct pt_regs *regs, unsigned long addr, + unsigned long *target, u32 type) { - if (IS_ENABLED(CONFIG_CFI_PERMISSIVE)) - WARN_RATELIMIT(1, "CFI failure (target: %pS):\n", ptr); + if (target) + pr_err("CFI failure at %pS (target: %pS; expected type: 0x%08x)\n", + (void *)addr, (void *)*target, type); else - panic("CFI failure (target: %pS)\n", ptr); + pr_err("CFI failure at %pS (no target information)\n", + (void *)addr); + + if (IS_ENABLED(CONFIG_CFI_PERMISSIVE)) { + __warn(NULL, 0, (void *)addr, 0, regs, NULL); + return BUG_TRAP_TYPE_WARN; + } + + return BUG_TRAP_TYPE_BUG; } -#ifdef CONFIG_MODULES +#ifdef CONFIG_ARCH_USES_CFI_TRAPS +static inline unsigned long trap_address(s32 *p) +{ + return (unsigned long)((long)p + (long)*p); +} -static inline cfi_check_fn find_module_check_fn(unsigned long ptr) +static bool is_trap(unsigned long addr, s32 *start, s32 *end) { - cfi_check_fn fn = NULL; - struct module *mod; + s32 *p; - rcu_read_lock_sched_notrace(); - mod = __module_address(ptr); - if (mod) - fn = mod->cfi_check; - rcu_read_unlock_sched_notrace(); + for (p = start; p < end; ++p) { + if (trap_address(p) == addr) + return true; + } - return fn; + return false; } -static inline cfi_check_fn find_check_fn(unsigned long ptr) +#ifdef CONFIG_MODULES +/* Populates `kcfi_trap(_end)?` fields in `struct module`. */ +void module_cfi_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs, + struct module *mod) { - cfi_check_fn fn = NULL; - unsigned long flags; - bool rcu_idle; - - if (is_kernel_text(ptr)) - return __cfi_check; - - /* - * Indirect call checks can happen when RCU is not watching. Both - * the shadow and __module_address use RCU, so we need to wake it - * up if necessary. - */ - rcu_idle = !rcu_is_watching(); - if (rcu_idle) { - local_irq_save(flags); - ct_irq_enter(); - } + char *secstrings; + unsigned int i; - fn = find_module_check_fn(ptr); + mod->kcfi_traps = NULL; + mod->kcfi_traps_end = NULL; - if (rcu_idle) { - ct_irq_exit(); - local_irq_restore(flags); - } + secstrings = (char *)hdr + sechdrs[hdr->e_shstrndx].sh_offset; + + for (i = 1; i < hdr->e_shnum; i++) { + if (strcmp(secstrings + sechdrs[i].sh_name, "__kcfi_traps")) + continue; - return fn; + mod->kcfi_traps = (s32 *)sechdrs[i].sh_addr; + mod->kcfi_traps_end = (s32 *)(sechdrs[i].sh_addr + sechdrs[i].sh_size); + break; + } } -void __cfi_slowpath_diag(uint64_t id, void *ptr, void *diag) +static bool is_module_cfi_trap(unsigned long addr) { - cfi_check_fn fn = find_check_fn((unsigned long)ptr); + struct module *mod; + bool found = false; - if (likely(fn)) - fn(id, ptr, diag); - else /* Don't allow unchecked modules */ - handle_cfi_failure(ptr); -} -EXPORT_SYMBOL(__cfi_slowpath_diag); + rcu_read_lock_sched_notrace(); -#else /* !CONFIG_MODULES */ + mod = __module_address(addr); + if (mod) + found = is_trap(addr, mod->kcfi_traps, mod->kcfi_traps_end); -void __cfi_slowpath_diag(uint64_t id, void *ptr, void *diag) + rcu_read_unlock_sched_notrace(); + + return found; +} +#else /* CONFIG_MODULES */ +static inline bool is_module_cfi_trap(unsigned long addr) { - handle_cfi_failure(ptr); /* No modules */ + return false; } -EXPORT_SYMBOL(__cfi_slowpath_diag); - #endif /* CONFIG_MODULES */ -void cfi_failure_handler(void *data, void *ptr, void *vtable) +extern s32 __start___kcfi_traps[]; +extern s32 __stop___kcfi_traps[]; + +bool is_cfi_trap(unsigned long addr) { - handle_cfi_failure(ptr); + if (is_trap(addr, __start___kcfi_traps, __stop___kcfi_traps)) + return true; + + return is_module_cfi_trap(addr); } -EXPORT_SYMBOL(cfi_failure_handler); +#endif /* CONFIG_ARCH_USES_CFI_TRAPS */ diff --git a/kernel/module/main.c b/kernel/module/main.c index 0228f44b58e5b..70c0b2c6fef8c 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -53,6 +53,7 @@ #include #include #include +#include #include #include "internal.h" @@ -2597,8 +2598,9 @@ static int complete_formation(struct module *mod, struct load_info *info) if (err < 0) goto out; - /* This relies on module_mutex for list integrity. */ + /* These rely on module_mutex for list integrity. */ module_bug_finalize(info->hdr, info->sechdrs, mod); + module_cfi_finalize(info->hdr, info->sechdrs, mod); if (module_check_misalignment(mod)) goto out_misaligned; @@ -2660,8 +2662,6 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname, return 0; } -static void cfi_init(struct module *mod); - /* * Allocate and load the module: note that size of section 0 is always * zero, and we rely on this for optional sections. @@ -2791,9 +2791,6 @@ static int load_module(struct load_info *info, const char __user *uargs, flush_module_icache(mod); - /* Setup CFI for the module. */ - cfi_init(mod); - /* Now copy in args */ mod->args = strndup_user(uargs, ~0UL >> 1); if (IS_ERR(mod->args)) { @@ -2955,32 +2952,6 @@ static inline int within(unsigned long addr, void *start, unsigned long size) return ((void *)addr >= start && (void *)addr < start + size); } -static void cfi_init(struct module *mod) -{ -#ifdef CONFIG_CFI_CLANG - initcall_t *init; -#ifdef CONFIG_MODULE_UNLOAD - exitcall_t *exit; -#endif - - rcu_read_lock_sched(); - mod->cfi_check = (cfi_check_fn) - find_kallsyms_symbol_value(mod, "__cfi_check"); - init = (initcall_t *) - find_kallsyms_symbol_value(mod, "__cfi_jt_init_module"); - /* Fix init/exit functions to point to the CFI jump table */ - if (init) - mod->init = *init; -#ifdef CONFIG_MODULE_UNLOAD - exit = (exitcall_t *) - find_kallsyms_symbol_value(mod, "__cfi_jt_cleanup_module"); - if (exit) - mod->exit = *exit; -#endif - rcu_read_unlock_sched(); -#endif -} - /* Keep in sync with MODULE_FLAGS_BUF_SIZE !!! */ char *module_flags(struct module *mod, char *buf, bool show_state) { diff --git a/scripts/module.lds.S b/scripts/module.lds.S index 3a3aa2354ed86..da4bddd261717 100644 --- a/scripts/module.lds.S +++ b/scripts/module.lds.S @@ -3,20 +3,10 @@ * Archs are free to supply their own linker scripts. ld will * combine them automatically. */ -#ifdef CONFIG_CFI_CLANG -# include -# define ALIGN_CFI ALIGN(PAGE_SIZE) -# define SANITIZER_DISCARDS *(.eh_frame) -#else -# define ALIGN_CFI -# define SANITIZER_DISCARDS -#endif - SECTIONS { /DISCARD/ : { *(.discard) *(.discard.*) - SANITIZER_DISCARDS } __ksymtab 0 : { *(SORT(___ksymtab+*)) } @@ -33,6 +23,10 @@ SECTIONS { __patchable_function_entries : { *(__patchable_function_entries) } +#ifdef CONFIG_ARCH_USES_CFI_TRAPS + __kcfi_traps : { KEEP(*(.kcfi_traps)) } +#endif + #ifdef CONFIG_LTO_CLANG /* * With CONFIG_LTO_CLANG, LLD always enables -fdata-sections and @@ -53,15 +47,6 @@ SECTIONS { *(.rodata .rodata.[0-9a-zA-Z_]*) *(.rodata..L*) } - - /* - * With CONFIG_CFI_CLANG, we assume __cfi_check is at the beginning - * of the .text section, and is aligned to PAGE_SIZE. - */ - .text : ALIGN_CFI { - *(.text.__cfi_check) - *(.text .text.[0-9a-zA-Z_]* .text..L.cfi*) - } #endif } From e84e008e7b02c015047e76261726da1550130a59 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:48 -0700 Subject: [PATCH 25/46] cfi: Add type helper macros With CONFIG_CFI_CLANG, assembly functions called indirectly from C code must be annotated with type identifiers to pass CFI checking. In order to make this easier, the compiler emits a __kcfi_typeid_ symbol for each address-taken function declaration in C, which contains the expected type identifier that we can refer to in assembly code. Add a typed version of SYM_FUNC_START, which emits the type identifier before the function. Architectures that support KCFI can define their own __CFI_TYPE macro to override the default preamble format. As an example, for the x86_64 blowfish_dec_blk function, the compiler emits the following type symbol: $ readelf -sW vmlinux | grep __kcfi_typeid_blowfish_dec_blk 120204: 00000000ef478db5 0 NOTYPE WEAK DEFAULT ABS __kcfi_typeid_blowfish_dec_blk And SYM_TYPED_FUNC_START will generate the following preamble based on the __CFI_TYPE definition for the architecture: $ objdump -dr arch/x86/crypto/blowfish-x86_64-asm_64.o ... 0000000000000400 <__cfi_blowfish_dec_blk>: ... 40b: b8 00 00 00 00 mov $0x0,%eax 40c: R_X86_64_32 __kcfi_typeid_blowfish_dec_blk 0000000000000410 : ... Note that the address of all assembly functions annotated with SYM_TYPED_FUNC_START must be taken in C code that's linked into the binary or the missing __kcfi_typeid_ symbol will result in a linker error with CONFIG_CFI_CLANG. If the code that contains the indirect call is not always compiled in, __ADDRESSABLE(functionname) can be used to ensure that the __kcfi_typeid_ symbol is emitted. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-7-samitolvanen@google.com --- include/linux/cfi_types.h | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 include/linux/cfi_types.h diff --git a/include/linux/cfi_types.h b/include/linux/cfi_types.h new file mode 100644 index 0000000000000..6b87136757655 --- /dev/null +++ b/include/linux/cfi_types.h @@ -0,0 +1,45 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Clang Control Flow Integrity (CFI) type definitions. + */ +#ifndef _LINUX_CFI_TYPES_H +#define _LINUX_CFI_TYPES_H + +#ifdef __ASSEMBLY__ +#include + +#ifdef CONFIG_CFI_CLANG +/* + * Use the __kcfi_typeid_ type identifier symbol to + * annotate indirectly called assembly functions. The compiler emits + * these symbols for all address-taken function declarations in C + * code. + */ +#ifndef __CFI_TYPE +#define __CFI_TYPE(name) \ + .4byte __kcfi_typeid_##name +#endif + +#define SYM_TYPED_ENTRY(name, linkage, align...) \ + linkage(name) ASM_NL \ + align ASM_NL \ + __CFI_TYPE(name) ASM_NL \ + name: + +#define SYM_TYPED_START(name, linkage, align...) \ + SYM_TYPED_ENTRY(name, linkage, align) + +#else /* CONFIG_CFI_CLANG */ + +#define SYM_TYPED_START(name, linkage, align...) \ + SYM_START(name, linkage, align) + +#endif /* CONFIG_CFI_CLANG */ + +#ifndef SYM_TYPED_FUNC_START +#define SYM_TYPED_FUNC_START(name) \ + SYM_TYPED_START(name, SYM_L_GLOBAL, SYM_A_ALIGN) +#endif + +#endif /* __ASSEMBLY__ */ +#endif /* _LINUX_CFI_TYPES_H */ From cf90d0383560de12330de8cf3f831b14cdd45914 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:49 -0700 Subject: [PATCH 26/46] lkdtm: Emit an indirect call for CFI tests Clang can convert the indirect calls in lkdtm_CFI_FORWARD_PROTO into direct calls. Move the call into a noinline function that accepts the target address as an argument to ensure the compiler actually emits an indirect call instead. Signed-off-by: Sami Tolvanen Reviewed-by: Nick Desaulniers Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Kees Cook Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-8-samitolvanen@google.com --- drivers/misc/lkdtm/cfi.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c index 71483cb1e422a..5245cf6013c95 100644 --- a/drivers/misc/lkdtm/cfi.c +++ b/drivers/misc/lkdtm/cfi.c @@ -20,6 +20,13 @@ static noinline int lkdtm_increment_int(int *counter) return *counter; } + +/* Don't allow the compiler to inline the calls. */ +static noinline void lkdtm_indirect_call(void (*func)(int *)) +{ + func(&called_count); +} + /* * This tries to call an indirect function with a mismatched prototype. */ @@ -29,15 +36,11 @@ static void lkdtm_CFI_FORWARD_PROTO(void) * Matches lkdtm_increment_void()'s prototype, but not * lkdtm_increment_int()'s prototype. */ - void (*func)(int *); - pr_info("Calling matched prototype ...\n"); - func = lkdtm_increment_void; - func(&called_count); + lkdtm_indirect_call(lkdtm_increment_void); pr_info("Calling mismatched prototype ...\n"); - func = (void *)lkdtm_increment_int; - func(&called_count); + lkdtm_indirect_call((void *)lkdtm_increment_int); pr_err("FAIL: survived mismatched prototype function call!\n"); pr_expected_config(CONFIG_CFI_CLANG); From 44f665b69c67f0a17a0c8748030ed30205532149 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:50 -0700 Subject: [PATCH 27/46] psci: Fix the function type for psci_initcall_t Functions called through a psci_initcall_t pointer all have non-const arguments. Fix the type definition to avoid tripping indirect call checks with CFI_CLANG. Suggested-by: Mark Rutland Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-9-samitolvanen@google.com --- drivers/firmware/psci/psci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c index cfb448eabdaa2..75ef784a37891 100644 --- a/drivers/firmware/psci/psci.c +++ b/drivers/firmware/psci/psci.c @@ -274,7 +274,7 @@ static void set_conduit(enum arm_smccc_conduit conduit) psci_conduit = conduit; } -static int get_set_conduit_method(struct device_node *np) +static int get_set_conduit_method(const struct device_node *np) { const char *method; @@ -528,7 +528,7 @@ typedef int (*psci_initcall_t)(const struct device_node *); * * Probe based on PSCI PSCI_VERSION function */ -static int __init psci_0_2_init(struct device_node *np) +static int __init psci_0_2_init(const struct device_node *np) { int err; @@ -549,7 +549,7 @@ static int __init psci_0_2_init(struct device_node *np) /* * PSCI < v0.2 get PSCI Function IDs via DT. */ -static int __init psci_0_1_init(struct device_node *np) +static int __init psci_0_1_init(const struct device_node *np) { u32 id; int err; @@ -585,7 +585,7 @@ static int __init psci_0_1_init(struct device_node *np) return 0; } -static int __init psci_1_0_init(struct device_node *np) +static int __init psci_1_0_init(const struct device_node *np) { int err; From c50d32859e70f6dbccb7d151408eb10afbbb7965 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:51 -0700 Subject: [PATCH 28/46] arm64: Add types to indirect called assembly functions With CONFIG_CFI_CLANG, assembly functions indirectly called from C code must be annotated with type identifiers to pass CFI checking. Use SYM_TYPED_FUNC_START for the indirectly called functions, and ensure we emit `bti c` also with SYM_TYPED_FUNC_START. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-10-samitolvanen@google.com --- arch/arm64/crypto/ghash-ce-core.S | 5 +++-- arch/arm64/crypto/sm3-ce-core.S | 3 ++- arch/arm64/include/asm/linkage.h | 4 ++++ arch/arm64/kernel/cpu-reset.S | 5 +++-- arch/arm64/mm/proc.S | 5 +++-- 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/arch/arm64/crypto/ghash-ce-core.S b/arch/arm64/crypto/ghash-ce-core.S index 7868330dd54ec..ebe5558929b7b 100644 --- a/arch/arm64/crypto/ghash-ce-core.S +++ b/arch/arm64/crypto/ghash-ce-core.S @@ -6,6 +6,7 @@ */ #include +#include #include SHASH .req v0 @@ -350,11 +351,11 @@ CPU_LE( rev64 T1.16b, T1.16b ) * void pmull_ghash_update(int blocks, u64 dg[], const char *src, * struct ghash_key const *k, const char *head) */ -SYM_FUNC_START(pmull_ghash_update_p64) +SYM_TYPED_FUNC_START(pmull_ghash_update_p64) __pmull_ghash p64 SYM_FUNC_END(pmull_ghash_update_p64) -SYM_FUNC_START(pmull_ghash_update_p8) +SYM_TYPED_FUNC_START(pmull_ghash_update_p8) __pmull_ghash p8 SYM_FUNC_END(pmull_ghash_update_p8) diff --git a/arch/arm64/crypto/sm3-ce-core.S b/arch/arm64/crypto/sm3-ce-core.S index ef97d3187cb7f..ca70cfacd0d0a 100644 --- a/arch/arm64/crypto/sm3-ce-core.S +++ b/arch/arm64/crypto/sm3-ce-core.S @@ -6,6 +6,7 @@ */ #include +#include #include .irp b, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12 @@ -73,7 +74,7 @@ * int blocks) */ .text -SYM_FUNC_START(sm3_ce_transform) +SYM_TYPED_FUNC_START(sm3_ce_transform) /* load state */ ld1 {v8.4s-v9.4s}, [x0] rev64 v8.4s, v8.4s diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h index 43f8c25b3fda6..1436fa1cde24d 100644 --- a/arch/arm64/include/asm/linkage.h +++ b/arch/arm64/include/asm/linkage.h @@ -39,4 +39,8 @@ SYM_START(name, SYM_L_WEAK, SYM_A_NONE) \ bti c ; +#define SYM_TYPED_FUNC_START(name) \ + SYM_TYPED_START(name, SYM_L_GLOBAL, SYM_A_ALIGN) \ + bti c ; + #endif diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S index 48a8af97faa9a..6b752fe897451 100644 --- a/arch/arm64/kernel/cpu-reset.S +++ b/arch/arm64/kernel/cpu-reset.S @@ -8,6 +8,7 @@ */ #include +#include #include #include #include @@ -28,7 +29,7 @@ * branch to what would be the reset vector. It must be executed with the * flat identity mapping. */ -SYM_CODE_START(cpu_soft_restart) +SYM_TYPED_FUNC_START(cpu_soft_restart) mov_q x12, INIT_SCTLR_EL1_MMU_OFF pre_disable_mmu_workaround /* @@ -47,6 +48,6 @@ SYM_CODE_START(cpu_soft_restart) mov x1, x3 // arg1 mov x2, x4 // arg2 br x8 -SYM_CODE_END(cpu_soft_restart) +SYM_FUNC_END(cpu_soft_restart) .popsection diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index 7837a69524c53..8b9f419fcad99 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -185,7 +186,7 @@ SYM_FUNC_END(cpu_do_resume) * This is the low-level counterpart to cpu_replace_ttbr1, and should not be * called by anything else. It can only be executed from a TTBR0 mapping. */ -SYM_FUNC_START(idmap_cpu_replace_ttbr1) +SYM_TYPED_FUNC_START(idmap_cpu_replace_ttbr1) save_and_disable_daif flags=x2 __idmap_cpu_set_reserved_ttbr1 x1, x3 @@ -253,7 +254,7 @@ SYM_FUNC_END(idmap_cpu_replace_ttbr1) SYM_DATA(__idmap_kpti_flag, .long 1) .popsection -SYM_FUNC_START(idmap_kpti_install_ng_mappings) +SYM_TYPED_FUNC_START(idmap_kpti_install_ng_mappings) cpu .req w0 temp_pte .req x0 num_cpus .req w1 From b26e484b8bb3a992ef30e851d771973a3dd2336b Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:52 -0700 Subject: [PATCH 29/46] arm64: Add CFI error handling With -fsanitize=kcfi, CFI always traps. Add arm64 support for handling CFI failures. The registers containing the target address and the expected type are encoded in the first ten bits of the ESR as follows: - 0-4: n, where the register Xn contains the target address - 5-9: m, where the register Wm contains the type hash This produces the following oops on CFI failure (generated using lkdtm): [ 21.885179] CFI failure at lkdtm_indirect_call+0x2c/0x44 [lkdtm] (target: lkdtm_increment_int+0x0/0x1c [lkdtm]; expected type: 0x7e0c52a) [ 21.886593] Internal error: Oops - CFI: 0 [#1] PREEMPT SMP [ 21.891060] Modules linked in: lkdtm [ 21.893363] CPU: 0 PID: 151 Comm: sh Not tainted 5.19.0-rc1-00021-g852f4e48dbab #1 [ 21.895560] Hardware name: linux,dummy-virt (DT) [ 21.896543] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 21.897583] pc : lkdtm_indirect_call+0x2c/0x44 [lkdtm] [ 21.898551] lr : lkdtm_CFI_FORWARD_PROTO+0x3c/0x6c [lkdtm] [ 21.899520] sp : ffff8000083a3c50 [ 21.900191] x29: ffff8000083a3c50 x28: ffff0000027e0ec0 x27: 0000000000000000 [ 21.902453] x26: 0000000000000000 x25: ffffc2aa3d07e7b0 x24: 0000000000000002 [ 21.903736] x23: ffffc2aa3d079088 x22: ffffc2aa3d07e7b0 x21: ffff000003379000 [ 21.905062] x20: ffff8000083a3dc0 x19: 0000000000000012 x18: 0000000000000000 [ 21.906371] x17: 000000007e0c52a5 x16: 000000003ad55aca x15: ffffc2aa60d92138 [ 21.907662] x14: ffffffffffffffff x13: 2e2e2e2065707974 x12: 0000000000000018 [ 21.909775] x11: ffffc2aa62322b88 x10: ffffc2aa62322aa0 x9 : c7e305fb5195d200 [ 21.911898] x8 : ffffc2aa3d077e20 x7 : 6d20676e696c6c61 x6 : 43203a6d74646b6c [ 21.913108] x5 : ffffc2aa6266c9df x4 : ffffc2aa6266c9e1 x3 : ffff8000083a3968 [ 21.914358] x2 : 80000000fffff122 x1 : 00000000fffff122 x0 : ffffc2aa3d07e8f8 [ 21.915827] Call trace: [ 21.916375] lkdtm_indirect_call+0x2c/0x44 [lkdtm] [ 21.918060] lkdtm_CFI_FORWARD_PROTO+0x3c/0x6c [lkdtm] [ 21.919030] lkdtm_do_action+0x34/0x4c [lkdtm] [ 21.919920] direct_entry+0x170/0x1ac [lkdtm] [ 21.920772] full_proxy_write+0x84/0x104 [ 21.921759] vfs_write+0x188/0x3d8 [ 21.922387] ksys_write+0x78/0xe8 [ 21.922986] __arm64_sys_write+0x1c/0x2c [ 21.923696] invoke_syscall+0x58/0x134 [ 21.924554] el0_svc_common+0xb4/0xf4 [ 21.925603] do_el0_svc+0x2c/0xb4 [ 21.926563] el0_svc+0x2c/0x7c [ 21.927147] el0t_64_sync_handler+0x84/0xf0 [ 21.927985] el0t_64_sync+0x18c/0x190 [ 21.929133] Code: 728a54b1 72afc191 6b11021f 54000040 (d4304500) [ 21.930690] ---[ end trace 0000000000000000 ]--- [ 21.930971] Kernel panic - not syncing: Oops - CFI: Fatal exception Suggested-by: Mark Rutland Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-11-samitolvanen@google.com --- arch/arm64/include/asm/brk-imm.h | 6 ++++ arch/arm64/kernel/traps.c | 47 ++++++++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h index ec7720dbe2c80..6e000113e508a 100644 --- a/arch/arm64/include/asm/brk-imm.h +++ b/arch/arm64/include/asm/brk-imm.h @@ -17,6 +17,7 @@ * 0x401: for compile time BRK instruction * 0x800: kernel-mode BUG() and WARN() traps * 0x9xx: tag-based KASAN trap (allowed values 0x900 - 0x9ff) + * 0x8xxx: Control-Flow Integrity traps */ #define KPROBES_BRK_IMM 0x004 #define UPROBES_BRK_IMM 0x005 @@ -28,4 +29,9 @@ #define KASAN_BRK_IMM 0x900 #define KASAN_BRK_MASK 0x0ff +#define CFI_BRK_IMM_TARGET GENMASK(4, 0) +#define CFI_BRK_IMM_TYPE GENMASK(9, 5) +#define CFI_BRK_IMM_BASE 0x8000 +#define CFI_BRK_IMM_MASK (CFI_BRK_IMM_TARGET | CFI_BRK_IMM_TYPE) + #endif diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index b7fed33981f7b..3c026da95bbc9 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -991,6 +992,38 @@ static struct break_hook bug_break_hook = { .imm = BUG_BRK_IMM, }; +#ifdef CONFIG_CFI_CLANG +static int cfi_handler(struct pt_regs *regs, unsigned long esr) +{ + unsigned long target; + u32 type; + + target = pt_regs_read_reg(regs, FIELD_GET(CFI_BRK_IMM_TARGET, esr)); + type = (u32)pt_regs_read_reg(regs, FIELD_GET(CFI_BRK_IMM_TYPE, esr)); + + switch (report_cfi_failure(regs, regs->pc, &target, type)) { + case BUG_TRAP_TYPE_BUG: + die("Oops - CFI", regs, 0); + break; + + case BUG_TRAP_TYPE_WARN: + break; + + default: + return DBG_HOOK_ERROR; + } + + arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE); + return DBG_HOOK_HANDLED; +} + +static struct break_hook cfi_break_hook = { + .fn = cfi_handler, + .imm = CFI_BRK_IMM_BASE, + .mask = CFI_BRK_IMM_MASK, +}; +#endif /* CONFIG_CFI_CLANG */ + static int reserved_fault_handler(struct pt_regs *regs, unsigned long esr) { pr_err("%s generated an invalid instruction at %pS!\n", @@ -1052,6 +1085,9 @@ static struct break_hook kasan_break_hook = { }; #endif + +#define esr_comment(esr) ((esr) & ESR_ELx_BRK64_ISS_COMMENT_MASK) + /* * Initial handler for AArch64 BRK exceptions * This handler only used until debug_traps_init(). @@ -1059,10 +1095,12 @@ static struct break_hook kasan_break_hook = { int __init early_brk64(unsigned long addr, unsigned long esr, struct pt_regs *regs) { +#ifdef CONFIG_CFI_CLANG + if ((esr_comment(esr) & ~CFI_BRK_IMM_MASK) == CFI_BRK_IMM_BASE) + return cfi_handler(regs, esr) != DBG_HOOK_HANDLED; +#endif #ifdef CONFIG_KASAN_SW_TAGS - unsigned long comment = esr & ESR_ELx_BRK64_ISS_COMMENT_MASK; - - if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM) + if ((esr_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM) return kasan_handler(regs, esr) != DBG_HOOK_HANDLED; #endif return bug_handler(regs, esr) != DBG_HOOK_HANDLED; @@ -1071,6 +1109,9 @@ int __init early_brk64(unsigned long addr, unsigned long esr, void __init trap_init(void) { register_kernel_break_hook(&bug_break_hook); +#ifdef CONFIG_CFI_CLANG + register_kernel_break_hook(&cfi_break_hook); +#endif register_kernel_break_hook(&fault_break_hook); #ifdef CONFIG_KASAN_SW_TAGS register_kernel_break_hook(&kasan_break_hook); From 5f20997c194e8b74254cbdb113b2b09bc1c0c734 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:53 -0700 Subject: [PATCH 30/46] arm64: Drop unneeded __nocfi attributes With -fsanitize=kcfi, CONFIG_CFI_CLANG no longer has issues with address space confusion in functions that switch to linear mapping. Now that the indirectly called assembly functions have type annotations, drop the __nocfi attributes. Suggested-by: Mark Rutland Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-12-samitolvanen@google.com --- arch/arm64/include/asm/mmu_context.h | 2 +- arch/arm64/kernel/alternative.c | 2 +- arch/arm64/kernel/cpufeature.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h index c7ccd82db1d24..bba0e630c8bc0 100644 --- a/arch/arm64/include/asm/mmu_context.h +++ b/arch/arm64/include/asm/mmu_context.h @@ -147,7 +147,7 @@ static inline void cpu_install_ttbr0(phys_addr_t ttbr0, unsigned long t0sz) * Atomically replaces the active TTBR1_EL1 PGD with a new VA-compatible PGD, * avoiding the possibility of conflicting TLB entries being allocated. */ -static inline void __nocfi cpu_replace_ttbr1(pgd_t *pgdp, pgd_t *idmap) +static inline void cpu_replace_ttbr1(pgd_t *pgdp, pgd_t *idmap) { typedef void (ttbr_replace_func)(phys_addr_t); extern ttbr_replace_func idmap_cpu_replace_ttbr1; diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c index 9bcaa5eacf16c..d2c66507398d7 100644 --- a/arch/arm64/kernel/alternative.c +++ b/arch/arm64/kernel/alternative.c @@ -133,7 +133,7 @@ static void clean_dcache_range_nopatch(u64 start, u64 end) } while (cur += d_size, cur < end); } -static void __nocfi __apply_alternatives(struct alt_region *region, bool is_module, +static void __apply_alternatives(struct alt_region *region, bool is_module, unsigned long *feature_mask) { struct alt_instr *alt; diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index af4de817d7123..ca6e5ca7104ed 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1685,7 +1685,7 @@ static phys_addr_t kpti_ng_pgd_alloc(int shift) return kpti_ng_temp_alloc; } -static void __nocfi +static void kpti_install_ng_mappings(const struct arm64_cpu_capabilities *__unused) { typedef void (kpti_remap_fn)(int, int, phys_addr_t, unsigned long); From 5dbbb3eaa2a784342f3206b77381b516181d089c Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:54 -0700 Subject: [PATCH 31/46] init: Drop __nocfi from __init It's no longer necessary to disable CFI checking for all __init functions. Drop the __nocfi attribute from __init. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-13-samitolvanen@google.com --- include/linux/init.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/init.h b/include/linux/init.h index baf0b29a7010a..88f2964097f59 100644 --- a/include/linux/init.h +++ b/include/linux/init.h @@ -47,7 +47,7 @@ /* These are for everybody (although not all archs will actually discard it in modules) */ -#define __init __section(".init.text") __cold __latent_entropy __noinitretpoline __nocfi +#define __init __section(".init.text") __cold __latent_entropy __noinitretpoline #define __initdata __section(".init.data") #define __initconst __section(".init.rodata") #define __exitdata __section(".exit.data") From 607289a7cd7a3ca42b8a6877fcb6072e6eb20c34 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:55 -0700 Subject: [PATCH 32/46] treewide: Drop function_nocfi With -fsanitize=kcfi, we no longer need function_nocfi() as the compiler won't change function references to point to a jump table. Remove all implementations and uses of the macro. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-14-samitolvanen@google.com --- arch/arm64/include/asm/ftrace.h | 2 +- arch/arm64/include/asm/mmu_context.h | 2 +- arch/arm64/kernel/acpi_parking_protocol.c | 2 +- arch/arm64/kernel/cpufeature.c | 2 +- arch/arm64/kernel/ftrace.c | 2 +- arch/arm64/kernel/machine_kexec.c | 2 +- arch/arm64/kernel/psci.c | 2 +- arch/arm64/kernel/smp_spin_table.c | 2 +- drivers/firmware/psci/psci.c | 4 ++-- drivers/misc/lkdtm/usercopy.c | 2 +- include/linux/compiler.h | 10 ---------- 11 files changed, 11 insertions(+), 21 deletions(-) diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h index dbc45a4157fa7..329dbbd4d50b6 100644 --- a/arch/arm64/include/asm/ftrace.h +++ b/arch/arm64/include/asm/ftrace.h @@ -26,7 +26,7 @@ #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS #define ARCH_SUPPORTS_FTRACE_OPS 1 #else -#define MCOUNT_ADDR ((unsigned long)function_nocfi(_mcount)) +#define MCOUNT_ADDR ((unsigned long)_mcount) #endif /* The BL at the callsite's adjusted rec->ip */ diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h index bba0e630c8bc0..d3f8b5df0c1fe 100644 --- a/arch/arm64/include/asm/mmu_context.h +++ b/arch/arm64/include/asm/mmu_context.h @@ -168,7 +168,7 @@ static inline void cpu_replace_ttbr1(pgd_t *pgdp, pgd_t *idmap) ttbr1 |= TTBR_CNP_BIT; } - replace_phys = (void *)__pa_symbol(function_nocfi(idmap_cpu_replace_ttbr1)); + replace_phys = (void *)__pa_symbol(idmap_cpu_replace_ttbr1); __cpu_install_idmap(idmap); replace_phys(ttbr1); diff --git a/arch/arm64/kernel/acpi_parking_protocol.c b/arch/arm64/kernel/acpi_parking_protocol.c index bfeeb5319abf9..b1990e38aed0a 100644 --- a/arch/arm64/kernel/acpi_parking_protocol.c +++ b/arch/arm64/kernel/acpi_parking_protocol.c @@ -99,7 +99,7 @@ static int acpi_parking_protocol_cpu_boot(unsigned int cpu) * that read this address need to convert this address to the * Boot-Loader's endianness before jumping. */ - writeq_relaxed(__pa_symbol(function_nocfi(secondary_entry)), + writeq_relaxed(__pa_symbol(secondary_entry), &mailbox->entry_point); writel_relaxed(cpu_entry->gic_cpu_id, &mailbox->cpu_id); diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index ca6e5ca7104ed..d8361691efeb6 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1713,7 +1713,7 @@ kpti_install_ng_mappings(const struct arm64_cpu_capabilities *__unused) if (arm64_use_ng_mappings) return; - remap_fn = (void *)__pa_symbol(function_nocfi(idmap_kpti_install_ng_mappings)); + remap_fn = (void *)__pa_symbol(idmap_kpti_install_ng_mappings); if (!cpu) { alloc = __get_free_pages(GFP_ATOMIC | __GFP_ZERO, order); diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c index ea5dc7c90f465..26789865748c6 100644 --- a/arch/arm64/kernel/ftrace.c +++ b/arch/arm64/kernel/ftrace.c @@ -56,7 +56,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func) unsigned long pc; u32 new; - pc = (unsigned long)function_nocfi(ftrace_call); + pc = (unsigned long)ftrace_call; new = aarch64_insn_gen_branch_imm(pc, (unsigned long)func, AARCH64_INSN_BRANCH_LINK); diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c index 19c2d487cb08f..ce3d40120f72f 100644 --- a/arch/arm64/kernel/machine_kexec.c +++ b/arch/arm64/kernel/machine_kexec.c @@ -204,7 +204,7 @@ void machine_kexec(struct kimage *kimage) typeof(cpu_soft_restart) *restart; cpu_install_idmap(); - restart = (void *)__pa_symbol(function_nocfi(cpu_soft_restart)); + restart = (void *)__pa_symbol(cpu_soft_restart); restart(is_hyp_nvhe(), kimage->start, kimage->arch.dtb_mem, 0, 0); } else { diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index ab7f4c4761046..29a8e444db836 100644 --- a/arch/arm64/kernel/psci.c +++ b/arch/arm64/kernel/psci.c @@ -38,7 +38,7 @@ static int __init cpu_psci_cpu_prepare(unsigned int cpu) static int cpu_psci_cpu_boot(unsigned int cpu) { - phys_addr_t pa_secondary_entry = __pa_symbol(function_nocfi(secondary_entry)); + phys_addr_t pa_secondary_entry = __pa_symbol(secondary_entry); int err = psci_ops.cpu_on(cpu_logical_map(cpu), pa_secondary_entry); if (err) pr_err("failed to boot CPU%d (%d)\n", cpu, err); diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c index 7e1624ecab3c8..49029eace3ad9 100644 --- a/arch/arm64/kernel/smp_spin_table.c +++ b/arch/arm64/kernel/smp_spin_table.c @@ -66,7 +66,7 @@ static int smp_spin_table_cpu_init(unsigned int cpu) static int smp_spin_table_cpu_prepare(unsigned int cpu) { __le64 __iomem *release_addr; - phys_addr_t pa_holding_pen = __pa_symbol(function_nocfi(secondary_holding_pen)); + phys_addr_t pa_holding_pen = __pa_symbol(secondary_holding_pen); if (!cpu_release_addr[cpu]) return -ENODEV; diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c index 75ef784a37891..bc6b5a12bf741 100644 --- a/drivers/firmware/psci/psci.c +++ b/drivers/firmware/psci/psci.c @@ -334,7 +334,7 @@ static int __init psci_features(u32 psci_func_id) static int psci_suspend_finisher(unsigned long state) { u32 power_state = state; - phys_addr_t pa_cpu_resume = __pa_symbol(function_nocfi(cpu_resume)); + phys_addr_t pa_cpu_resume = __pa_symbol(cpu_resume); return psci_ops.cpu_suspend(power_state, pa_cpu_resume); } @@ -359,7 +359,7 @@ int psci_cpu_suspend_enter(u32 state) static int psci_system_suspend(unsigned long unused) { - phys_addr_t pa_cpu_resume = __pa_symbol(function_nocfi(cpu_resume)); + phys_addr_t pa_cpu_resume = __pa_symbol(cpu_resume); return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND), pa_cpu_resume, 0, 0); diff --git a/drivers/misc/lkdtm/usercopy.c b/drivers/misc/lkdtm/usercopy.c index 6215ec995cd3c..67db57249a347 100644 --- a/drivers/misc/lkdtm/usercopy.c +++ b/drivers/misc/lkdtm/usercopy.c @@ -330,7 +330,7 @@ static void lkdtm_USERCOPY_KERNEL(void) pr_info("attempting bad copy_to_user from kernel text: %px\n", vm_mmap); - if (copy_to_user((void __user *)user_addr, function_nocfi(vm_mmap), + if (copy_to_user((void __user *)user_addr, vm_mmap, unconst + PAGE_SIZE)) { pr_warn("copy_to_user failed, but lacked Oops\n"); goto free_user; diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 7bfafc69172af..973a1bfd7ef53 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -203,16 +203,6 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, __v; \ }) -/* - * With CONFIG_CFI_CLANG, the compiler replaces function addresses in - * instrumented C code with jump table addresses. Architectures that - * support CFI can define this macro to return the actual function address - * when needed. - */ -#ifndef function_nocfi -#define function_nocfi(x) (x) -#endif - #endif /* __KERNEL__ */ /* From 4b24356312fbe1bace72f9905d529b14fc34c1c3 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:56 -0700 Subject: [PATCH 33/46] treewide: Drop WARN_ON_FUNCTION_MISMATCH CONFIG_CFI_CLANG no longer breaks cross-module function address equality, which makes WARN_ON_FUNCTION_MISMATCH unnecessary. Remove the definition and switch back to WARN_ON_ONCE. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-15-samitolvanen@google.com --- include/asm-generic/bug.h | 16 ---------------- kernel/kthread.c | 3 +-- kernel/workqueue.c | 2 +- 3 files changed, 2 insertions(+), 19 deletions(-) diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index ba1f860af38ba..4050b191e1a97 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -220,22 +220,6 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); # define WARN_ON_SMP(x) ({0;}) #endif -/* - * WARN_ON_FUNCTION_MISMATCH() warns if a value doesn't match a - * function address, and can be useful for catching issues with - * callback functions, for example. - * - * With CONFIG_CFI_CLANG, the warning is disabled because the - * compiler replaces function addresses taken in C code with - * local jump table addresses, which breaks cross-module function - * address equality. - */ -#if defined(CONFIG_CFI_CLANG) && defined(CONFIG_MODULES) -# define WARN_ON_FUNCTION_MISMATCH(x, fn) ({ 0; }) -#else -# define WARN_ON_FUNCTION_MISMATCH(x, fn) WARN_ON_ONCE((x) != (fn)) -#endif - #endif /* __ASSEMBLY__ */ #endif diff --git a/kernel/kthread.c b/kernel/kthread.c index 3c677918d8f2f..28a6b7ab4a0fa 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -1050,8 +1050,7 @@ static void __kthread_queue_delayed_work(struct kthread_worker *worker, struct timer_list *timer = &dwork->timer; struct kthread_work *work = &dwork->work; - WARN_ON_FUNCTION_MISMATCH(timer->function, - kthread_delayed_work_timer_fn); + WARN_ON_ONCE(timer->function != kthread_delayed_work_timer_fn); /* * If @delay is 0, queue @dwork->work immediately. This is for diff --git a/kernel/workqueue.c b/kernel/workqueue.c index aeea9731ef804..16df315d2a3d9 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -1651,7 +1651,7 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq, struct work_struct *work = &dwork->work; WARN_ON_ONCE(!wq); - WARN_ON_FUNCTION_MISMATCH(timer->function, delayed_work_timer_fn); + WARN_ON_ONCE(timer->function != delayed_work_timer_fn); WARN_ON_ONCE(timer_pending(timer)); WARN_ON_ONCE(!list_empty(&work->entry)); From 5659b598b4dcb352b1a567c55fc5a658bc80076c Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:57 -0700 Subject: [PATCH 34/46] treewide: Drop __cficanonical CONFIG_CFI_CLANG doesn't use a jump table anymore and therefore, won't change function references to point elsewhere. Remove the __cficanonical attribute and all uses of it. Note that the Clang definition of the attribute was removed earlier, just clean up the no-op definition and users. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-16-samitolvanen@google.com --- include/linux/compiler_types.h | 4 ---- include/linux/init.h | 4 ++-- include/linux/pci.h | 4 ++-- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 4f2a819fd60a3..6f2ec0976e2d9 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -265,10 +265,6 @@ struct ftrace_likely_data { # define __nocfi #endif -#ifndef __cficanonical -# define __cficanonical -#endif - /* * Any place that could be marked with the "alloc_size" attribute is also * a place to be marked with the "malloc" attribute. Do this as part of the diff --git a/include/linux/init.h b/include/linux/init.h index 88f2964097f59..a0a90cd73ebee 100644 --- a/include/linux/init.h +++ b/include/linux/init.h @@ -220,8 +220,8 @@ extern bool initcall_debug; __initcall_name(initstub, __iid, id) #define __define_initcall_stub(__stub, fn) \ - int __init __cficanonical __stub(void); \ - int __init __cficanonical __stub(void) \ + int __init __stub(void); \ + int __init __stub(void) \ { \ return fn(); \ } \ diff --git a/include/linux/pci.h b/include/linux/pci.h index 060af91bafcd4..5da0846aa3c17 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2019,8 +2019,8 @@ enum pci_fixup_pass { #ifdef CONFIG_LTO_CLANG #define __DECLARE_PCI_FIXUP_SECTION(sec, name, vendor, device, class, \ class_shift, hook, stub) \ - void __cficanonical stub(struct pci_dev *dev); \ - void __cficanonical stub(struct pci_dev *dev) \ + void stub(struct pci_dev *dev); \ + void stub(struct pci_dev *dev) \ { \ hook(dev); \ } \ From 5141d3a06b2da1731ac82091298b766a1f95d3d8 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:58 -0700 Subject: [PATCH 35/46] objtool: Preserve special st_shndx indexes in elf_update_symbol elf_update_symbol fails to preserve the special st_shndx values between [SHN_LORESERVE, SHN_HIRESERVE], which results in it converting SHN_ABS entries into SHN_UNDEF, for example. Explicitly check for the special indexes and ensure these symbols are not marked undefined. Fixes: ead165fa1042 ("objtool: Fix symbol creation") Signed-off-by: Sami Tolvanen Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-17-samitolvanen@google.com --- tools/objtool/elf.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c index c25e957c1e520..7e24b09b1163a 100644 --- a/tools/objtool/elf.c +++ b/tools/objtool/elf.c @@ -619,6 +619,11 @@ static int elf_update_symbol(struct elf *elf, struct section *symtab, Elf64_Xword entsize = symtab->sh.sh_entsize; int max_idx, idx = sym->idx; Elf_Scn *s, *t = NULL; + bool is_special_shndx = sym->sym.st_shndx >= SHN_LORESERVE && + sym->sym.st_shndx != SHN_XINDEX; + + if (is_special_shndx) + shndx = sym->sym.st_shndx; s = elf_getscn(elf->elf, symtab->idx); if (!s) { @@ -704,7 +709,7 @@ static int elf_update_symbol(struct elf *elf, struct section *symtab, } /* setup extended section index magic and write the symbol */ - if (shndx >= SHN_UNDEF && shndx < SHN_LORESERVE) { + if ((shndx >= SHN_UNDEF && shndx < SHN_LORESERVE) || is_special_shndx) { sym->sym.st_shndx = shndx; if (!shndx_data) shndx = 0; From 3c68a92d17add767109441f4040391b9e8a14a98 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:54:59 -0700 Subject: [PATCH 36/46] objtool: Disable CFI warnings The __cfi_ preambles contain a mov instruction that embeds the KCFI type identifier in the following format: ; type preamble __cfi_function: mov , %eax function: ... While the preamble symbols are STT_FUNC and contain valid instructions, they are never executed and always fall through. Skip the warning for them. .kcfi_traps sections point to CFI traps in text sections. Also skip the warning about them referencing !ENDBR instructions. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Josh Poimboeuf Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-18-samitolvanen@google.com --- tools/objtool/check.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tools/objtool/check.c b/tools/objtool/check.c index e55fdf952a3a1..48e18737a2d18 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -3316,6 +3316,10 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, next_insn = next_insn_to_validate(file, insn); if (func && insn->func && func != insn->func->pfunc) { + /* Ignore KCFI type preambles, which always fall through */ + if (!strncmp(func->name, "__cfi_", 6)) + return 0; + WARN("%s() falls through to next function %s()", func->name, insn->func->name); return 1; @@ -4113,7 +4117,8 @@ static int validate_ibt(struct objtool_file *file) !strcmp(sec->name, "__bug_table") || !strcmp(sec->name, "__ex_table") || !strcmp(sec->name, "__jump_table") || - !strcmp(sec->name, "__mcount_loc")) + !strcmp(sec->name, "__mcount_loc") || + !strcmp(sec->name, ".kcfi_traps")) continue; list_for_each_entry(reloc, &sec->reloc->reloc_list, list) From dfb352ab1162f73b8c6dc98150fa32cf5aa2f623 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:55:00 -0700 Subject: [PATCH 37/46] kallsyms: Drop CONFIG_CFI_CLANG workarounds With -fsanitize=kcfi, the compiler no longer renames static functions with CONFIG_CFI_CLANG + ThinLTO. Drop the code that cleans up the ThinLTO hash from the function names. Signed-off-by: Sami Tolvanen Reviewed-by: Nick Desaulniers Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-19-samitolvanen@google.com --- kernel/kallsyms.c | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 3e7e2c2ad2f75..b27e6ea31f8bc 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -159,7 +159,6 @@ static bool cleanup_symbol_name(char *s) * character in an identifier in C. Suffixes observed: * - foo.llvm.[0-9a-f]+ * - foo.[0-9a-f]+ - * - foo.[0-9a-f]+.cfi_jt */ res = strchr(s, '.'); if (res) { @@ -167,22 +166,6 @@ static bool cleanup_symbol_name(char *s) return true; } - if (!IS_ENABLED(CONFIG_CFI_CLANG) || - !IS_ENABLED(CONFIG_LTO_CLANG_THIN) || - CONFIG_CLANG_VERSION >= 130000) - return false; - - /* - * Prior to LLVM 13, the following suffixes were observed when thinLTO - * and CFI are both enabled: - * - foo$[0-9]+ - */ - res = strrchr(s, '$'); - if (res) { - *res = '\0'; - return true; - } - return false; } From ca7e10bff196f69a450b9072a7b757713d3bb2dd Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:55:01 -0700 Subject: [PATCH 38/46] x86/tools/relocs: Ignore __kcfi_typeid_ relocations The compiler generates __kcfi_typeid_ symbols for annotating assembly functions with type information. These are constants that can be referenced in assembly code and are resolved by the linker. Ignore them in relocs. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-20-samitolvanen@google.com --- arch/x86/tools/relocs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c index e2c5b296120d6..2925074b9a588 100644 --- a/arch/x86/tools/relocs.c +++ b/arch/x86/tools/relocs.c @@ -56,6 +56,7 @@ static const char * const sym_regex_kernel[S_NSYMTYPES] = { "^(xen_irq_disable_direct_reloc$|" "xen_save_fl_direct_reloc$|" "VDSO|" + "__kcfi_typeid_|" "__crc_)", /* From ccace936eec7b805e1ab9268a6d163a00047b3a9 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:55:02 -0700 Subject: [PATCH 39/46] x86: Add types to indirectly called assembly functions With CONFIG_CFI_CLANG, assembly functions indirectly called from C code must be annotated with type identifiers to pass CFI checking. Define the __CFI_TYPE helper macro to match the compiler generated function preamble, and ensure SYM_TYPED_FUNC_START also emits ENDBR with IBT. Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-21-samitolvanen@google.com --- arch/x86/crypto/blowfish-x86_64-asm_64.S | 5 +++-- arch/x86/include/asm/linkage.h | 12 ++++++++++++ arch/x86/lib/memcpy_64.S | 3 ++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/arch/x86/crypto/blowfish-x86_64-asm_64.S b/arch/x86/crypto/blowfish-x86_64-asm_64.S index 802d715826891..4a43e072d2d1d 100644 --- a/arch/x86/crypto/blowfish-x86_64-asm_64.S +++ b/arch/x86/crypto/blowfish-x86_64-asm_64.S @@ -6,6 +6,7 @@ */ #include +#include .file "blowfish-x86_64-asm.S" .text @@ -141,7 +142,7 @@ SYM_FUNC_START(__blowfish_enc_blk) RET; SYM_FUNC_END(__blowfish_enc_blk) -SYM_FUNC_START(blowfish_dec_blk) +SYM_TYPED_FUNC_START(blowfish_dec_blk) /* input: * %rdi: ctx * %rsi: dst @@ -332,7 +333,7 @@ SYM_FUNC_START(__blowfish_enc_blk_4way) RET; SYM_FUNC_END(__blowfish_enc_blk_4way) -SYM_FUNC_START(blowfish_dec_blk_4way) +SYM_TYPED_FUNC_START(blowfish_dec_blk_4way) /* input: * %rdi: ctx * %rsi: dst diff --git a/arch/x86/include/asm/linkage.h b/arch/x86/include/asm/linkage.h index 73ca200498356..f484d656d34ee 100644 --- a/arch/x86/include/asm/linkage.h +++ b/arch/x86/include/asm/linkage.h @@ -43,6 +43,18 @@ #endif /* __ASSEMBLY__ */ +#define __CFI_TYPE(name) \ + SYM_START(__cfi_##name, SYM_L_LOCAL, SYM_A_NONE) \ + .fill 11, 1, 0x90 ASM_NL \ + .byte 0xb8 ASM_NL \ + .long __kcfi_typeid_##name ASM_NL \ + SYM_FUNC_END(__cfi_##name) + +/* SYM_TYPED_FUNC_START -- use for indirectly called globals, w/ CFI type */ +#define SYM_TYPED_FUNC_START(name) \ + SYM_TYPED_START(name, SYM_L_GLOBAL, SYM_A_ALIGN) \ + ENDBR + /* SYM_FUNC_START -- use for global functions */ #define SYM_FUNC_START(name) \ SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN) \ diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S index d0d7b9bc6cad3..dd8cd8831251f 100644 --- a/arch/x86/lib/memcpy_64.S +++ b/arch/x86/lib/memcpy_64.S @@ -2,6 +2,7 @@ /* Copyright 2002 Andi Kleen */ #include +#include #include #include #include @@ -27,7 +28,7 @@ * Output: * rax original destination */ -SYM_FUNC_START(__memcpy) +SYM_TYPED_FUNC_START(__memcpy) ALTERNATIVE_2 "jmp memcpy_orig", "", X86_FEATURE_REP_GOOD, \ "jmp memcpy_erms", X86_FEATURE_ERMS From a4b7a12c5594fe5e6ab2a5aa514a9ae3c0b85573 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:55:03 -0700 Subject: [PATCH 40/46] x86/purgatory: Disable CFI Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro. Signed-off-by: Sami Tolvanen Reviewed-by: Nick Desaulniers Reviewed-by: Kees Cook Tested-by: Nick Desaulniers Tested-by: Sedat Dilek Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-22-samitolvanen@google.com --- arch/x86/purgatory/Makefile | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile index 31c634a228183..58a200dc762d6 100644 --- a/arch/x86/purgatory/Makefile +++ b/arch/x86/purgatory/Makefile @@ -55,6 +55,10 @@ ifdef CONFIG_RETPOLINE PURGATORY_CFLAGS_REMOVE += $(RETPOLINE_CFLAGS) endif +ifdef CONFIG_CFI_CLANG +PURGATORY_CFLAGS_REMOVE += $(CC_FLAGS_CFI) +endif + CFLAGS_REMOVE_purgatory.o += $(PURGATORY_CFLAGS_REMOVE) CFLAGS_purgatory.o += $(PURGATORY_CFLAGS) From 3c516f89e17e56b4738f05588e51267e295b5e63 Mon Sep 17 00:00:00 2001 From: Sami Tolvanen Date: Thu, 8 Sep 2022 14:55:04 -0700 Subject: [PATCH 41/46] x86: Add support for CONFIG_CFI_CLANG With CONFIG_CFI_CLANG, the compiler injects a type preamble immediately before each function and a check to validate the target function type before indirect calls: ; type preamble __cfi_function: mov , %eax function: ... ; indirect call check mov -,%r10d add -0x4(%r11),%r10d je .Ltmp1 ud2 .Ltmp1: call __x86_indirect_thunk_r11 Add error handling code for the ud2 traps emitted for the checks, and allow CONFIG_CFI_CLANG to be selected on x86_64. This produces the following oops on CFI failure (generated using lkdtm): [ 21.441706] CFI failure at lkdtm_indirect_call+0x16/0x20 [lkdtm] (target: lkdtm_increment_int+0x0/0x10 [lkdtm]; expected type: 0x7e0c52a) [ 21.444579] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI [ 21.445296] CPU: 0 PID: 132 Comm: sh Not tainted 5.19.0-rc8-00020-g9f27360e674c #1 [ 21.445296] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014 [ 21.445296] RIP: 0010:lkdtm_indirect_call+0x16/0x20 [lkdtm] [ 21.445296] Code: 52 1c c0 48 c7 c1 c5 50 1c c0 e9 25 48 2a cc 0f 1f 44 00 00 49 89 fb 48 c7 c7 50 b4 1c c0 41 ba 5b ad f3 81 45 03 53 f8 [ 21.445296] RSP: 0018:ffffa9f9c02ffdc0 EFLAGS: 00000292 [ 21.445296] RAX: 0000000000000027 RBX: ffffffffc01cb300 RCX: 385cbbd2e070a700 [ 21.445296] RDX: 0000000000000000 RSI: c0000000ffffdfff RDI: ffffffffc01cb450 [ 21.445296] RBP: 0000000000000006 R08: 0000000000000000 R09: ffffffff8d081610 [ 21.445296] R10: 00000000bcc90825 R11: ffffffffc01c2fc0 R12: 0000000000000000 [ 21.445296] R13: ffffa31b827a6000 R14: 0000000000000000 R15: 0000000000000002 [ 21.445296] FS: 00007f08b42216a0(0000) GS:ffffa31b9f400000(0000) knlGS:0000000000000000 [ 21.445296] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 21.445296] CR2: 0000000000c76678 CR3: 0000000001940000 CR4: 00000000000006f0 [ 21.445296] Call Trace: [ 21.445296] [ 21.445296] lkdtm_CFI_FORWARD_PROTO+0x30/0x50 [lkdtm] [ 21.445296] direct_entry+0x12d/0x140 [lkdtm] [ 21.445296] full_proxy_write+0x5d/0xb0 [ 21.445296] vfs_write+0x144/0x460 [ 21.445296] ? __x64_sys_wait4+0x5a/0xc0 [ 21.445296] ksys_write+0x69/0xd0 [ 21.445296] do_syscall_64+0x51/0xa0 [ 21.445296] entry_SYSCALL_64_after_hwframe+0x63/0xcd [ 21.445296] RIP: 0033:0x7f08b41a6fe1 [ 21.445296] Code: be 07 00 00 00 41 89 c0 e8 7e ff ff ff 44 89 c7 89 04 24 e8 91 c6 02 00 8b 04 24 48 83 c4 68 c3 48 63 ff b8 01 00 00 03 [ 21.445296] RSP: 002b:00007ffcdf65c2e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 [ 21.445296] RAX: ffffffffffffffda RBX: 00007f08b4221690 RCX: 00007f08b41a6fe1 [ 21.445296] RDX: 0000000000000012 RSI: 0000000000c738f0 RDI: 0000000000000001 [ 21.445296] RBP: 0000000000000001 R08: fefefefefefefeff R09: fefefefeffc5ff4e [ 21.445296] R10: 00007f08b42222b0 R11: 0000000000000246 R12: 0000000000c738f0 [ 21.445296] R13: 0000000000000012 R14: 00007ffcdf65c401 R15: 0000000000c70450 [ 21.445296] [ 21.445296] Modules linked in: lkdtm [ 21.445296] Dumping ftrace buffer: [ 21.445296] (ftrace buffer empty) [ 21.471442] ---[ end trace 0000000000000000 ]--- [ 21.471811] RIP: 0010:lkdtm_indirect_call+0x16/0x20 [lkdtm] [ 21.472467] Code: 52 1c c0 48 c7 c1 c5 50 1c c0 e9 25 48 2a cc 0f 1f 44 00 00 49 89 fb 48 c7 c7 50 b4 1c c0 41 ba 5b ad f3 81 45 03 53 f8 [ 21.474400] RSP: 0018:ffffa9f9c02ffdc0 EFLAGS: 00000292 [ 21.474735] RAX: 0000000000000027 RBX: ffffffffc01cb300 RCX: 385cbbd2e070a700 [ 21.475664] RDX: 0000000000000000 RSI: c0000000ffffdfff RDI: ffffffffc01cb450 [ 21.476471] RBP: 0000000000000006 R08: 0000000000000000 R09: ffffffff8d081610 [ 21.477127] R10: 00000000bcc90825 R11: ffffffffc01c2fc0 R12: 0000000000000000 [ 21.477959] R13: ffffa31b827a6000 R14: 0000000000000000 R15: 0000000000000002 [ 21.478657] FS: 00007f08b42216a0(0000) GS:ffffa31b9f400000(0000) knlGS:0000000000000000 [ 21.479577] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 21.480307] CR2: 0000000000c76678 CR3: 0000000001940000 CR4: 00000000000006f0 [ 21.481460] Kernel panic - not syncing: Fatal exception Signed-off-by: Sami Tolvanen Reviewed-by: Kees Cook Tested-by: Kees Cook Tested-by: Nathan Chancellor Acked-by: Peter Zijlstra (Intel) Tested-by: Peter Zijlstra (Intel) Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220908215504.3686827-23-samitolvanen@google.com --- arch/x86/Kconfig | 2 + arch/x86/include/asm/cfi.h | 22 ++++++++++ arch/x86/kernel/Makefile | 2 + arch/x86/kernel/cfi.c | 86 ++++++++++++++++++++++++++++++++++++++ arch/x86/kernel/traps.c | 4 +- 5 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 arch/x86/include/asm/cfi.h create mode 100644 arch/x86/kernel/cfi.c diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index f9920f1341c8d..1fe6a83dac051 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -107,6 +107,8 @@ config X86 select ARCH_SUPPORTS_PAGE_TABLE_CHECK if X86_64 select ARCH_SUPPORTS_NUMA_BALANCING if X86_64 select ARCH_SUPPORTS_KMAP_LOCAL_FORCE_MAP if NR_CPUS <= 4096 + select ARCH_SUPPORTS_CFI_CLANG if X86_64 + select ARCH_USES_CFI_TRAPS if X86_64 && CFI_CLANG select ARCH_SUPPORTS_LTO_CLANG select ARCH_SUPPORTS_LTO_CLANG_THIN select ARCH_USE_BUILTIN_BSWAP diff --git a/arch/x86/include/asm/cfi.h b/arch/x86/include/asm/cfi.h new file mode 100644 index 0000000000000..58dacd90daefc --- /dev/null +++ b/arch/x86/include/asm/cfi.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_X86_CFI_H +#define _ASM_X86_CFI_H + +/* + * Clang Control Flow Integrity (CFI) support. + * + * Copyright (C) 2022 Google LLC + */ + +#include + +#ifdef CONFIG_CFI_CLANG +enum bug_trap_type handle_cfi_failure(struct pt_regs *regs); +#else +static inline enum bug_trap_type handle_cfi_failure(struct pt_regs *regs) +{ + return BUG_TRAP_TYPE_NONE; +} +#endif /* CONFIG_CFI_CLANG */ + +#endif /* _ASM_X86_CFI_H */ diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index a20a5ebfacd73..1286a73ebdbc5 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -139,6 +139,8 @@ obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o obj-$(CONFIG_AMD_MEM_ENCRYPT) += sev.o +obj-$(CONFIG_CFI_CLANG) += cfi.o + ### # 64 bit specific files ifeq ($(CONFIG_X86_64),y) diff --git a/arch/x86/kernel/cfi.c b/arch/x86/kernel/cfi.c new file mode 100644 index 0000000000000..8674a5c0c031d --- /dev/null +++ b/arch/x86/kernel/cfi.c @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Clang Control Flow Integrity (CFI) support. + * + * Copyright (C) 2022 Google LLC + */ +#include +#include +#include +#include + +/* + * Returns the target address and the expected type when regs->ip points + * to a compiler-generated CFI trap. + */ +static bool decode_cfi_insn(struct pt_regs *regs, unsigned long *target, + u32 *type) +{ + char buffer[MAX_INSN_SIZE]; + struct insn insn; + int offset = 0; + + *target = *type = 0; + + /* + * The compiler generates the following instruction sequence + * for indirect call checks: + * + *   movl -, %r10d ; 6 bytes + * addl -4(%reg), %r10d ; 4 bytes + * je .Ltmp1 ; 2 bytes + * ud2 ; <- regs->ip + * .Ltmp1: + * + * We can decode the expected type and the target address from the + * movl/addl instructions. + */ + if (copy_from_kernel_nofault(buffer, (void *)regs->ip - 12, MAX_INSN_SIZE)) + return false; + if (insn_decode_kernel(&insn, &buffer[offset])) + return false; + if (insn.opcode.value != 0xBA) + return false; + + *type = -(u32)insn.immediate.value; + + if (copy_from_kernel_nofault(buffer, (void *)regs->ip - 6, MAX_INSN_SIZE)) + return false; + if (insn_decode_kernel(&insn, &buffer[offset])) + return false; + if (insn.opcode.value != 0x3) + return false; + + /* Read the target address from the register. */ + offset = insn_get_modrm_rm_off(&insn, regs); + if (offset < 0) + return false; + + *target = *(unsigned long *)((void *)regs + offset); + + return true; +} + +/* + * Checks if a ud2 trap is because of a CFI failure, and handles the trap + * if needed. Returns a bug_trap_type value similarly to report_bug. + */ +enum bug_trap_type handle_cfi_failure(struct pt_regs *regs) +{ + unsigned long target; + u32 type; + + if (!is_cfi_trap(regs->ip)) + return BUG_TRAP_TYPE_NONE; + + if (!decode_cfi_insn(regs, &target, &type)) + return report_cfi_failure_noaddr(regs, regs->ip); + + return report_cfi_failure(regs, regs->ip, &target, type); +} + +/* + * Ensure that __kcfi_typeid_ symbols are emitted for functions that may + * not be indirectly called with all configurations. + */ +__ADDRESSABLE(__memcpy) diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index d62b2cb85ceae..178015a820f08 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -63,6 +63,7 @@ #include #include #include +#include #ifdef CONFIG_X86_64 #include @@ -313,7 +314,8 @@ static noinstr bool handle_bug(struct pt_regs *regs) */ if (regs->flags & X86_EFLAGS_IF) raw_local_irq_enable(); - if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) { + if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN || + handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) { regs->ip += LEN_UD2; handled = true; } From 3e1730842f142add55dc658929221521a9ea62b6 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 19 Sep 2022 19:45:14 -0700 Subject: [PATCH 42/46] x86/entry: Work around Clang __bdos() bug Clang produces a false positive when building with CONFIG_FORTIFY_SOURCE=y and CONFIG_UBSAN_BOUNDS=y when operating on an array with a dynamic offset. Work around this by using a direct assignment of an empty instance. Avoids this warning: ../include/linux/fortify-string.h:309:4: warning: call to __write_overflow_field declared with 'warn ing' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wat tribute-warning] __write_overflow_field(p_size_field, size); ^ which was isolated to the memset() call in xen_load_idt(). Note that this looks very much like another bug that was worked around: https://github.com/ClangBuiltLinux/linux/issues/1592 Cc: Juergen Gross Cc: Boris Ostrovsky Cc: Thomas Gleixner Cc: Ingo Molnar Cc: Borislav Petkov Cc: Dave Hansen Cc: x86@kernel.org Cc: "H. Peter Anvin" Cc: xen-devel@lists.xenproject.org Reviewed-by: Boris Ostrovsky Link: https://lore.kernel.org/lkml/41527d69-e8ab-3f86-ff37-6b298c01d5bc@oracle.com Signed-off-by: Kees Cook --- arch/x86/xen/enlighten_pv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 0ed2e487a693f..9b1a58dda935b 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -765,6 +765,7 @@ static void xen_load_idt(const struct desc_ptr *desc) { static DEFINE_SPINLOCK(lock); static struct trap_info traps[257]; + static const struct trap_info zero = { }; unsigned out; trace_xen_cpu_load_idt(desc); @@ -774,7 +775,7 @@ static void xen_load_idt(const struct desc_ptr *desc) memcpy(this_cpu_ptr(&idt_desc), desc, sizeof(idt_desc)); out = xen_convert_trap_info(desc, traps, false); - memset(&traps[out], 0, sizeof(traps[0])); + traps[out] = zero; xen_mc_flush(); if (HYPERVISOR_set_trap_table(traps)) From fa35198f39571bbdae53c5b321020021eaad6bd2 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 19 Sep 2022 16:33:33 -0700 Subject: [PATCH 43/46] fortify: Explicitly check bounds are compile-time constants In preparation for replacing __builtin_object_size() with __builtin_dynamic_object_size(), all the compile-time size checks need to check that the bounds comparisons are, in fact, known at compile-time. Enforce what was guaranteed with __bos(). In other words, since all uses of __bos() were constant expressions, it was not required to test for this. When these change to __bdos(), they _may_ be constant expressions, and the checks are only valid when the prior condition holds. This results in no binary differences. Cc: linux-hardening@vger.kernel.org Link: https://lore.kernel.org/lkml/20220920192202.190793-3-keescook@chromium.org Signed-off-by: Kees Cook --- include/linux/fortify-string.h | 49 +++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index ff879efe94ed5..1c582224c5259 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -80,6 +80,11 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) #define POS __pass_object_size(1) #define POS0 __pass_object_size(0) +#define __compiletime_lessthan(bounds, length) ( \ + __builtin_constant_p((bounds) < (length)) && \ + (bounds) < (length) \ +) + /** * strncpy - Copy a string to memory with non-guaranteed NUL padding * @@ -117,7 +122,7 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size) { size_t p_size = __builtin_object_size(p, 1); - if (__builtin_constant_p(size) && p_size < size) + if (__compiletime_lessthan(p_size, size)) __write_overflow(); if (p_size < size) fortify_panic(__func__); @@ -224,7 +229,7 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s * 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) + if (__compiletime_lessthan(p_size, size)) __write_overflow(); /* @@ -281,15 +286,16 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size, /* * Length argument is a constant expression, so we * can perform compile-time bounds checking where - * buffer sizes are known. + * buffer sizes are also known at compile time. */ /* Error when size is larger than enclosing struct. */ - if (p_size > p_size_field && p_size < size) + if (__compiletime_lessthan(p_size_field, p_size) && + __compiletime_lessthan(p_size, size)) __write_overflow(); /* Warn when write size is larger than dest field. */ - if (p_size_field < size) + if (__compiletime_lessthan(p_size_field, size)) __write_overflow_field(p_size_field, size); } /* @@ -365,25 +371,28 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size, /* * Length argument is a constant expression, so we * can perform compile-time bounds checking where - * buffer sizes are known. + * buffer sizes are also known at compile time. */ /* Error when size is larger than enclosing struct. */ - if (p_size > p_size_field && p_size < size) + if (__compiletime_lessthan(p_size_field, p_size) && + __compiletime_lessthan(p_size, size)) __write_overflow(); - if (q_size > q_size_field && q_size < size) + if (__compiletime_lessthan(q_size_field, q_size) && + __compiletime_lessthan(q_size, size)) __read_overflow2(); /* Warn when write size argument larger than dest field. */ - if (p_size_field < size) + if (__compiletime_lessthan(p_size_field, size)) __write_overflow_field(p_size_field, size); /* * 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) + if ((IS_ENABLED(KBUILD_EXTRA_WARN1) || + __compiletime_lessthan(p_size_field, size)) && + __compiletime_lessthan(q_size_field, size)) __read_overflow2_field(q_size_field, size); } /* @@ -494,7 +503,7 @@ __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size) { size_t p_size = __builtin_object_size(p, 0); - if (__builtin_constant_p(size) && p_size < size) + if (__compiletime_lessthan(p_size, size)) __read_overflow(); if (p_size < size) fortify_panic(__func__); @@ -508,9 +517,9 @@ int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t size_t q_size = __builtin_object_size(q, 0); if (__builtin_constant_p(size)) { - if (p_size < size) + if (__compiletime_lessthan(p_size, size)) __read_overflow(); - if (q_size < size) + if (__compiletime_lessthan(q_size, size)) __read_overflow2(); } if (p_size < size || q_size < size) @@ -523,7 +532,7 @@ void *memchr(const void * const POS0 p, int c, __kernel_size_t size) { size_t p_size = __builtin_object_size(p, 0); - if (__builtin_constant_p(size) && p_size < size) + if (__compiletime_lessthan(p_size, size)) __read_overflow(); if (p_size < size) fortify_panic(__func__); @@ -535,7 +544,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size) { size_t p_size = __builtin_object_size(p, 0); - if (__builtin_constant_p(size) && p_size < size) + if (__compiletime_lessthan(p_size, size)) __read_overflow(); if (p_size < size) fortify_panic(__func__); @@ -547,7 +556,7 @@ __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp { size_t p_size = __builtin_object_size(p, 0); - if (__builtin_constant_p(size) && p_size < size) + if (__compiletime_lessthan(p_size, size)) __read_overflow(); if (p_size < size) fortify_panic(__func__); @@ -563,11 +572,13 @@ char *strcpy(char * const POS p, const char * const POS q) size_t size; /* If neither buffer size is known, immediately give up. */ - if (p_size == SIZE_MAX && q_size == SIZE_MAX) + if (__builtin_constant_p(p_size) && + __builtin_constant_p(q_size) && + p_size == SIZE_MAX && q_size == SIZE_MAX) return __underlying_strcpy(p, q); size = strlen(q) + 1; /* Compile-time check for const size overflow. */ - if (__builtin_constant_p(size) && p_size < size) + if (__compiletime_lessthan(p_size, size)) __write_overflow(); /* Run-time check for dynamic size overflow. */ if (p_size < size) From 9f7d69c5cd23904a29178a7ecc4eee9c1cfba04b Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 19 Sep 2022 19:50:32 -0700 Subject: [PATCH 44/46] fortify: Convert to struct vs member helpers In preparation for adding support for __builtin_dynamic_object_size(), wrap each instance of __builtin_object_size(p, N) with either the new __struct_size(p) as __bos(p, 0), or __member_size(p) as __bos(p, 1). This will allow us to replace the definitions with __bdos() next. There are no binary differences from this change. Cc: Nathan Chancellor Cc: Nick Desaulniers Cc: Tom Rix Cc: linux-hardening@vger.kernel.org Cc: llvm@lists.linux.dev Link: https://lore.kernel.org/lkml/20220920192202.190793-4-keescook@chromium.org Signed-off-by: Kees Cook --- include/linux/fortify-string.h | 68 +++++++++++++++++----------------- 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index 1c582224c5259..b62c90cfafaf1 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -20,7 +20,7 @@ void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning(" ({ \ unsigned char *__p = (unsigned char *)(p); \ size_t __ret = SIZE_MAX; \ - size_t __p_size = __builtin_object_size(p, 1); \ + size_t __p_size = __member_size(p); \ if (__p_size != SIZE_MAX && \ __builtin_constant_p(*__p)) { \ size_t __p_len = __p_size - 1; \ @@ -72,13 +72,15 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) __underlying_memcpy(dst, src, bytes) /* - * Clang's use of __builtin_object_size() within inlines needs hinting via - * __pass_object_size(). The preference is to only ever use type 1 (member + * Clang's use of __builtin_*object_size() within inlines needs hinting via + * __pass_*object_size(). The preference is to only ever use type 1 (member * size, rather than struct size), but there remain some stragglers using * type 0 that will be converted in the future. */ -#define POS __pass_object_size(1) -#define POS0 __pass_object_size(0) +#define POS __pass_object_size(1) +#define POS0 __pass_object_size(0) +#define __struct_size(p) __builtin_object_size(p, 0) +#define __member_size(p) __builtin_object_size(p, 1) #define __compiletime_lessthan(bounds, length) ( \ __builtin_constant_p((bounds) < (length)) && \ @@ -120,7 +122,7 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) __FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3) char *strncpy(char * const POS p, const char *q, __kernel_size_t size) { - size_t p_size = __builtin_object_size(p, 1); + size_t p_size = __member_size(p); if (__compiletime_lessthan(p_size, size)) __write_overflow(); @@ -132,7 +134,7 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size) __FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2) char *strcat(char * const POS p, const char *q) { - size_t p_size = __builtin_object_size(p, 1); + size_t p_size = __member_size(p); if (p_size == SIZE_MAX) return __underlying_strcat(p, q); @@ -144,7 +146,7 @@ char *strcat(char * const POS p, const char *q) extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen); __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size_t maxlen) { - size_t p_size = __builtin_object_size(p, 1); + size_t p_size = __member_size(p); size_t p_len = __compiletime_strlen(p); size_t ret; @@ -174,7 +176,7 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1) __kernel_size_t __fortify_strlen(const char * const POS p) { __kernel_size_t ret; - size_t p_size = __builtin_object_size(p, 1); + size_t p_size = __member_size(p); /* Give up if we don't know how large p is. */ if (p_size == SIZE_MAX) @@ -189,8 +191,8 @@ __kernel_size_t __fortify_strlen(const char * const POS p) extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy); __FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, size_t size) { - size_t p_size = __builtin_object_size(p, 1); - size_t q_size = __builtin_object_size(q, 1); + size_t p_size = __member_size(p); + size_t q_size = __member_size(q); size_t q_len; /* Full count of source string length. */ size_t len; /* Count of characters going into destination. */ @@ -218,8 +220,8 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s { 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); + size_t p_size = __member_size(p); + size_t q_size = __member_size(q); /* If we cannot get size of p and q default to call strscpy. */ if (p_size == SIZE_MAX && q_size == SIZE_MAX) @@ -264,8 +266,8 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strncat, 1, 2, 3) char *strncat(char * const POS p, const char * const POS q, __kernel_size_t count) { size_t p_len, copy_len; - size_t p_size = __builtin_object_size(p, 1); - size_t q_size = __builtin_object_size(q, 1); + size_t p_size = __member_size(p); + size_t q_size = __member_size(q); if (p_size == SIZE_MAX && q_size == SIZE_MAX) return __underlying_strncat(p, q, count); @@ -323,11 +325,11 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size, }) /* - * __builtin_object_size() must be captured here to avoid evaluating argument - * side-effects further into the macro layers. + * __struct_size() vs __member_size() must be captured here to avoid + * evaluating argument side-effects further into the macro layers. */ #define memset(p, c, s) __fortify_memset_chk(p, c, s, \ - __builtin_object_size(p, 0), __builtin_object_size(p, 1)) + __struct_size(p), __member_size(p)) /* * To make sure the compiler can enforce protection against buffer overflows, @@ -420,7 +422,7 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size, * fake flexible arrays, until they are all converted to * proper flexible arrays. * - * The implementation of __builtin_object_size() behaves + * 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 @@ -486,22 +488,22 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size, */ /* - * __builtin_object_size() must be captured here to avoid evaluating argument - * side-effects further into the macro layers. + * __struct_size() vs __member_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), \ + __struct_size(p), __struct_size(q), \ + __member_size(p), __member_size(q), \ memcpy) #define memmove(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), \ + __struct_size(p), __struct_size(q), \ + __member_size(p), __member_size(q), \ memmove) extern void *__real_memscan(void *, int, __kernel_size_t) __RENAME(memscan); __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size) { - size_t p_size = __builtin_object_size(p, 0); + size_t p_size = __struct_size(p); if (__compiletime_lessthan(p_size, size)) __read_overflow(); @@ -513,8 +515,8 @@ __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size) __FORTIFY_INLINE __diagnose_as(__builtin_memcmp, 1, 2, 3) int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t size) { - size_t p_size = __builtin_object_size(p, 0); - size_t q_size = __builtin_object_size(q, 0); + size_t p_size = __struct_size(p); + size_t q_size = __struct_size(q); if (__builtin_constant_p(size)) { if (__compiletime_lessthan(p_size, size)) @@ -530,7 +532,7 @@ int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t __FORTIFY_INLINE __diagnose_as(__builtin_memchr, 1, 2, 3) void *memchr(const void * const POS0 p, int c, __kernel_size_t size) { - size_t p_size = __builtin_object_size(p, 0); + size_t p_size = __struct_size(p); if (__compiletime_lessthan(p_size, size)) __read_overflow(); @@ -542,7 +544,7 @@ void *memchr(const void * const POS0 p, int c, __kernel_size_t size) void *__real_memchr_inv(const void *s, int c, size_t n) __RENAME(memchr_inv); __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size) { - size_t p_size = __builtin_object_size(p, 0); + size_t p_size = __struct_size(p); if (__compiletime_lessthan(p_size, size)) __read_overflow(); @@ -554,7 +556,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size) extern void *__real_kmemdup(const void *src, size_t len, gfp_t gfp) __RENAME(kmemdup); __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp) { - size_t p_size = __builtin_object_size(p, 0); + size_t p_size = __struct_size(p); if (__compiletime_lessthan(p_size, size)) __read_overflow(); @@ -567,8 +569,8 @@ __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp __FORTIFY_INLINE __diagnose_as(__builtin_strcpy, 1, 2) char *strcpy(char * const POS p, const char * const POS q) { - size_t p_size = __builtin_object_size(p, 1); - size_t q_size = __builtin_object_size(q, 1); + size_t p_size = __member_size(p); + size_t q_size = __member_size(q); size_t size; /* If neither buffer size is known, immediately give up. */ From f67b90be20097294cd58b7db91435f4f3278d75f Mon Sep 17 00:00:00 2001 From: Bill Wendling Date: Fri, 2 Sep 2022 21:37:49 +0000 Subject: [PATCH 45/46] x86/paravirt: clean up typos and grammaros Drive-by clean up of the comment. [ Impact: cleanup] Signed-off-by: Bill Wendling Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220902213750.1124421-2-morbo@google.com --- arch/x86/include/asm/paravirt_types.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h index 89df6c6617f50..f04157456a49a 100644 --- a/arch/x86/include/asm/paravirt_types.h +++ b/arch/x86/include/asm/paravirt_types.h @@ -328,7 +328,7 @@ int paravirt_disable_iospace(void); * Unfortunately, this is a relatively slow operation for modern CPUs, * because it cannot necessarily determine what the destination * address is. In this case, the address is a runtime constant, so at - * the very least we can patch the call to e a simple direct call, or + * the very least we can patch the call to a simple direct call, or, * ideally, patch an inline implementation into the callsite. (Direct * calls are essentially free, because the call and return addresses * are completely predictable.) @@ -339,10 +339,10 @@ int paravirt_disable_iospace(void); * on the stack. All caller-save registers (eax,edx,ecx) are expected * to be modified (either clobbered or used for return values). * X86_64, on the other hand, already specifies a register-based calling - * conventions, returning at %rax, with parameters going on %rdi, %rsi, + * conventions, returning at %rax, with parameters going in %rdi, %rsi, * %rdx, and %rcx. Note that for this reason, x86_64 does not need any * special handling for dealing with 4 arguments, unlike i386. - * However, x86_64 also have to clobber all caller saved registers, which + * However, x86_64 also has to clobber all caller saved registers, which * unfortunately, are quite a bit (r8 - r11) * * The call instruction itself is marked by placing its start address @@ -360,22 +360,22 @@ int paravirt_disable_iospace(void); * There are 5 sets of PVOP_* macros for dealing with 0-4 arguments. * It could be extended to more arguments, but there would be little * to be gained from that. For each number of arguments, there are - * the two VCALL and CALL variants for void and non-void functions. + * two VCALL and CALL variants for void and non-void functions. * * When there is a return value, the invoker of the macro must specify * the return type. The macro then uses sizeof() on that type to - * determine whether its a 32 or 64 bit value, and places the return + * determine whether it's a 32 or 64 bit value and places the return * in the right register(s) (just %eax for 32-bit, and %edx:%eax for - * 64-bit). For x86_64 machines, it just returns at %rax regardless of + * 64-bit). For x86_64 machines, it just returns in %rax regardless of * the return value size. * - * 64-bit arguments are passed as a pair of adjacent 32-bit arguments + * 64-bit arguments are passed as a pair of adjacent 32-bit arguments; * i386 also passes 64-bit arguments as a pair of adjacent 32-bit arguments * in low,high order * * Small structures are passed and returned in registers. The macro * calling convention can't directly deal with this, so the wrapper - * functions must do this. + * functions must do it. * * These PVOP_* macros are only defined within this header. This * means that all uses must be wrapped in inline functions. This also From 8c86f29bfb18465d15b05cfd26a6454ec787b793 Mon Sep 17 00:00:00 2001 From: Bill Wendling Date: Fri, 2 Sep 2022 21:37:50 +0000 Subject: [PATCH 46/46] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled The ZERO_CALL_USED_REGS feature may zero out caller-saved registers before returning. In spurious_kernel_fault(), the "pte_offset_kernel()" call results in this assembly code: .Ltmp151: #APP # ALT: oldnstr .Ltmp152: .Ltmp153: .Ltmp154: .section .discard.retpoline_safe,"",@progbits .quad .Ltmp154 .text callq *pv_ops+536(%rip) .Ltmp155: .section .parainstructions,"a",@progbits .p2align 3, 0x0 .quad .Ltmp153 .byte 67 .byte .Ltmp155-.Ltmp153 .short 1 .text .Ltmp156: # ALT: padding .zero (-(((.Ltmp157-.Ltmp158)-(.Ltmp156-.Ltmp152))>0))*((.Ltmp157-.Ltmp158)-(.Ltmp156-.Ltmp152)),144 .Ltmp159: .section .altinstructions,"a",@progbits .Ltmp160: .long .Ltmp152-.Ltmp160 .Ltmp161: .long .Ltmp158-.Ltmp161 .short 33040 .byte .Ltmp159-.Ltmp152 .byte .Ltmp157-.Ltmp158 .text .section .altinstr_replacement,"ax",@progbits # ALT: replacement 1 .Ltmp158: movq %rdi, %rax .Ltmp157: .text #NO_APP .Ltmp162: testb $-128, %dil The "testb" here is using %dil, but the %rdi register was cleared before returning from "callq *pv_ops+536(%rip)". Adding the proper constraints results in the use of a different register: movq %r11, %rdi # Similar to above. testb $-128, %r11b Link: https://github.com/KSPP/linux/issues/192 Signed-off-by: Bill Wendling Reported-and-tested-by: Nathan Chancellor Fixes: 035f7f87b729 ("randstruct: Enable Clang support") Reviewed-by: Juergen Gross Link: https://lore.kernel.org/lkml/fa6df43b-8a1a-8ad1-0236-94d2a0b588fa@suse.com/ Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20220902213750.1124421-3-morbo@google.com --- arch/x86/include/asm/paravirt_types.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h index f04157456a49a..b1ab5d94881b1 100644 --- a/arch/x86/include/asm/paravirt_types.h +++ b/arch/x86/include/asm/paravirt_types.h @@ -414,8 +414,17 @@ int paravirt_disable_iospace(void); "=c" (__ecx) #define PVOP_CALL_CLOBBERS PVOP_VCALL_CLOBBERS, "=a" (__eax) -/* void functions are still allowed [re]ax for scratch */ +/* + * void functions are still allowed [re]ax for scratch. + * + * The ZERO_CALL_USED REGS feature may end up zeroing out callee-saved + * registers. Make sure we model this with the appropriate clobbers. + */ +#ifdef CONFIG_ZERO_CALL_USED_REGS +#define PVOP_VCALLEE_CLOBBERS "=a" (__eax), PVOP_VCALL_CLOBBERS +#else #define PVOP_VCALLEE_CLOBBERS "=a" (__eax) +#endif #define PVOP_CALLEE_CLOBBERS PVOP_VCALLEE_CLOBBERS #define EXTRA_CLOBBERS , "r8", "r9", "r10", "r11"