| Message ID | 20221219061758.23321-1-ashimida.1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC/RFT] CFI: Add support for gcc CFI in aarch64 | expand |
+ Cc: linux-hardening@vger.kernel.org On 12/18, Dan Li wrote: > Based on Sami's patch[1], this patch makes the corresponding kernel > configuration of CFI available when compiling the kernel with the gcc[2]. > > The code after enabling cfi is as follows: > > int (*p)(void); > int func (int) > { > p(); > } > > __cfi_func: > .4byte 0x439d3502 > func: > ...... > adrp x0, p > add x0, x0, :lo12:p > mov w1, 23592 > movk w1, 0x4601, lsl 16 > cmp w0, w1 > beq .L2 > ...... > bl cfi_check_failed > .L2: > blr x19 > ret > > In the compiler part[4], there are some differences from Sami's > implementation[3], mainly including:
On Sun, Dec 18, 2022 at 10:17:58PM -0800, Dan Li wrote: > In the compiler part[4], there are some differences from Sami's > implementation[3], mainly including: > > 1. When a typeid mismatch is detected, the cfi_check_failed function > will be called instead of the brk instruction. This function needs > to be implemented by the compiler user. > If there are user mode programs or other systems that want to use > this feature, it may be more convenient to use a callback (so this > compilation option is set to -fsanitize=cfi instead of kcfi). This is not going to be acceptible for x86_64. > 2. A reserved typeid (such as 0x0U on the aarch64 platform) is always > inserted in front of functions that should not be called indirectly. > Functions that can be called indirectly will not use this hash value, > which prevents instructions/data before the function from being used > as a typeid by an attacker. > > 3. Some bits are ignored in the typeid to avoid conflicts between the > typeid and the instruction set of a specific platform, thereby > preventing an attacker from bypassing the CFI check by using the > instruction as a typeid, such as on the aarch64 platform: > * If the following instruction sequence exists: > 400620: a9be7bfd stp x29, x30, [sp, #-32]! > 400624: 910003fd mov x29, sp > 400628: f9000bf3 str x19, [sp, #16] > * If the expected typeid of the indirect call is exactly 0x910003fd, > the attacker can jump to the next instruction position of any > "mov x29,sp" instruction (such as 0x400628 here). > > 4. Insert a symbol __cfi_<function> before each function's typeid, > which may be helpful for fine-grained KASLR implementations (or not?). > > 5. The current implementation of gcc only supports the aarch64 platform. What, if any, are the plans for x86_64 support?
Hi Peter, On 12/19, Peter Zijlstra wrote: > On Sun, Dec 18, 2022 at 10:17:58PM -0800, Dan Li wrote: > > > 1. When a typeid mismatch is detected, the cfi_check_failed function > > will be called instead of the brk instruction. This function needs > > to be implemented by the compiler user. > > If there are user mode programs or other systems that want to use > > this feature, it may be more convenient to use a callback (so this > > compilation option is set to -fsanitize=cfi instead of kcfi). > > This is not going to be acceptible for x86_64. I'm not familiar enough with the x86_64 platform, could you please tell me why this is not acceptable? Is there a similar situation on the arm64 platform? > > 5. The current implementation of gcc only supports the aarch64 platform. > > What, if any, are the plans for x86_64 support? I'd like to implement something similar on x86_64 later, but currently I'm doing this in my spare time, so it might take a little longer. :( Thanks, Dan
On Mon, Dec 19, 2022 at 05:32:04AM -0800, Dan Li wrote: > Hi Peter, > > On 12/19, Peter Zijlstra wrote: > > On Sun, Dec 18, 2022 at 10:17:58PM -0800, Dan Li wrote: > > > > > 1. When a typeid mismatch is detected, the cfi_check_failed function > > > will be called instead of the brk instruction. This function needs > > > to be implemented by the compiler user. > > > If there are user mode programs or other systems that want to use > > > this feature, it may be more convenient to use a callback (so this > > > compilation option is set to -fsanitize=cfi instead of kcfi). > > > > This is not going to be acceptible for x86_64. > > I'm not familiar enough with the x86_64 platform, could you please > tell me why this is not acceptable? Is there a similar situation > on the arm64 platform? Mostly because the call would be a 5 byte instruction while the trap (UD2) is only 2 bytes. I suspect Argh64 has a similar problem if the to be called function is outside the immediate range (26 bits or thereabout), in which case you end up with a multi-instruction sequence to construct the call target or so. A trap is always a single instruction.
On 12/19, Peter Zijlstra wrote: > On Mon, Dec 19, 2022 at 05:32:04AM -0800, Dan Li wrote: > > Hi Peter, > > > > On 12/19, Peter Zijlstra wrote: > > > On Sun, Dec 18, 2022 at 10:17:58PM -0800, Dan Li wrote: > > > > > > > 1. When a typeid mismatch is detected, the cfi_check_failed function > > > > will be called instead of the brk instruction. This function needs > > > > to be implemented by the compiler user. > > > > If there are user mode programs or other systems that want to use > > > > this feature, it may be more convenient to use a callback (so this > > > > compilation option is set to -fsanitize=cfi instead of kcfi). > > > > > > This is not going to be acceptible for x86_64. > > > > I'm not familiar enough with the x86_64 platform, could you please > > tell me why this is not acceptable? Is there a similar situation > > on the arm64 platform? > > Mostly because the call would be a 5 byte instruction while the trap > (UD2) is only 2 bytes. Oh ok, got it. > I suspect Argh64 has a similar problem if the to be called function is > outside the immediate range (26 bits or thereabout), in which case you > end up with a multi-instruction sequence to construct the call target or > so. A trap is always a single instruction. > Yes, IIRC, long jumps also typically require at least three instructions in arm64. Thanks, Dan.
On Sun, Dec 18, 2022 at 10:17:58PM -0800, Dan Li wrote: > Based on Sami's patch[1], this patch makes the corresponding kernel > configuration of CFI available when compiling the kernel with the gcc[2]. > > The code after enabling cfi is as follows: > > int (*p)(void); > int func (int) > { > p(); > } > > __cfi_func: > .4byte 0x439d3502 > func: > ...... > adrp x0, p > add x0, x0, :lo12:p > mov w1, 23592 > movk w1, 0x4601, lsl 16 > cmp w0, w1 > beq .L2 > ...... > bl cfi_check_failed > .L2: > blr x19 > ret > > In the compiler part[4], there are some differences from Sami's > implementation[3], mainly including: > > 1. When a typeid mismatch is detected, the cfi_check_failed function > will be called instead of the brk instruction. This function needs > to be implemented by the compiler user. For arm64, we specifically wanted the BRK approach (with the information endoed into an immediate) because it gave us all the information we needed to diagnose the failed check, while giving the compiler freedom to perform the check however it wanted, and without needing to shuffle a bunch of registers (or having a weird calling convention for the cfi_check_failed handler). > If there are user mode programs or other systems that want to use > this feature, it may be more convenient to use a callback (so this > compilation option is set to -fsanitize=cfi instead of kcfi). I appreciate that may be nicer for userspace, but it would be far nicer for the kernel if we could have a kcfi mode that behaves the same as LLVM, using a BRK. That's going to be simpler for the kernel to deal with, and should result in nicer code / smaller binary size (for the reasons given above). Can we please have an LLVM-compatible KCFI mode, and have the -fsanitize=cfi be a separate option from -fsanitize=kcfi? > 2. A reserved typeid (such as 0x0U on the aarch64 platform) is always > inserted in front of functions that should not be called indirectly. > Functions that can be called indirectly will not use this hash value, > which prevents instructions/data before the function from being used > as a typeid by an attacker. That sounds sensible, though it meanse we'll need to go audit all the assembly without type annotations. I presume that "functions that should not be called indirectly" only includes those which are not directly visible outside the compilation unit AND whose address is never taken / escaped from the compilation unit. Is that the case? > 3. Some bits are ignored in the typeid to avoid conflicts between the > typeid and the instruction set of a specific platform, thereby > preventing an attacker from bypassing the CFI check by using the > instruction as a typeid, such as on the aarch64 platform: > * If the following instruction sequence exists: > 400620: a9be7bfd stp x29, x30, [sp, #-32]! > 400624: 910003fd mov x29, sp > 400628: f9000bf3 str x19, [sp, #16] > * If the expected typeid of the indirect call is exactly 0x910003fd, > the attacker can jump to the next instruction position of any > "mov x29,sp" instruction (such as 0x400628 here). Which bits exactly are ignored on arm64? e.g. are these encoded into UDF immediates? > 4. Insert a symbol __cfi_<function> before each function's typeid, > which may be helpful for fine-grained KASLR implementations (or not?). I can imagine this is useful, but I am not immediately sure. > 5. The current implementation of gcc only supports the aarch64 platform. > > This produces the following oops on CFI failure (generated using lkdtm): > > /kselftest_install/lkdtm # ./CFI_FORWARD_PROTO.sh > [ 74.856516] lkdtm: Performing direct entry CFI_FORWARD_PROTO > [ 74.856878] lkdtm: Calling matched prototype ... > [ 74.857011] lkdtm: Calling mismatched prototype ... > [ 74.857133] CFI failure at lkdtm_indirect_call+0x30/0x50 (target: lkdtm_increment_int+0x0/0x1c; expected type: 0xc59c68f1) > [ 74.858185] Kernel panic - not syncing: Oops - CFI > [ 74.859240] CPU: 0 PID: 129 Comm: cat Not tainted 6.0.0-rc4-00024-g32bf7f14f497-dirty #150 > [ 74.859481] Hardware name: linux,dummy-virt (DT) > [ 74.859795] Call trace: > [ 74.859959] dump_backtrace.part.0+0xcc/0xe0 > [ 74.860212] show_stack+0x18/0x5c > [ 74.860327] dump_stack_lvl+0x64/0x84 > [ 74.860398] dump_stack+0x18/0x38 > [ 74.860443] panic+0x170/0x36c > [ 74.860496] cfi_check_failed+0x38/0x44 > [ 74.860564] lkdtm_indirect_call+0x30/0x50 > [ 74.860614] lkdtm_CFI_FORWARD_PROTO+0x3c/0x6c > [ 74.860701] lkdtm_do_action+0x44/0x58 > [ 74.860764] direct_entry+0x148/0x160 > [ 74.860814] full_proxy_write+0x74/0xe0 > [ 74.860874] vfs_write+0xd8/0x2d0 > [ 74.860942] ksys_write+0x70/0x110 > [ 74.861000] __arm64_sys_write+0x1c/0x30 > [ 74.861067] invoke_syscall+0x5c/0x140 > [ 74.861117] el0_svc_common.constprop.0+0x44/0xf0 > [ 74.861190] do_el0_svc+0x2c/0xc0 > [ 74.861233] el0_svc+0x20/0x60 > [ 74.861287] el0t_64_sync_handler+0xf4/0x124 > [ 74.861340] el0t_64_sync+0x160/0x164 > [ 74.861782] SMP: stopping secondary CPUs > [ 74.862336] Kernel Offset: disabled > [ 74.862439] CPU features: 0x0000,00075024,699418af > [ 74.862799] Memory Limit: none > [ 74.863373] ---[ end Kernel panic - not syncing: Oops - CFI ]--- > > The gcc-related patches[4] are based on tag: releases/gcc-12.2.0. > > Any suggestion please let me know :). As a general thing, how does this work with -fpatchable-function-entry=M,N, where N is non-zero? We still need to fix that for LLVM, and it would be good to align on the same behaviour. Thanks, Mark. > > Thanks, Dan. > > [1] https://lore.kernel.org/all/20220908215504.3686827-1-samitolvanen@google.com/ > [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107048 > [3] https://reviews.llvm.org/D119296 > [4] https://lore.kernel.org/linux-hardening/20221219055431.22596-1-ashimida.1990@gmail.com/ > > Signed-off-by: Dan Li <ashimida.1990@gmail.com> > --- > Makefile | 6 ++++++ > arch/Kconfig | 24 +++++++++++++++++++++++- > arch/arm64/Kconfig | 1 + > include/linux/cfi_types.h | 15 +++++++++++---- > include/linux/compiler-gcc.h | 4 ++++ > kernel/Makefile | 1 + > kernel/cfi.c | 23 +++++++++++++++++++++++ > scripts/kallsyms.c | 4 +++- > 8 files changed, 72 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index 43e08c9f95e9..7c74dac57aa4 100644 > --- a/Makefile > +++ b/Makefile > @@ -926,6 +926,12 @@ KBUILD_CFLAGS += $(CC_FLAGS_CFI) > export CC_FLAGS_CFI > endif > > +ifdef CONFIG_CFI_GCC > +CC_FLAGS_CFI := -fsanitize=cfi > +KBUILD_CFLAGS += $(CC_FLAGS_CFI) > +export CC_FLAGS_CFI > +endif > + > ifdef CONFIG_DEBUG_FORCE_FUNCTION_ALIGN_64B > KBUILD_CFLAGS += -falign-functions=64 > endif > diff --git a/arch/Kconfig b/arch/Kconfig > index 1c1eca0c0019..8b43a9fd3b54 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -756,9 +756,31 @@ config CFI_CLANG > > https://clang.llvm.org/docs/ControlFlowIntegrity.html > > +config ARCH_SUPPORTS_CFI_GCC > + bool > + help > + An architecture should select this option if it can support GCC's > + Control-Flow Integrity (CFI) checking. > + > +config CFI_GCC > + bool "Use GCC's Control Flow Integrity (CFI)" > + depends on ARCH_SUPPORTS_CFI_GCC > + depends on $(cc-option,-fsanitize=cfi) > + help > + This option enables GCC’s forward-edge Control Flow Integrity > + (CFI) checking, where the compiler injects a runtime check to each > + indirect function call to ensure the target is a valid function with > + the correct static type. This restricts possible call targets and > + makes it more difficult for an attacker to exploit bugs that allow > + the modification of stored function pointers. More information can be > + found from the compiler's documentation: > + > + - Clang: https://clang.llvm.org/docs/ControlFlowIntegrity.html > + - GCC: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#Instrumentation-Options > + > config CFI_PERMISSIVE > bool "Use CFI in permissive mode" > - depends on CFI_CLANG > + depends on CFI_CLANG || CFI_GCC > help > When selected, Control Flow Integrity (CFI) violations result in a > warning instead of a kernel panic. This option should only be used > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 9fb9fff08c94..60fdfb01cecb 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -89,6 +89,7 @@ config ARM64 > select ARCH_SUPPORTS_LTO_CLANG if CPU_LITTLE_ENDIAN > select ARCH_SUPPORTS_LTO_CLANG_THIN > select ARCH_SUPPORTS_CFI_CLANG > + select ARCH_SUPPORTS_CFI_GCC > select ARCH_SUPPORTS_ATOMIC_RMW > select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 > select ARCH_SUPPORTS_NUMA_BALANCING > diff --git a/include/linux/cfi_types.h b/include/linux/cfi_types.h > index 6b8713675765..1c3b7ea6a79f 100644 > --- a/include/linux/cfi_types.h > +++ b/include/linux/cfi_types.h > @@ -8,18 +8,25 @@ > #ifdef __ASSEMBLY__ > #include <linux/linkage.h> > > -#ifdef CONFIG_CFI_CLANG > +#if defined(CONFIG_CFI_CLANG) || defined(CONFIG_CFI_GCC) > /* > - * Use the __kcfi_typeid_<function> type identifier symbol to > + * Use the __[k]cfi_typeid_<function> 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 > + > +#ifdef CONFIG_CFI_GCC > +#define __CFI_TYPE(name) \ > + .4byte __cfi_typeid_##name > +#else > #define __CFI_TYPE(name) \ > .4byte __kcfi_typeid_##name > #endif > > +#endif > + > #define SYM_TYPED_ENTRY(name, linkage, align...) \ > linkage(name) ASM_NL \ > align ASM_NL \ > @@ -29,12 +36,12 @@ > #define SYM_TYPED_START(name, linkage, align...) \ > SYM_TYPED_ENTRY(name, linkage, align) > > -#else /* CONFIG_CFI_CLANG */ > +#else /* defined(CONFIG_CFI_CLANG) || defined(CONFIG_CFI_GCC) */ > > #define SYM_TYPED_START(name, linkage, align...) \ > SYM_START(name, linkage, align) > > -#endif /* CONFIG_CFI_CLANG */ > +#endif /* defined(CONFIG_CFI_CLANG) || defined(CONFIG_CFI_GCC) */ > > #ifndef SYM_TYPED_FUNC_START > #define SYM_TYPED_FUNC_START(name) \ > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index 9b157b71036f..aec1ce327b1a 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -82,6 +82,10 @@ > #define __noscs __attribute__((__no_sanitize__("shadow-call-stack"))) > #endif > > +#ifdef CONFIG_CFI_GCC > +#define __nocfi __attribute__((no_sanitize("cfi"))) > +#endif > + > #if __has_attribute(__no_sanitize_address__) > #define __no_sanitize_address __attribute__((no_sanitize_address)) > #else > diff --git a/kernel/Makefile b/kernel/Makefile > index 318789c728d3..923d3e060852 100644 > --- a/kernel/Makefile > +++ b/kernel/Makefile > @@ -114,6 +114,7 @@ obj-$(CONFIG_SHADOW_CALL_STACK) += scs.o > obj-$(CONFIG_HAVE_STATIC_CALL) += static_call.o > obj-$(CONFIG_HAVE_STATIC_CALL_INLINE) += static_call_inline.o > obj-$(CONFIG_CFI_CLANG) += cfi.o > +obj-$(CONFIG_CFI_GCC) += cfi.o > > obj-$(CONFIG_PERF_EVENTS) += events/ > > diff --git a/kernel/cfi.c b/kernel/cfi.c > index 08caad776717..9bff35736756 100644 > --- a/kernel/cfi.c > +++ b/kernel/cfi.c > @@ -25,6 +25,7 @@ enum bug_trap_type report_cfi_failure(struct pt_regs *regs, unsigned long addr, > return BUG_TRAP_TYPE_BUG; > } > > +#ifdef CONFIG_CFI_CLANG > #ifdef CONFIG_ARCH_USES_CFI_TRAPS > static inline unsigned long trap_address(s32 *p) > { > @@ -99,3 +100,25 @@ bool is_cfi_trap(unsigned long addr) > return is_module_cfi_trap(addr); > } > #endif /* CONFIG_ARCH_USES_CFI_TRAPS */ > +#endif /* CONFIG_CFI_CLANG */ > + > + > +#ifdef CONFIG_CFI_GCC > +void cfi_check_failed(u32 caller_hash, u32 callee_hash, void *callee_addr) > +{ > + unsigned long pc, target; > + > + pc = (unsigned long)__builtin_return_address(0); > + target = (unsigned long)callee_addr; > + > + switch (report_cfi_failure(NULL, pc, &target, caller_hash)) { > + case BUG_TRAP_TYPE_WARN: > + break; > + > + default: > + panic("Oops - CFI"); > + } > +} > +EXPORT_SYMBOL(cfi_check_failed); > + > +#endif /* CONFIG_CFI_GCC */ > diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c > index ccdf0c897f31..ed8db513b918 100644 > --- a/scripts/kallsyms.c > +++ b/scripts/kallsyms.c > @@ -119,7 +119,9 @@ static bool is_ignored_symbol(const char *name, char type) > "__ThumbV7PILongThunk_", > "__LA25Thunk_", /* mips lld */ > "__microLA25Thunk_", > - "__kcfi_typeid_", /* CFI type identifiers */ > + "__kcfi_typeid_", /* CFI type identifiers in Clang */ > + "__cfi_", /* CFI type identifiers in GCC */ > + "__pi___cfi", /* CFI type identifiers in GCC */ > NULL > }; > > -- > 2.17.1 >
On Mon, Dec 19, 2022 at 04:04:55PM +0100, Peter Zijlstra wrote: > On Mon, Dec 19, 2022 at 05:32:04AM -0800, Dan Li wrote: > > Hi Peter, > > > > On 12/19, Peter Zijlstra wrote: > > > On Sun, Dec 18, 2022 at 10:17:58PM -0800, Dan Li wrote: > > > > > > > 1. When a typeid mismatch is detected, the cfi_check_failed function > > > > will be called instead of the brk instruction. This function needs > > > > to be implemented by the compiler user. > > > > If there are user mode programs or other systems that want to use > > > > this feature, it may be more convenient to use a callback (so this > > > > compilation option is set to -fsanitize=cfi instead of kcfi). > > > > > > This is not going to be acceptible for x86_64. > > > > I'm not familiar enough with the x86_64 platform, could you please > > tell me why this is not acceptable? Is there a similar situation > > on the arm64 platform? > > Mostly because the call would be a 5 byte instruction while the trap > (UD2) is only 2 bytes. > > I suspect Argh64 has a similar problem if the to be called function is > outside the immediate range (26 bits or thereabout), in which case you > end up with a multi-instruction sequence to construct the call target or > so. Either that or a direct branc to a PLT. > A trap is always a single instruction. Indeed. I strongly prefer the BRK for the reasons I've given in my other reply, which include code size. Thanks, Mark.
On Mon, Dec 19, 2022 at 05:32:04AM -0800, Dan Li wrote: > Hi Peter, > > On 12/19, Peter Zijlstra wrote: > > On Sun, Dec 18, 2022 at 10:17:58PM -0800, Dan Li wrote: > > > > > 1. When a typeid mismatch is detected, the cfi_check_failed function > > > will be called instead of the brk instruction. This function needs > > > to be implemented by the compiler user. > > > If there are user mode programs or other systems that want to use > > > this feature, it may be more convenient to use a callback (so this > > > compilation option is set to -fsanitize=cfi instead of kcfi). > > > > This is not going to be acceptible for x86_64. > > I'm not familiar enough with the x86_64 platform, could you please > tell me why this is not acceptable? Is there a similar situation > on the arm64 platform? > > > > 5. The current implementation of gcc only supports the aarch64 platform. > > > > What, if any, are the plans for x86_64 support? > > I'd like to implement something similar on x86_64 later, but > currently I'm doing this in my spare time, so it might take a > little longer. :( Hi! First of all, thank you thank you for working on this in GCC. This will make a big difference for folks that don't have the option to build with Clang to gain CFI coverage. As for the implementation details, the core issue is really that this type of CFI is specifically designed for the Linux kernel, and it took a rather long time to figure out all the specifics needed (down to the byte counts and instruction layouts). GCC's version will ultimately need to exactly match the Clang output, or Linux is unlikely to support it. We're already on our second CFI -- the original Clang CFI was just too clunky for long-term use in Linux, so unless we're going to improve on the latest Clang KCFI implementation in some way, it's better to stick to exactly byte-for-byte identical results. The KCFI support in Linux depends on the arm64 and x86_64 runtimes for catching the traps, and the post-processing done (on x86_64) with objtool that prepares the kernel for IBT use, and converts to the optional FineIBT CFI mechanism. With all those moving parts, there needs to be a very compelling reason to have GCC KCFI implementation differ from Clang's. Hopefully that context helps a little. I'm excited to try out future versions! -Kees
Hi Mark, Sorry for the late reply. On 01/03, Mark Rutland wrote: > On Sun, Dec 18, 2022 at 10:17:58PM -0800, Dan Li wrote: > > If there are user mode programs or other systems that want to use > > this feature, it may be more convenient to use a callback (so this > > compilation option is set to -fsanitize=cfi instead of kcfi). > > I appreciate that may be nicer for userspace, but it would be far nicer for the > kernel if we could have a kcfi mode that behaves the same as LLVM, using a BRK. > That's going to be simpler for the kernel to deal with, and should result in > nicer code / smaller binary size (for the reasons given above). > > Can we please have an LLVM-compatible KCFI mode, and have the -fsanitize=cfi be > a separate option from -fsanitize=kcfi? Ok, in the next version I will change to the same option as clang :) > > > 2. A reserved typeid (such as 0x0U on the aarch64 platform) is always > > inserted in front of functions that should not be called indirectly. > > Functions that can be called indirectly will not use this hash value, > > which prevents instructions/data before the function from being used > > as a typeid by an attacker. > > That sounds sensible, though it meanse we'll need to go audit all the assembly > without type annotations. > > I presume that "functions that should not be called indirectly" only includes > those which are not directly visible outside the compilation unit AND whose > address is never taken / escaped from the compilation unit. Is that the case? Yes. > > > 3. Some bits are ignored in the typeid to avoid conflicts between the > > typeid and the instruction set of a specific platform, thereby > > preventing an attacker from bypassing the CFI check by using the > > instruction as a typeid, such as on the aarch64 platform: > > * If the following instruction sequence exists: > > 400620: a9be7bfd stp x29, x30, [sp, #-32]! > > 400624: 910003fd mov x29, sp > > 400628: f9000bf3 str x19, [sp, #16] > > * If the expected typeid of the indirect call is exactly 0x910003fd, > > the attacker can jump to the next instruction position of any > > "mov x29,sp" instruction (such as 0x400628 here). > > Which bits exactly are ignored on arm64? > > e.g. are these encoded into UDF immediates? In aarch64, I currently ignore bit [28:27]. IUCC, according to the manual[1], it is a UDF instruction only when the upper 16 bits are all 0. But due to this has too much impact on the entropy of typeid, so I (not rigorously) only ignore 2 bits here, and most of the instruction codes covered by it belong to 'Reserved' or 'UNALLOCATED' (probably not a good idea). But as Kees said, if clang doesn't handle it here, in order to be consistent, I think it's better for gcc to not handle it when implementing kernel cfi. [1] https://developer.arm.com/documentation/ddi0602/2022-06/Index-by-Encoding?lang=en > > As a general thing, how does this work with -fpatchable-function-entry=M,N, > where N is non-zero? > > We still need to fix that for LLVM, and it would be good to align on the same behaviour. > Yeah, it makes sense. Currently, it is consistent with llvm. Taking -fpatchable-function-entry=2,1 as an example, the currently generated code is as follows: __cfi_main: .4byte 0x439d3502 .global main .section __patchable_function_entries .align 3 .8byte .LPFE3 .text .LPFE3: nop .type main, %function main: nop .LFB2: .cfi_startproc stp x29, x30, [sp, -32]! Finally, do we want to generate code like this? nop .4byte 0x439d3502 main: nop ... Thanks, Dan. > > > > -- > > 2.17.1 > >
Hi Kees, On 01/06, Kees Cook wrote: > On Mon, Dec 19, 2022 at 05:32:04AM -0800, Dan Li wrote: > > Hi Peter, > Hi! > > First of all, thank you thank you for working on this in GCC. This will > make a big difference for folks that don't have the option to build with > Clang to gain CFI coverage. > > As for the implementation details, the core issue is really that this > type of CFI is specifically designed for the Linux kernel, and it took a > rather long time to figure out all the specifics needed (down to the > byte counts and instruction layouts). GCC's version will ultimately need > to exactly match the Clang output, or Linux is unlikely to support it. > > We're already on our second CFI -- the original Clang CFI was just too > clunky for long-term use in Linux, so unless we're going to improve on > the latest Clang KCFI implementation in some way, it's better to stick > to exactly byte-for-byte identical results. The KCFI support in Linux > depends on the arm64 and x86_64 runtimes for catching the traps, and the > post-processing done (on x86_64) with objtool that prepares the kernel > for IBT use, and converts to the optional FineIBT CFI mechanism. With > all those moving parts, there needs to be a very compelling reason to > have GCC KCFI implementation differ from Clang's. > > Hopefully that context helps a little. I'm excited to try out future > versions! Thanks for the context, it makes sense and helped me a lot. :) In the next version I'll make the gcc implementation consistent with clang. Thanks, Dan. > > -Kees > > -- > Kees Cook
On Sat, Jan 07, 2023 at 07:42:13AM -0800, Dan Li wrote: > Hi Kees, > > On 01/06, Kees Cook wrote: > > On Mon, Dec 19, 2022 at 05:32:04AM -0800, Dan Li wrote: > > > Hi Peter, > > Hi! > > > > First of all, thank you thank you for working on this in GCC. This will > > make a big difference for folks that don't have the option to build with > > Clang to gain CFI coverage. > > > > As for the implementation details, the core issue is really that this > > type of CFI is specifically designed for the Linux kernel, and it took a > > rather long time to figure out all the specifics needed (down to the > > byte counts and instruction layouts). GCC's version will ultimately need > > to exactly match the Clang output, or Linux is unlikely to support it. > > > > We're already on our second CFI -- the original Clang CFI was just too > > clunky for long-term use in Linux, so unless we're going to improve on > > the latest Clang KCFI implementation in some way, it's better to stick > > to exactly byte-for-byte identical results. The KCFI support in Linux > > depends on the arm64 and x86_64 runtimes for catching the traps, and the > > post-processing done (on x86_64) with objtool that prepares the kernel > > for IBT use, and converts to the optional FineIBT CFI mechanism. With > > all those moving parts, there needs to be a very compelling reason to > > have GCC KCFI implementation differ from Clang's. > > > > Hopefully that context helps a little. I'm excited to try out future > > versions! > > Thanks for the context, it makes sense and helped me a lot. :) > > In the next version I'll make the gcc implementation consistent with clang. Hi! Just checking in on this, since there are a lot of interested folks. :) What's the status on the next version (and has anyone been found to tackle the x86 backend part)? Is there anything we can help with? Thanks! -Kees
On 02/08, Kees Cook wrote: > On Sat, Jan 07, 2023 at 07:42:13AM -0800, Dan Li wrote: > > Hi Kees, > > > > In the next version I'll make the gcc implementation consistent with clang. > > Hi! > > Just checking in on this, since there are a lot of interested folks. :) > What's the status on the next version (and has anyone been found to > tackle the x86 backend part)? Is there anything we can help with? > Hi Kees, Sorry for the long delay. It's been hard for me to find time to finish this until the end of February due to job changes (even weekends are busy :( ). As much as I'd love to get it done as soon as possible, I'm sorry I probably won't be able to submit the next version until March (maybe mid). If anyone could help improve this, I'd be very grateful, otherwise I'll have the next version done by the end of March (as a promise), and also try it in x86 platform. Sincerely apologize! Dan. > Thanks! > > -Kees > > -- > Kees Cook
Hi all, I am happy to introduce to you that I have contacted a colleague Likun Wang, who is willing to continue completing this patch. This patch has been delayed for a long time. I hope that gcc will support this feature in the near future. Thanks. Dan. On Mon, 19 Dec 2022 at 14:18, Dan Li <ashimida.1990@gmail.com> wrote: > > Based on Sami's patch[1], this patch makes the corresponding kernel > configuration of CFI available when compiling the kernel with the gcc[2]. > > The code after enabling cfi is as follows: > > int (*p)(void); > int func (int) > { > p(); > } > > __cfi_func: > .4byte 0x439d3502 > func: > ...... > adrp x0, p > add x0, x0, :lo12:p > mov w1, 23592 > movk w1, 0x4601, lsl 16 > cmp w0, w1 > beq .L2 > ...... > bl cfi_check_failed > .L2: > blr x19 > ret > > In the compiler part[4], there are some differences from Sami's > implementation[3], mainly including: > > 1. When a typeid mismatch is detected, the cfi_check_failed function > will be called instead of the brk instruction. This function needs > to be implemented by the compiler user. > If there are user mode programs or other systems that want to use > this feature, it may be more convenient to use a callback (so this > compilation option is set to -fsanitize=cfi instead of kcfi). > > 2. A reserved typeid (such as 0x0U on the aarch64 platform) is always > inserted in front of functions that should not be called indirectly. > Functions that can be called indirectly will not use this hash value, > which prevents instructions/data before the function from being used > as a typeid by an attacker. > > 3. Some bits are ignored in the typeid to avoid conflicts between the > typeid and the instruction set of a specific platform, thereby > preventing an attacker from bypassing the CFI check by using the > instruction as a typeid, such as on the aarch64 platform: > * If the following instruction sequence exists: > 400620: a9be7bfd stp x29, x30, [sp, #-32]! > 400624: 910003fd mov x29, sp > 400628: f9000bf3 str x19, [sp, #16] > * If the expected typeid of the indirect call is exactly 0x910003fd, > the attacker can jump to the next instruction position of any > "mov x29,sp" instruction (such as 0x400628 here). > > 4. Insert a symbol __cfi_<function> before each function's typeid, > which may be helpful for fine-grained KASLR implementations (or not?). > > 5. The current implementation of gcc only supports the aarch64 platform. > > This produces the following oops on CFI failure (generated using lkdtm): > > /kselftest_install/lkdtm # ./CFI_FORWARD_PROTO.sh > [ 74.856516] lkdtm: Performing direct entry CFI_FORWARD_PROTO > [ 74.856878] lkdtm: Calling matched prototype ... > [ 74.857011] lkdtm: Calling mismatched prototype ... > [ 74.857133] CFI failure at lkdtm_indirect_call+0x30/0x50 (target: lkdtm_increment_int+0x0/0x1c; expected type: 0xc59c68f1) > [ 74.858185] Kernel panic - not syncing: Oops - CFI > [ 74.859240] CPU: 0 PID: 129 Comm: cat Not tainted 6.0.0-rc4-00024-g32bf7f14f497-dirty #150 > [ 74.859481] Hardware name: linux,dummy-virt (DT) > [ 74.859795] Call trace: > [ 74.859959] dump_backtrace.part.0+0xcc/0xe0 > [ 74.860212] show_stack+0x18/0x5c > [ 74.860327] dump_stack_lvl+0x64/0x84 > [ 74.860398] dump_stack+0x18/0x38 > [ 74.860443] panic+0x170/0x36c > [ 74.860496] cfi_check_failed+0x38/0x44 > [ 74.860564] lkdtm_indirect_call+0x30/0x50 > [ 74.860614] lkdtm_CFI_FORWARD_PROTO+0x3c/0x6c > [ 74.860701] lkdtm_do_action+0x44/0x58 > [ 74.860764] direct_entry+0x148/0x160 > [ 74.860814] full_proxy_write+0x74/0xe0 > [ 74.860874] vfs_write+0xd8/0x2d0 > [ 74.860942] ksys_write+0x70/0x110 > [ 74.861000] __arm64_sys_write+0x1c/0x30 > [ 74.861067] invoke_syscall+0x5c/0x140 > [ 74.861117] el0_svc_common.constprop.0+0x44/0xf0 > [ 74.861190] do_el0_svc+0x2c/0xc0 > [ 74.861233] el0_svc+0x20/0x60 > [ 74.861287] el0t_64_sync_handler+0xf4/0x124 > [ 74.861340] el0t_64_sync+0x160/0x164 > [ 74.861782] SMP: stopping secondary CPUs > [ 74.862336] Kernel Offset: disabled > [ 74.862439] CPU features: 0x0000,00075024,699418af > [ 74.862799] Memory Limit: none > [ 74.863373] ---[ end Kernel panic - not syncing: Oops - CFI ]--- > > The gcc-related patches[4] are based on tag: releases/gcc-12.2.0. > > Any suggestion please let me know :). > > Thanks, Dan. > > [1] https://lore.kernel.org/all/20220908215504.3686827-1-samitolvanen@google.com/ > [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107048 > [3] https://reviews.llvm.org/D119296 > [4] https://lore.kernel.org/linux-hardening/20221219055431.22596-1-ashimida.1990@gmail.com/ > > Signed-off-by: Dan Li <ashimida.1990@gmail.com> > --- > Makefile | 6 ++++++ > arch/Kconfig | 24 +++++++++++++++++++++++- > arch/arm64/Kconfig | 1 + > include/linux/cfi_types.h | 15 +++++++++++---- > include/linux/compiler-gcc.h | 4 ++++ > kernel/Makefile | 1 + > kernel/cfi.c | 23 +++++++++++++++++++++++ > scripts/kallsyms.c | 4 +++- > 8 files changed, 72 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index 43e08c9f95e9..7c74dac57aa4 100644 > --- a/Makefile > +++ b/Makefile > @@ -926,6 +926,12 @@ KBUILD_CFLAGS += $(CC_FLAGS_CFI) > export CC_FLAGS_CFI > endif > > +ifdef CONFIG_CFI_GCC > +CC_FLAGS_CFI := -fsanitize=cfi > +KBUILD_CFLAGS += $(CC_FLAGS_CFI) > +export CC_FLAGS_CFI > +endif > + > ifdef CONFIG_DEBUG_FORCE_FUNCTION_ALIGN_64B > KBUILD_CFLAGS += -falign-functions=64 > endif > diff --git a/arch/Kconfig b/arch/Kconfig > index 1c1eca0c0019..8b43a9fd3b54 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -756,9 +756,31 @@ config CFI_CLANG > > https://clang.llvm.org/docs/ControlFlowIntegrity.html > > +config ARCH_SUPPORTS_CFI_GCC > + bool > + help > + An architecture should select this option if it can support GCC's > + Control-Flow Integrity (CFI) checking. > + > +config CFI_GCC > + bool "Use GCC's Control Flow Integrity (CFI)" > + depends on ARCH_SUPPORTS_CFI_GCC > + depends on $(cc-option,-fsanitize=cfi) > + help > + This option enables GCC’s forward-edge Control Flow Integrity > + (CFI) checking, where the compiler injects a runtime check to each > + indirect function call to ensure the target is a valid function with > + the correct static type. This restricts possible call targets and > + makes it more difficult for an attacker to exploit bugs that allow > + the modification of stored function pointers. More information can be > + found from the compiler's documentation: > + > + - Clang: https://clang.llvm.org/docs/ControlFlowIntegrity.html > + - GCC: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#Instrumentation-Options > + > config CFI_PERMISSIVE > bool "Use CFI in permissive mode" > - depends on CFI_CLANG > + depends on CFI_CLANG || CFI_GCC > help > When selected, Control Flow Integrity (CFI) violations result in a > warning instead of a kernel panic. This option should only be used > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 9fb9fff08c94..60fdfb01cecb 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -89,6 +89,7 @@ config ARM64 > select ARCH_SUPPORTS_LTO_CLANG if CPU_LITTLE_ENDIAN > select ARCH_SUPPORTS_LTO_CLANG_THIN > select ARCH_SUPPORTS_CFI_CLANG > + select ARCH_SUPPORTS_CFI_GCC > select ARCH_SUPPORTS_ATOMIC_RMW > select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 > select ARCH_SUPPORTS_NUMA_BALANCING > diff --git a/include/linux/cfi_types.h b/include/linux/cfi_types.h > index 6b8713675765..1c3b7ea6a79f 100644 > --- a/include/linux/cfi_types.h > +++ b/include/linux/cfi_types.h > @@ -8,18 +8,25 @@ > #ifdef __ASSEMBLY__ > #include <linux/linkage.h> > > -#ifdef CONFIG_CFI_CLANG > +#if defined(CONFIG_CFI_CLANG) || defined(CONFIG_CFI_GCC) > /* > - * Use the __kcfi_typeid_<function> type identifier symbol to > + * Use the __[k]cfi_typeid_<function> 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 > + > +#ifdef CONFIG_CFI_GCC > +#define __CFI_TYPE(name) \ > + .4byte __cfi_typeid_##name > +#else > #define __CFI_TYPE(name) \ > .4byte __kcfi_typeid_##name > #endif > > +#endif > + > #define SYM_TYPED_ENTRY(name, linkage, align...) \ > linkage(name) ASM_NL \ > align ASM_NL \ > @@ -29,12 +36,12 @@ > #define SYM_TYPED_START(name, linkage, align...) \ > SYM_TYPED_ENTRY(name, linkage, align) > > -#else /* CONFIG_CFI_CLANG */ > +#else /* defined(CONFIG_CFI_CLANG) || defined(CONFIG_CFI_GCC) */ > > #define SYM_TYPED_START(name, linkage, align...) \ > SYM_START(name, linkage, align) > > -#endif /* CONFIG_CFI_CLANG */ > +#endif /* defined(CONFIG_CFI_CLANG) || defined(CONFIG_CFI_GCC) */ > > #ifndef SYM_TYPED_FUNC_START > #define SYM_TYPED_FUNC_START(name) \ > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index 9b157b71036f..aec1ce327b1a 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -82,6 +82,10 @@ > #define __noscs __attribute__((__no_sanitize__("shadow-call-stack"))) > #endif > > +#ifdef CONFIG_CFI_GCC > +#define __nocfi __attribute__((no_sanitize("cfi"))) > +#endif > + > #if __has_attribute(__no_sanitize_address__) > #define __no_sanitize_address __attribute__((no_sanitize_address)) > #else > diff --git a/kernel/Makefile b/kernel/Makefile > index 318789c728d3..923d3e060852 100644 > --- a/kernel/Makefile > +++ b/kernel/Makefile > @@ -114,6 +114,7 @@ obj-$(CONFIG_SHADOW_CALL_STACK) += scs.o > obj-$(CONFIG_HAVE_STATIC_CALL) += static_call.o > obj-$(CONFIG_HAVE_STATIC_CALL_INLINE) += static_call_inline.o > obj-$(CONFIG_CFI_CLANG) += cfi.o > +obj-$(CONFIG_CFI_GCC) += cfi.o > > obj-$(CONFIG_PERF_EVENTS) += events/ > > diff --git a/kernel/cfi.c b/kernel/cfi.c > index 08caad776717..9bff35736756 100644 > --- a/kernel/cfi.c > +++ b/kernel/cfi.c > @@ -25,6 +25,7 @@ enum bug_trap_type report_cfi_failure(struct pt_regs *regs, unsigned long addr, > return BUG_TRAP_TYPE_BUG; > } > > +#ifdef CONFIG_CFI_CLANG > #ifdef CONFIG_ARCH_USES_CFI_TRAPS > static inline unsigned long trap_address(s32 *p) > { > @@ -99,3 +100,25 @@ bool is_cfi_trap(unsigned long addr) > return is_module_cfi_trap(addr); > } > #endif /* CONFIG_ARCH_USES_CFI_TRAPS */ > +#endif /* CONFIG_CFI_CLANG */ > + > + > +#ifdef CONFIG_CFI_GCC > +void cfi_check_failed(u32 caller_hash, u32 callee_hash, void *callee_addr) > +{ > + unsigned long pc, target; > + > + pc = (unsigned long)__builtin_return_address(0); > + target = (unsigned long)callee_addr; > + > + switch (report_cfi_failure(NULL, pc, &target, caller_hash)) { > + case BUG_TRAP_TYPE_WARN: > + break; > + > + default: > + panic("Oops - CFI"); > + } > +} > +EXPORT_SYMBOL(cfi_check_failed); > + > +#endif /* CONFIG_CFI_GCC */ > diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c > index ccdf0c897f31..ed8db513b918 100644 > --- a/scripts/kallsyms.c > +++ b/scripts/kallsyms.c > @@ -119,7 +119,9 @@ static bool is_ignored_symbol(const char *name, char type) > "__ThumbV7PILongThunk_", > "__LA25Thunk_", /* mips lld */ > "__microLA25Thunk_", > - "__kcfi_typeid_", /* CFI type identifiers */ > + "__kcfi_typeid_", /* CFI type identifiers in Clang */ > + "__cfi_", /* CFI type identifiers in GCC */ > + "__pi___cfi", /* CFI type identifiers in GCC */ > NULL > }; > > -- > 2.17.1 >
diff --git a/Makefile b/Makefile index 43e08c9f95e9..7c74dac57aa4 100644 --- a/Makefile +++ b/Makefile @@ -926,6 +926,12 @@ KBUILD_CFLAGS += $(CC_FLAGS_CFI) export CC_FLAGS_CFI endif +ifdef CONFIG_CFI_GCC +CC_FLAGS_CFI := -fsanitize=cfi +KBUILD_CFLAGS += $(CC_FLAGS_CFI) +export CC_FLAGS_CFI +endif + ifdef CONFIG_DEBUG_FORCE_FUNCTION_ALIGN_64B KBUILD_CFLAGS += -falign-functions=64 endif diff --git a/arch/Kconfig b/arch/Kconfig index 1c1eca0c0019..8b43a9fd3b54 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -756,9 +756,31 @@ config CFI_CLANG https://clang.llvm.org/docs/ControlFlowIntegrity.html +config ARCH_SUPPORTS_CFI_GCC + bool + help + An architecture should select this option if it can support GCC's + Control-Flow Integrity (CFI) checking. + +config CFI_GCC + bool "Use GCC's Control Flow Integrity (CFI)" + depends on ARCH_SUPPORTS_CFI_GCC + depends on $(cc-option,-fsanitize=cfi) + help + This option enables GCC’s forward-edge Control Flow Integrity + (CFI) checking, where the compiler injects a runtime check to each + indirect function call to ensure the target is a valid function with + the correct static type. This restricts possible call targets and + makes it more difficult for an attacker to exploit bugs that allow + the modification of stored function pointers. More information can be + found from the compiler's documentation: + + - Clang: https://clang.llvm.org/docs/ControlFlowIntegrity.html + - GCC: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#Instrumentation-Options + config CFI_PERMISSIVE bool "Use CFI in permissive mode" - depends on CFI_CLANG + depends on CFI_CLANG || CFI_GCC help When selected, Control Flow Integrity (CFI) violations result in a warning instead of a kernel panic. This option should only be used diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 9fb9fff08c94..60fdfb01cecb 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -89,6 +89,7 @@ config ARM64 select ARCH_SUPPORTS_LTO_CLANG if CPU_LITTLE_ENDIAN select ARCH_SUPPORTS_LTO_CLANG_THIN select ARCH_SUPPORTS_CFI_CLANG + select ARCH_SUPPORTS_CFI_GCC select ARCH_SUPPORTS_ATOMIC_RMW select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 select ARCH_SUPPORTS_NUMA_BALANCING diff --git a/include/linux/cfi_types.h b/include/linux/cfi_types.h index 6b8713675765..1c3b7ea6a79f 100644 --- a/include/linux/cfi_types.h +++ b/include/linux/cfi_types.h @@ -8,18 +8,25 @@ #ifdef __ASSEMBLY__ #include <linux/linkage.h> -#ifdef CONFIG_CFI_CLANG +#if defined(CONFIG_CFI_CLANG) || defined(CONFIG_CFI_GCC) /* - * Use the __kcfi_typeid_<function> type identifier symbol to + * Use the __[k]cfi_typeid_<function> 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 + +#ifdef CONFIG_CFI_GCC +#define __CFI_TYPE(name) \ + .4byte __cfi_typeid_##name +#else #define __CFI_TYPE(name) \ .4byte __kcfi_typeid_##name #endif +#endif + #define SYM_TYPED_ENTRY(name, linkage, align...) \ linkage(name) ASM_NL \ align ASM_NL \ @@ -29,12 +36,12 @@ #define SYM_TYPED_START(name, linkage, align...) \ SYM_TYPED_ENTRY(name, linkage, align) -#else /* CONFIG_CFI_CLANG */ +#else /* defined(CONFIG_CFI_CLANG) || defined(CONFIG_CFI_GCC) */ #define SYM_TYPED_START(name, linkage, align...) \ SYM_START(name, linkage, align) -#endif /* CONFIG_CFI_CLANG */ +#endif /* defined(CONFIG_CFI_CLANG) || defined(CONFIG_CFI_GCC) */ #ifndef SYM_TYPED_FUNC_START #define SYM_TYPED_FUNC_START(name) \ diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 9b157b71036f..aec1ce327b1a 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -82,6 +82,10 @@ #define __noscs __attribute__((__no_sanitize__("shadow-call-stack"))) #endif +#ifdef CONFIG_CFI_GCC +#define __nocfi __attribute__((no_sanitize("cfi"))) +#endif + #if __has_attribute(__no_sanitize_address__) #define __no_sanitize_address __attribute__((no_sanitize_address)) #else diff --git a/kernel/Makefile b/kernel/Makefile index 318789c728d3..923d3e060852 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -114,6 +114,7 @@ obj-$(CONFIG_SHADOW_CALL_STACK) += scs.o obj-$(CONFIG_HAVE_STATIC_CALL) += static_call.o obj-$(CONFIG_HAVE_STATIC_CALL_INLINE) += static_call_inline.o obj-$(CONFIG_CFI_CLANG) += cfi.o +obj-$(CONFIG_CFI_GCC) += cfi.o obj-$(CONFIG_PERF_EVENTS) += events/ diff --git a/kernel/cfi.c b/kernel/cfi.c index 08caad776717..9bff35736756 100644 --- a/kernel/cfi.c +++ b/kernel/cfi.c @@ -25,6 +25,7 @@ enum bug_trap_type report_cfi_failure(struct pt_regs *regs, unsigned long addr, return BUG_TRAP_TYPE_BUG; } +#ifdef CONFIG_CFI_CLANG #ifdef CONFIG_ARCH_USES_CFI_TRAPS static inline unsigned long trap_address(s32 *p) { @@ -99,3 +100,25 @@ bool is_cfi_trap(unsigned long addr) return is_module_cfi_trap(addr); } #endif /* CONFIG_ARCH_USES_CFI_TRAPS */ +#endif /* CONFIG_CFI_CLANG */ + + +#ifdef CONFIG_CFI_GCC +void cfi_check_failed(u32 caller_hash, u32 callee_hash, void *callee_addr) +{ + unsigned long pc, target; + + pc = (unsigned long)__builtin_return_address(0); + target = (unsigned long)callee_addr; + + switch (report_cfi_failure(NULL, pc, &target, caller_hash)) { + case BUG_TRAP_TYPE_WARN: + break; + + default: + panic("Oops - CFI"); + } +} +EXPORT_SYMBOL(cfi_check_failed); + +#endif /* CONFIG_CFI_GCC */ diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c index ccdf0c897f31..ed8db513b918 100644 --- a/scripts/kallsyms.c +++ b/scripts/kallsyms.c @@ -119,7 +119,9 @@ static bool is_ignored_symbol(const char *name, char type) "__ThumbV7PILongThunk_", "__LA25Thunk_", /* mips lld */ "__microLA25Thunk_", - "__kcfi_typeid_", /* CFI type identifiers */ + "__kcfi_typeid_", /* CFI type identifiers in Clang */ + "__cfi_", /* CFI type identifiers in GCC */ + "__pi___cfi", /* CFI type identifiers in GCC */ NULL };
Based on Sami's patch[1], this patch makes the corresponding kernel configuration of CFI available when compiling the kernel with the gcc[2]. The code after enabling cfi is as follows: int (*p)(void); int func (int) { p(); } __cfi_func: .4byte 0x439d3502 func: ...... adrp x0, p add x0, x0, :lo12:p mov w1, 23592 movk w1, 0x4601, lsl 16 cmp w0, w1 beq .L2 ...... bl cfi_check_failed .L2: blr x19 ret In the compiler part[4], there are some differences from Sami's implementation[3], mainly including: 1. When a typeid mismatch is detected, the cfi_check_failed function will be called instead of the brk instruction. This function needs to be implemented by the compiler user. If there are user mode programs or other systems that want to use this feature, it may be more convenient to use a callback (so this compilation option is set to -fsanitize=cfi instead of kcfi). 2. A reserved typeid (such as 0x0U on the aarch64 platform) is always inserted in front of functions that should not be called indirectly. Functions that can be called indirectly will not use this hash value, which prevents instructions/data before the function from being used as a typeid by an attacker. 3. Some bits are ignored in the typeid to avoid conflicts between the typeid and the instruction set of a specific platform, thereby preventing an attacker from bypassing the CFI check by using the instruction as a typeid, such as on the aarch64 platform: * If the following instruction sequence exists: 400620: a9be7bfd stp x29, x30, [sp, #-32]! 400624: 910003fd mov x29, sp 400628: f9000bf3 str x19, [sp, #16] * If the expected typeid of the indirect call is exactly 0x910003fd, the attacker can jump to the next instruction position of any "mov x29,sp" instruction (such as 0x400628 here). 4. Insert a symbol __cfi_<function> before each function's typeid, which may be helpful for fine-grained KASLR implementations (or not?). 5. The current implementation of gcc only supports the aarch64 platform. This produces the following oops on CFI failure (generated using lkdtm): /kselftest_install/lkdtm # ./CFI_FORWARD_PROTO.sh [ 74.856516] lkdtm: Performing direct entry CFI_FORWARD_PROTO [ 74.856878] lkdtm: Calling matched prototype ... [ 74.857011] lkdtm: Calling mismatched prototype ... [ 74.857133] CFI failure at lkdtm_indirect_call+0x30/0x50 (target: lkdtm_increment_int+0x0/0x1c; expected type: 0xc59c68f1) [ 74.858185] Kernel panic - not syncing: Oops - CFI [ 74.859240] CPU: 0 PID: 129 Comm: cat Not tainted 6.0.0-rc4-00024-g32bf7f14f497-dirty #150 [ 74.859481] Hardware name: linux,dummy-virt (DT) [ 74.859795] Call trace: [ 74.859959] dump_backtrace.part.0+0xcc/0xe0 [ 74.860212] show_stack+0x18/0x5c [ 74.860327] dump_stack_lvl+0x64/0x84 [ 74.860398] dump_stack+0x18/0x38 [ 74.860443] panic+0x170/0x36c [ 74.860496] cfi_check_failed+0x38/0x44 [ 74.860564] lkdtm_indirect_call+0x30/0x50 [ 74.860614] lkdtm_CFI_FORWARD_PROTO+0x3c/0x6c [ 74.860701] lkdtm_do_action+0x44/0x58 [ 74.860764] direct_entry+0x148/0x160 [ 74.860814] full_proxy_write+0x74/0xe0 [ 74.860874] vfs_write+0xd8/0x2d0 [ 74.860942] ksys_write+0x70/0x110 [ 74.861000] __arm64_sys_write+0x1c/0x30 [ 74.861067] invoke_syscall+0x5c/0x140 [ 74.861117] el0_svc_common.constprop.0+0x44/0xf0 [ 74.861190] do_el0_svc+0x2c/0xc0 [ 74.861233] el0_svc+0x20/0x60 [ 74.861287] el0t_64_sync_handler+0xf4/0x124 [ 74.861340] el0t_64_sync+0x160/0x164 [ 74.861782] SMP: stopping secondary CPUs [ 74.862336] Kernel Offset: disabled [ 74.862439] CPU features: 0x0000,00075024,699418af [ 74.862799] Memory Limit: none [ 74.863373] ---[ end Kernel panic - not syncing: Oops - CFI ]--- The gcc-related patches[4] are based on tag: releases/gcc-12.2.0. Any suggestion please let me know :). Thanks, Dan. [1] https://lore.kernel.org/all/20220908215504.3686827-1-samitolvanen@google.com/ [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107048 [3] https://reviews.llvm.org/D119296 [4] https://lore.kernel.org/linux-hardening/20221219055431.22596-1-ashimida.1990@gmail.com/ Signed-off-by: Dan Li <ashimida.1990@gmail.com> --- Makefile | 6 ++++++ arch/Kconfig | 24 +++++++++++++++++++++++- arch/arm64/Kconfig | 1 + include/linux/cfi_types.h | 15 +++++++++++---- include/linux/compiler-gcc.h | 4 ++++ kernel/Makefile | 1 + kernel/cfi.c | 23 +++++++++++++++++++++++ scripts/kallsyms.c | 4 +++- 8 files changed, 72 insertions(+), 6 deletions(-)