This series adds support for Clang's Control-Flow Integrity (CFI)
checking to x86_64. With CFI, the compiler injects a runtime
check before 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. For more details, see:
https://clang.llvm.org/docs/ControlFlowIntegrity.html
Version 2 depends on Clang >=14, where we fixed the issue with
referencing static functions from inline assembly. Based on the
feedback for v1, this version also changes the declaration of
functions that are not callable from C to use an opaque type,
which stops the compiler from replacing references to them. This
avoids the need to sprinkle function_nocfi() macros in the kernel
code.
The first two patches contain objtool support for CFI, the
remaining patches change function declarations to use opaque
types, fix type mismatch issues that confuse the compiler, and
disable CFI where it can't be used.
You can also pull this series from
https://github.com/samitolvanen/linux.git x86-cfi-v2
---
Changes in v2:
- Dropped the first objtool patch as the warnings were fixed in
separate patches.
- Changed fix_cfi_relocs() in objtool to not rely on jump table
symbols, and to return an error if it can't find a relocation.
- Fixed a build issue with ASM_STACK_FRAME_NON_STANDARD().
- Dropped workarounds for inline assembly references to
address-taken static functions with CFI as this was fixed in
the compiler.
- Changed the C declarations of non-callable functions to use
opaque types and dropped the function_nocfi() patches.
- Changed ARCH_SUPPORTS_CFI_CLANG to depend on Clang >=14 for
the compiler fixes.
Kees Cook (2):
x86/extable: Do not mark exception callback as CFI
x86, relocs: Ignore __typeid__ relocations
Sami Tolvanen (12):
objtool: Add CONFIG_CFI_CLANG support
objtool: Add ASM_STACK_FRAME_NON_STANDARD
linkage: Add DECLARE_ASM_FUNC_SYMBOL
ftrace: Use an opaque type for functions not callable from C
lkdtm: Disable UNSET_SMEP with CFI
lkdtm: Use an opaque type for lkdtm_rodata_do_nothing
x86: Use an opaque type for functions not callable from C
x86/purgatory: Disable CFI
x86, module: Ignore __typeid__ relocations
x86, cpu: Use LTO for cpu.c with CFI
x86, kprobes: Fix optprobe_template_func type mismatch
x86, build: Allow CONFIG_CFI_CLANG to be selected
arch/x86/Kconfig | 1 +
arch/x86/include/asm/ftrace.h | 2 +-
arch/x86/include/asm/idtentry.h | 10 +++---
arch/x86/include/asm/page_64.h | 7 ++--
arch/x86/include/asm/paravirt_types.h | 3 +-
arch/x86/include/asm/processor.h | 2 +-
arch/x86/include/asm/proto.h | 25 ++++++-------
arch/x86/include/asm/uaccess_64.h | 9 ++---
arch/x86/kernel/alternative.c | 2 +-
arch/x86/kernel/ftrace.c | 2 +-
arch/x86/kernel/kprobes/opt.c | 4 +--
arch/x86/kernel/module.c | 4 +++
arch/x86/kernel/paravirt.c | 4 +--
arch/x86/kvm/emulate.c | 4 +--
arch/x86/kvm/kvm_emulate.h | 9 ++---
arch/x86/mm/extable.c | 1 +
arch/x86/power/Makefile | 2 ++
arch/x86/purgatory/Makefile | 2 +-
arch/x86/tools/relocs.c | 7 ++++
arch/x86/xen/enlighten_pv.c | 6 ++--
arch/x86/xen/xen-ops.h | 10 +++---
drivers/misc/lkdtm/bugs.c | 2 +-
drivers/misc/lkdtm/lkdtm.h | 2 +-
drivers/misc/lkdtm/perms.c | 2 +-
drivers/misc/lkdtm/rodata.c | 2 +-
include/linux/ftrace.h | 7 ++--
include/linux/linkage.h | 13 +++++++
include/linux/objtool.h | 6 ++++
tools/include/linux/objtool.h | 6 ++++
tools/objtool/arch/x86/decode.c | 16 +++++++++
tools/objtool/elf.c | 51 +++++++++++++++++++++++++++
tools/objtool/include/objtool/arch.h | 3 ++
tools/objtool/include/objtool/elf.h | 2 +-
33 files changed, 166 insertions(+), 62 deletions(-)
base-commit: d5ae8d7f85b7f6f6e60f1af8ff4be52b0926fde1
--
2.33.0.rc2.250.ged5fa647cd-goog
Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro.
Signed-off-by: Sami Tolvanen <[email protected]>
---
arch/x86/purgatory/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index 95ea17a9d20c..ed46ad780130 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -31,7 +31,7 @@ KCOV_INSTRUMENT := n
# These are adjustments to the compiler flags used for objects that
# make up the standalone purgatory.ro
-PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
+PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel $(CC_FLAGS_CFI)
PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss -g0
PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN) -DDISABLE_BRANCH_PROFILING
PURGATORY_CFLAGS += -fno-stack-protector
--
2.33.0.rc2.250.ged5fa647cd-goog
From: Kees Cook <[email protected]>
The __typeid__* symbols aren't actually relocations, so they can be
ignored during relocation generation.
Signed-off-by: Kees Cook <[email protected]>
Signed-off-by: Sami Tolvanen <[email protected]>
---
arch/x86/tools/relocs.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c
index 9ba700dc47de..fbc57cc00914 100644
--- a/arch/x86/tools/relocs.c
+++ b/arch/x86/tools/relocs.c
@@ -48,6 +48,7 @@ static const char * const sym_regex_kernel[S_NSYMTYPES] = {
"^(xen_irq_disable_direct_reloc$|"
"xen_save_fl_direct_reloc$|"
"VDSO|"
+ "__typeid__|"
"__crc_)",
/*
@@ -808,6 +809,12 @@ static int do_reloc64(struct section *sec, Elf_Rel *rel, ElfW(Sym) *sym,
symname);
break;
+ case R_X86_64_8:
+ if (!shn_abs || !is_reloc(S_ABS, symname))
+ die("Non-whitelisted %s relocation: %s\n",
+ rel_type(r_type), symname);
+ break;
+
case R_X86_64_32:
case R_X86_64_32S:
case R_X86_64_64:
--
2.33.0.rc2.250.ged5fa647cd-goog
The kernel has several assembly functions that are not directly callable
from C. Use an opaque type for these function prototypes to make misuse
harder, and to avoid the need to annotate references to these functions
for Clang's Control-Flow Integrity (CFI).
Suggested-by: Andy Lutomirski <[email protected]>
Suggested-by: Alexander Lobakin <[email protected]>
Signed-off-by: Sami Tolvanen <[email protected]>
---
arch/x86/include/asm/ftrace.h | 2 +-
arch/x86/include/asm/idtentry.h | 10 +++++-----
arch/x86/include/asm/page_64.h | 7 ++++---
arch/x86/include/asm/paravirt_types.h | 3 ++-
arch/x86/include/asm/processor.h | 2 +-
arch/x86/include/asm/proto.h | 25 +++++++++++++------------
arch/x86/include/asm/uaccess_64.h | 9 +++------
arch/x86/kernel/alternative.c | 2 +-
arch/x86/kernel/ftrace.c | 2 +-
arch/x86/kernel/paravirt.c | 4 ++--
arch/x86/kvm/emulate.c | 4 ++--
arch/x86/kvm/kvm_emulate.h | 9 ++-------
arch/x86/xen/enlighten_pv.c | 6 +++---
arch/x86/xen/xen-ops.h | 10 +++++-----
14 files changed, 45 insertions(+), 50 deletions(-)
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index 9f3130f40807..54d23f421c16 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -17,7 +17,7 @@
#ifndef __ASSEMBLY__
extern atomic_t modifying_ftrace_code;
-extern void __fentry__(void);
+DECLARE_ASM_FUNC_SYMBOL(__fentry__);
static inline unsigned long ftrace_call_adjust(unsigned long addr)
{
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index 1345088e9902..2f6d0528bdd2 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -27,8 +27,8 @@
* as well which is used to emit the entry stubs in entry_32/64.S.
*/
#define DECLARE_IDTENTRY(vector, func) \
- asmlinkage void asm_##func(void); \
- asmlinkage void xen_asm_##func(void); \
+ DECLARE_ASM_FUNC_SYMBOL(asm_##func); \
+ DECLARE_ASM_FUNC_SYMBOL(xen_asm_##func); \
__visible void func(struct pt_regs *regs)
/**
@@ -78,8 +78,8 @@ static __always_inline void __##func(struct pt_regs *regs)
* C-handler.
*/
#define DECLARE_IDTENTRY_ERRORCODE(vector, func) \
- asmlinkage void asm_##func(void); \
- asmlinkage void xen_asm_##func(void); \
+ DECLARE_ASM_FUNC_SYMBOL(asm_##func); \
+ DECLARE_ASM_FUNC_SYMBOL(xen_asm_##func); \
__visible void func(struct pt_regs *regs, unsigned long error_code)
/**
@@ -386,7 +386,7 @@ static __always_inline void __##func(struct pt_regs *regs)
* - The C handler called from the C shim
*/
#define DECLARE_IDTENTRY_DF(vector, func) \
- asmlinkage void asm_##func(void); \
+ DECLARE_ASM_FUNC_SYMBOL(asm_##func); \
__visible void func(struct pt_regs *regs, \
unsigned long error_code, \
unsigned long address)
diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
index 4bde0dc66100..d6760b6773de 100644
--- a/arch/x86/include/asm/page_64.h
+++ b/arch/x86/include/asm/page_64.h
@@ -5,6 +5,7 @@
#include <asm/page_64_types.h>
#ifndef __ASSEMBLY__
+#include <linux/linkage.h>
#include <asm/alternative.h>
/* duplicated to the one in bootmem.h */
@@ -40,9 +41,9 @@ extern unsigned long __phys_addr_symbol(unsigned long);
#define pfn_valid(pfn) ((pfn) < max_pfn)
#endif
-void clear_page_orig(void *page);
-void clear_page_rep(void *page);
-void clear_page_erms(void *page);
+DECLARE_ASM_FUNC_SYMBOL(clear_page_orig);
+DECLARE_ASM_FUNC_SYMBOL(clear_page_rep);
+DECLARE_ASM_FUNC_SYMBOL(clear_page_erms);
static inline void clear_page(void *page)
{
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index d9d6b0203ec4..7f71d52c55f5 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -38,6 +38,7 @@
#include <asm/desc_defs.h>
#include <asm/pgtable_types.h>
#include <asm/nospec-branch.h>
+#include <asm/proto.h>
struct page;
struct thread_struct;
@@ -271,7 +272,7 @@ struct paravirt_patch_template {
extern struct pv_info pv_info;
extern struct paravirt_patch_template pv_ops;
-extern void (*paravirt_iret)(void);
+extern asm_func_t paravirt_iret;
#define PARAVIRT_PATCH(x) \
(offsetof(struct paravirt_patch_template, x) / sizeof(void *))
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index f3020c54e2cb..005f519c2648 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -447,7 +447,7 @@ static inline unsigned long cpu_kernelmode_gs_base(int cpu)
DECLARE_PER_CPU(void *, hardirq_stack_ptr);
DECLARE_PER_CPU(bool, hardirq_stack_inuse);
-extern asmlinkage void ignore_sysret(void);
+DECLARE_ASM_FUNC_SYMBOL(ignore_sysret);
/* Save actual FS/GS selectors and bases to current->thread */
void current_save_fsgs(void);
diff --git a/arch/x86/include/asm/proto.h b/arch/x86/include/asm/proto.h
index 8c5d1910a848..a6aa64eb3657 100644
--- a/arch/x86/include/asm/proto.h
+++ b/arch/x86/include/asm/proto.h
@@ -2,6 +2,7 @@
#ifndef _ASM_X86_PROTO_H
#define _ASM_X86_PROTO_H
+#include <linux/linkage.h>
#include <asm/ldt.h>
struct task_struct;
@@ -11,26 +12,26 @@ struct task_struct;
void syscall_init(void);
#ifdef CONFIG_X86_64
-void entry_SYSCALL_64(void);
-void entry_SYSCALL_64_safe_stack(void);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSCALL_64);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSCALL_64_safe_stack);
long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2);
#endif
#ifdef CONFIG_X86_32
-void entry_INT80_32(void);
-void entry_SYSENTER_32(void);
-void __begin_SYSENTER_singlestep_region(void);
-void __end_SYSENTER_singlestep_region(void);
+DECLARE_ASM_FUNC_SYMBOL(entry_INT80_32);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSENTER_32);
+DECLARE_ASM_FUNC_SYMBOL(__begin_SYSENTER_singlestep_region);
+DECLARE_ASM_FUNC_SYMBOL(__end_SYSENTER_singlestep_region);
#endif
#ifdef CONFIG_IA32_EMULATION
-void entry_SYSENTER_compat(void);
-void __end_entry_SYSENTER_compat(void);
-void entry_SYSCALL_compat(void);
-void entry_SYSCALL_compat_safe_stack(void);
-void entry_INT80_compat(void);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSENTER_compat);
+DECLARE_ASM_FUNC_SYMBOL(__end_entry_SYSENTER_compat);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSCALL_compat);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSCALL_compat_safe_stack);
+DECLARE_ASM_FUNC_SYMBOL(entry_INT80_compat);
#ifdef CONFIG_XEN_PV
-void xen_entry_INT80_compat(void);
+DECLARE_ASM_FUNC_SYMBOL(xen_entry_INT80_compat);
#endif
#endif
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index e7265a552f4f..502f72724b8d 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -17,12 +17,9 @@
*/
/* Handles exceptions in both to and from, but doesn't do access_ok */
-__must_check unsigned long
-copy_user_enhanced_fast_string(void *to, const void *from, unsigned len);
-__must_check unsigned long
-copy_user_generic_string(void *to, const void *from, unsigned len);
-__must_check unsigned long
-copy_user_generic_unrolled(void *to, const void *from, unsigned len);
+DECLARE_ASM_FUNC_SYMBOL(copy_user_enhanced_fast_string);
+DECLARE_ASM_FUNC_SYMBOL(copy_user_generic_string);
+DECLARE_ASM_FUNC_SYMBOL(copy_user_generic_unrolled);
static __always_inline __must_check unsigned long
copy_user_generic(void *to, const void *from, unsigned len)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index e9da3dc71254..0c60a7fa6fa5 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -530,7 +530,7 @@ extern struct paravirt_patch_site __start_parainstructions[],
* convention such that we can 'call' it from assembly.
*/
-extern void int3_magic(unsigned int *ptr); /* defined in asm */
+DECLARE_ASM_FUNC_SYMBOL(int3_magic);
asm (
" .pushsection .init.text, \"ax\", @progbits\n"
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 1b3ce3b4a2a2..9e0c07a82b44 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -589,7 +589,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
#ifdef CONFIG_DYNAMIC_FTRACE
-extern void ftrace_graph_call(void);
+DECLARE_ASM_FUNC_SYMBOL(ftrace_graph_call);
static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
{
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 04cafc057bed..7015ec9ca134 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -138,7 +138,7 @@ void paravirt_set_sched_clock(u64 (*func)(void))
}
/* These are in entry.S */
-extern void native_iret(void);
+DECLARE_ASM_FUNC_SYMBOL(native_iret);
static struct resource reserve_ioports = {
.start = 0,
@@ -376,7 +376,7 @@ NOKPROBE_SYMBOL(native_get_debugreg);
NOKPROBE_SYMBOL(native_set_debugreg);
NOKPROBE_SYMBOL(native_load_idt);
-void (*paravirt_iret)(void) = native_iret;
+asm_func_t paravirt_iret = native_iret;
#endif
EXPORT_SYMBOL(pv_ops);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 2837110e66ed..1f81f939d982 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -201,7 +201,7 @@ struct opcode {
const struct escape *esc;
const struct instr_dual *idual;
const struct mode_dual *mdual;
- void (*fastop)(struct fastop *fake);
+ fastop_t fastop;
} u;
int (*check_perm)(struct x86_emulate_ctxt *ctxt);
};
@@ -322,7 +322,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
__FOP_RET(#name)
#define FOP_START(op) \
- extern void em_##op(struct fastop *fake); \
+ DECLARE_ASM_FUNC_SYMBOL(em_##op); \
asm(".pushsection .text, \"ax\" \n\t" \
".global em_" #op " \n\t" \
".align " __stringify(FASTOP_SIZE) " \n\t" \
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 68b420289d7e..c9449ebee067 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -290,13 +290,8 @@ enum x86emul_mode {
#define X86EMUL_SMM_MASK (1 << 6)
#define X86EMUL_SMM_INSIDE_NMI_MASK (1 << 7)
-/*
- * fastop functions are declared as taking a never-defined fastop parameter,
- * so they can't be called from C directly.
- */
-struct fastop;
-
-typedef void (*fastop_t)(struct fastop *);
+/* fastop functions cannot be called from C directly. */
+typedef asm_func_t fastop_t;
struct x86_emulate_ctxt {
void *vcpu;
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 03149422dce2..bbd6cc58f6a8 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -613,8 +613,8 @@ DEFINE_IDTENTRY_RAW(xenpv_exc_machine_check)
#endif
struct trap_array_entry {
- void (*orig)(void);
- void (*xen)(void);
+ asm_func_t orig;
+ asm_func_t xen;
bool ist_okay;
};
@@ -673,7 +673,7 @@ static bool __ref get_trap_addr(void **addr, unsigned int ist)
struct trap_array_entry *entry = trap_array + nr;
if (*addr == entry->orig) {
- *addr = entry->xen;
+ *addr = (void *)entry->xen;
ist_okay = entry->ist_okay;
found = true;
break;
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 8d7ec49a35fb..b5ceb3007cfe 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -8,12 +8,12 @@
#include <xen/xen-ops.h>
/* These are code, but not functions. Defined in entry.S */
-extern const char xen_failsafe_callback[];
+DECLARE_ASM_FUNC_SYMBOL(xen_failsafe_callback);
-void xen_sysenter_target(void);
+DECLARE_ASM_FUNC_SYMBOL(xen_sysenter_target);
#ifdef CONFIG_X86_64
-void xen_syscall_target(void);
-void xen_syscall32_target(void);
+DECLARE_ASM_FUNC_SYMBOL(xen_syscall_target);
+DECLARE_ASM_FUNC_SYMBOL(xen_syscall32_target);
#endif
extern void *xen_initial_gdt;
@@ -136,7 +136,7 @@ __visible unsigned long xen_read_cr2(void);
__visible unsigned long xen_read_cr2_direct(void);
/* These are not functions, and cannot be called normally */
-__visible void xen_iret(void);
+DECLARE_ASM_FUNC_SYMBOL(xen_iret);
extern int xen_panic_handler_init(void);
--
2.33.0.rc2.250.ged5fa647cd-goog
To use the STACK_FRAME_NON_STANDARD macro for a static symbol
defined in inline assembly, we need a C declaration that implies
global visibility. This type mismatch confuses the compiler with
CONFIG_CFI_CLANG. This change adds an inline assembly version of
the macro to avoid the issue.
Signed-off-by: Sami Tolvanen <[email protected]>
---
include/linux/objtool.h | 6 ++++++
tools/include/linux/objtool.h | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/include/linux/objtool.h b/include/linux/objtool.h
index 7e72d975cb76..080e95174536 100644
--- a/include/linux/objtool.h
+++ b/include/linux/objtool.h
@@ -66,6 +66,11 @@ struct unwind_hint {
static void __used __section(".discard.func_stack_frame_non_standard") \
*__func_stack_frame_non_standard_##func = func
+#define ASM_STACK_FRAME_NON_STANDARD(func) \
+ ".pushsection .discard.func_stack_frame_non_standard, \"aw\"\n" \
+ ".long " __stringify(func) " - .\n" \
+ ".popsection\n"
+
#else /* __ASSEMBLY__ */
/*
@@ -127,6 +132,7 @@ struct unwind_hint {
#define UNWIND_HINT(sp_reg, sp_offset, type, end) \
"\n\t"
#define STACK_FRAME_NON_STANDARD(func)
+#define ASM_STACK_FRAME_NON_STANDARD(func)
#else
#define ANNOTATE_INTRA_FUNCTION_CALL
.macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
diff --git a/tools/include/linux/objtool.h b/tools/include/linux/objtool.h
index 7e72d975cb76..080e95174536 100644
--- a/tools/include/linux/objtool.h
+++ b/tools/include/linux/objtool.h
@@ -66,6 +66,11 @@ struct unwind_hint {
static void __used __section(".discard.func_stack_frame_non_standard") \
*__func_stack_frame_non_standard_##func = func
+#define ASM_STACK_FRAME_NON_STANDARD(func) \
+ ".pushsection .discard.func_stack_frame_non_standard, \"aw\"\n" \
+ ".long " __stringify(func) " - .\n" \
+ ".popsection\n"
+
#else /* __ASSEMBLY__ */
/*
@@ -127,6 +132,7 @@ struct unwind_hint {
#define UNWIND_HINT(sp_reg, sp_offset, type, end) \
"\n\t"
#define STACK_FRAME_NON_STANDARD(func)
+#define ASM_STACK_FRAME_NON_STANDARD(func)
#else
#define ANNOTATE_INTRA_FUNCTION_CALL
.macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
--
2.33.0.rc2.250.ged5fa647cd-goog
Ignore the __typeid__ relocations generated with CONFIG_CFI_CLANG
when loading modules.
Signed-off-by: Sami Tolvanen <[email protected]>
---
arch/x86/kernel/module.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index 5e9a34b5bd74..c4aeba237eef 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -197,6 +197,10 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
val -= (u64)loc;
write(loc, &val, 8);
break;
+ case R_X86_64_8:
+ if (!strncmp(strtab + sym->st_name, "__typeid__", 10))
+ break;
+ fallthrough;
default:
pr_err("%s: Unknown rela relocation: %llu\n",
me->name, ELF64_R_TYPE(rel[i].r_info));
--
2.33.0.rc2.250.ged5fa647cd-goog
Select ARCH_SUPPORTS_CFI_CLANG to allow CFI to be enabled with
Clang >=14.
Signed-off-by: Sami Tolvanen <[email protected]>
---
arch/x86/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 88fb922c23a0..c487c482ed67 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -107,6 +107,7 @@ config X86
select ARCH_SUPPORTS_KMAP_LOCAL_FORCE_MAP if NR_CPUS <= 4096
select ARCH_SUPPORTS_LTO_CLANG
select ARCH_SUPPORTS_LTO_CLANG_THIN
+ select ARCH_SUPPORTS_CFI_CLANG if X86_64 && CLANG_VERSION >= 140000
select ARCH_USE_BUILTIN_BSWAP
select ARCH_USE_MEMTEST
select ARCH_USE_QUEUED_RWLOCKS
--
2.33.0.rc2.250.ged5fa647cd-goog
Allow LTO to be used for cpu.c when CONFIG_CFI_CLANG is enabled to avoid
indirect call failures. CFI requires Clang >= 12, which doesn't have the
stack protector inlining bug.
Signed-off-by: Sami Tolvanen <[email protected]>
---
arch/x86/power/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/power/Makefile b/arch/x86/power/Makefile
index 379777572bc9..a0532851fed7 100644
--- a/arch/x86/power/Makefile
+++ b/arch/x86/power/Makefile
@@ -4,9 +4,11 @@
# itself be stack-protected
CFLAGS_cpu.o := -fno-stack-protector
+ifndef CONFIG_CFI_CLANG
# Clang may incorrectly inline functions with stack protector enabled into
# __restore_processor_state(): https://bugs.llvm.org/show_bug.cgi?id=47479
CFLAGS_REMOVE_cpu.o := $(CC_FLAGS_LTO)
+endif
obj-$(CONFIG_PM_SLEEP) += cpu.o
obj-$(CONFIG_HIBERNATION) += hibernate_$(BITS).o hibernate_asm_$(BITS).o hibernate.o
--
2.33.0.rc2.250.ged5fa647cd-goog
With CONFIG_CFI_CLANG, the compiler replaces function references with
references to the CFI jump table, which confuses objtool. This change,
based on Josh's initial patch [1], goes through the list of relocations
and replaces jump table symbols with the actual function symbols.
[1] https://lore.kernel.org/r/d743f4b36e120c06506567a9f87a062ae03da47f.1611263462.git.jpoimboe@redhat.com/
Reported-by: Sedat Dilek <[email protected]>
Suggested-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Sami Tolvanen <[email protected]>
---
tools/objtool/arch/x86/decode.c | 16 +++++++++
tools/objtool/elf.c | 51 ++++++++++++++++++++++++++++
tools/objtool/include/objtool/arch.h | 3 ++
tools/objtool/include/objtool/elf.h | 2 +-
4 files changed, 71 insertions(+), 1 deletion(-)
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index bc821056aba9..318189c8065e 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -62,6 +62,22 @@ bool arch_callee_saved_reg(unsigned char reg)
}
}
+unsigned long arch_cfi_section_reloc_offset(struct reloc *reloc)
+{
+ if (!reloc->addend)
+ return 0;
+
+ if (reloc->type == R_X86_64_PC32 || reloc->type == R_X86_64_PLT32)
+ return reloc->addend + 4;
+
+ return reloc->addend;
+}
+
+unsigned long arch_cfi_jump_reloc_offset(unsigned long offset)
+{
+ return offset + 1;
+}
+
unsigned long arch_dest_reloc_offset(int addend)
{
return addend + 4;
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 8676c7598728..05a5f51aad2c 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -18,6 +18,7 @@
#include <errno.h>
#include <objtool/builtin.h>
+#include <objtool/arch.h>
#include <objtool/elf.h>
#include <objtool/warn.h>
@@ -291,6 +292,10 @@ static int read_sections(struct elf *elf)
if (sec->sh.sh_flags & SHF_EXECINSTR)
elf->text_size += sec->len;
+ /* Detect -fsanitize=cfi jump table sections */
+ if (!strncmp(sec->name, ".text..L.cfi.jumptable", 22))
+ sec->cfi_jt = true;
+
list_add_tail(&sec->list, &elf->sections);
elf_hash_add(section, &sec->hash, sec->idx);
elf_hash_add(section_name, &sec->name_hash, str_hash(sec->name));
@@ -576,6 +581,49 @@ static int read_rela_reloc(struct section *sec, int i, struct reloc *reloc, unsi
return 0;
}
+/*
+ * CONFIG_CFI_CLANG replaces function relocations to refer to an intermediate
+ * jump table. Undo the conversion so objtool can make sense of things.
+ */
+static int fix_cfi_relocs(const struct elf *elf)
+{
+ struct section *sec;
+ struct reloc *reloc;
+
+ list_for_each_entry(sec, &elf->sections, list) {
+ list_for_each_entry(reloc, &sec->reloc_list, list) {
+ struct reloc *cfi_reloc;
+ unsigned long offset;
+
+ if (!reloc->sym->sec->cfi_jt)
+ continue;
+
+ if (reloc->sym->type == STT_SECTION)
+ offset = arch_cfi_section_reloc_offset(reloc);
+ else
+ offset = reloc->sym->offset;
+
+ /*
+ * The jump table immediately jumps to the actual function,
+ * so look up the relocation there.
+ */
+ offset = arch_cfi_jump_reloc_offset(offset);
+ cfi_reloc = find_reloc_by_dest(elf, reloc->sym->sec, offset);
+
+ if (!cfi_reloc || !cfi_reloc->sym) {
+ WARN("can't find a CFI jump table relocation at %s+0x%lx",
+ reloc->sym->sec->name, offset);
+ return -1;
+ }
+
+ reloc->sym = cfi_reloc->sym;
+ reloc->addend = 0;
+ }
+ }
+
+ return 0;
+}
+
static int read_relocs(struct elf *elf)
{
struct section *sec;
@@ -639,6 +687,9 @@ static int read_relocs(struct elf *elf)
tot_reloc += nr_reloc;
}
+ if (fix_cfi_relocs(elf))
+ return -1;
+
if (stats) {
printf("max_reloc: %lu\n", max_reloc);
printf("tot_reloc: %lu\n", tot_reloc);
diff --git a/tools/objtool/include/objtool/arch.h b/tools/objtool/include/objtool/arch.h
index 062bb6e9b865..2205b2b08268 100644
--- a/tools/objtool/include/objtool/arch.h
+++ b/tools/objtool/include/objtool/arch.h
@@ -81,6 +81,9 @@ unsigned long arch_jump_destination(struct instruction *insn);
unsigned long arch_dest_reloc_offset(int addend);
+unsigned long arch_cfi_section_reloc_offset(struct reloc *reloc);
+unsigned long arch_cfi_jump_reloc_offset(unsigned long offset);
+
const char *arch_nop_insn(int len);
int arch_decode_hint_reg(struct instruction *insn, u8 sp_reg);
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index e34395047530..d9c1dacc6572 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -39,7 +39,7 @@ struct section {
char *name;
int idx;
unsigned int len;
- bool changed, text, rodata, noinstr;
+ bool changed, text, rodata, noinstr, cfi_jt;
};
struct symbol {
--
2.33.0.rc2.250.ged5fa647cd-goog
With CONFIG_CFI_CLANG, the compiler changes function references to point
to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.
Signed-off-by: Sami Tolvanen <[email protected]>
---
include/linux/ftrace.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index a69f363b61bf..6fdfbcc14608 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -578,9 +578,10 @@ extern void ftrace_replace_code(int enable);
extern int ftrace_update_ftrace_func(ftrace_func_t func);
extern void ftrace_caller(void);
extern void ftrace_regs_caller(void);
-extern void ftrace_call(void);
-extern void ftrace_regs_call(void);
-extern void mcount_call(void);
+
+DECLARE_ASM_FUNC_SYMBOL(ftrace_call);
+DECLARE_ASM_FUNC_SYMBOL(ftrace_regs_call);
+DECLARE_ASM_FUNC_SYMBOL(mcount_call);
void ftrace_modify_all_code(int command);
--
2.33.0.rc2.250.ged5fa647cd-goog
The optprobe_template_func symbol is defined in inline assembly,
but it's not marked global, which conflicts with the C declaration
needed for STACK_FRAME_NON_STANDARD and confuses the compiler when
CONFIG_CFI_CLANG is enabled.
Marking the symbol global would make the compiler happy, but as the
compiler also generates a CFI jump table entry for all address-taken
functions, the jump table ends up containing a jump to the .rodata
section where optprobe_template_func resides, which results in an
objtool warning.
Use ASM_STACK_FRAME_NON_STANDARD instead to avoid both issues.
Signed-off-by: Sami Tolvanen <[email protected]>
---
arch/x86/kernel/kprobes/opt.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 71425ebba98a..95375ef5deee 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -103,6 +103,7 @@ static void synthesize_set_arg1(kprobe_opcode_t *addr, unsigned long val)
asm (
".pushsection .rodata\n"
"optprobe_template_func:\n"
+ ASM_STACK_FRAME_NON_STANDARD(optprobe_template_func)
".global optprobe_template_entry\n"
"optprobe_template_entry:\n"
#ifdef CONFIG_X86_64
@@ -154,9 +155,6 @@ asm (
"optprobe_template_end:\n"
".popsection\n");
-void optprobe_template_func(void);
-STACK_FRAME_NON_STANDARD(optprobe_template_func);
-
#define TMPL_CLAC_IDX \
((long)optprobe_template_clac - (long)optprobe_template_entry)
#define TMPL_MOVE_IDX \
--
2.33.0.rc2.250.ged5fa647cd-goog
On 8/23/21 10:13 AM, 'Sami Tolvanen' via Clang Built Linux wrote:
> This series adds support for Clang's Control-Flow Integrity (CFI)
> checking to x86_64. With CFI, the compiler injects a runtime
> check before 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. For more details, see:
>
> https://clang.llvm.org/docs/ControlFlowIntegrity.html
>
> Version 2 depends on Clang >=14, where we fixed the issue with
> referencing static functions from inline assembly. Based on the
> feedback for v1, this version also changes the declaration of
> functions that are not callable from C to use an opaque type,
> which stops the compiler from replacing references to them. This
> avoids the need to sprinkle function_nocfi() macros in the kernel
> code.
How invasive are the changes in clang 14 necessary to make CFI work?
Would it be possible to backport them to LLVM 13?
-Tom
>
> The first two patches contain objtool support for CFI, the
> remaining patches change function declarations to use opaque
> types, fix type mismatch issues that confuse the compiler, and
> disable CFI where it can't be used.
>
> You can also pull this series from
>
> https://github.com/samitolvanen/linux.git x86-cfi-v2
>
> ---
> Changes in v2:
> - Dropped the first objtool patch as the warnings were fixed in
> separate patches.
>
> - Changed fix_cfi_relocs() in objtool to not rely on jump table
> symbols, and to return an error if it can't find a relocation.
>
> - Fixed a build issue with ASM_STACK_FRAME_NON_STANDARD().
>
> - Dropped workarounds for inline assembly references to
> address-taken static functions with CFI as this was fixed in
> the compiler.
>
> - Changed the C declarations of non-callable functions to use
> opaque types and dropped the function_nocfi() patches.
>
> - Changed ARCH_SUPPORTS_CFI_CLANG to depend on Clang >=14 for
> the compiler fixes.
>
> Kees Cook (2):
> x86/extable: Do not mark exception callback as CFI
> x86, relocs: Ignore __typeid__ relocations
>
> Sami Tolvanen (12):
> objtool: Add CONFIG_CFI_CLANG support
> objtool: Add ASM_STACK_FRAME_NON_STANDARD
> linkage: Add DECLARE_ASM_FUNC_SYMBOL
> ftrace: Use an opaque type for functions not callable from C
> lkdtm: Disable UNSET_SMEP with CFI
> lkdtm: Use an opaque type for lkdtm_rodata_do_nothing
> x86: Use an opaque type for functions not callable from C
> x86/purgatory: Disable CFI
> x86, module: Ignore __typeid__ relocations
> x86, cpu: Use LTO for cpu.c with CFI
> x86, kprobes: Fix optprobe_template_func type mismatch
> x86, build: Allow CONFIG_CFI_CLANG to be selected
>
> arch/x86/Kconfig | 1 +
> arch/x86/include/asm/ftrace.h | 2 +-
> arch/x86/include/asm/idtentry.h | 10 +++---
> arch/x86/include/asm/page_64.h | 7 ++--
> arch/x86/include/asm/paravirt_types.h | 3 +-
> arch/x86/include/asm/processor.h | 2 +-
> arch/x86/include/asm/proto.h | 25 ++++++-------
> arch/x86/include/asm/uaccess_64.h | 9 ++---
> arch/x86/kernel/alternative.c | 2 +-
> arch/x86/kernel/ftrace.c | 2 +-
> arch/x86/kernel/kprobes/opt.c | 4 +--
> arch/x86/kernel/module.c | 4 +++
> arch/x86/kernel/paravirt.c | 4 +--
> arch/x86/kvm/emulate.c | 4 +--
> arch/x86/kvm/kvm_emulate.h | 9 ++---
> arch/x86/mm/extable.c | 1 +
> arch/x86/power/Makefile | 2 ++
> arch/x86/purgatory/Makefile | 2 +-
> arch/x86/tools/relocs.c | 7 ++++
> arch/x86/xen/enlighten_pv.c | 6 ++--
> arch/x86/xen/xen-ops.h | 10 +++---
> drivers/misc/lkdtm/bugs.c | 2 +-
> drivers/misc/lkdtm/lkdtm.h | 2 +-
> drivers/misc/lkdtm/perms.c | 2 +-
> drivers/misc/lkdtm/rodata.c | 2 +-
> include/linux/ftrace.h | 7 ++--
> include/linux/linkage.h | 13 +++++++
> include/linux/objtool.h | 6 ++++
> tools/include/linux/objtool.h | 6 ++++
> tools/objtool/arch/x86/decode.c | 16 +++++++++
> tools/objtool/elf.c | 51 +++++++++++++++++++++++++++
> tools/objtool/include/objtool/arch.h | 3 ++
> tools/objtool/include/objtool/elf.h | 2 +-
> 33 files changed, 166 insertions(+), 62 deletions(-)
>
>
> base-commit: d5ae8d7f85b7f6f6e60f1af8ff4be52b0926fde1
>
On Mon, Aug 23, 2021 at 10:16 AM Tom Stellard <[email protected]> wrote:
>
> On 8/23/21 10:13 AM, 'Sami Tolvanen' via Clang Built Linux wrote:
> > This series adds support for Clang's Control-Flow Integrity (CFI)
> > checking to x86_64. With CFI, the compiler injects a runtime
> > check before 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. For more details, see:
> >
> > https://clang.llvm.org/docs/ControlFlowIntegrity.html
> >
> > Version 2 depends on Clang >=14, where we fixed the issue with
> > referencing static functions from inline assembly. Based on the
> > feedback for v1, this version also changes the declaration of
> > functions that are not callable from C to use an opaque type,
> > which stops the compiler from replacing references to them. This
> > avoids the need to sprinkle function_nocfi() macros in the kernel
> > code.
>
> How invasive are the changes in clang 14 necessary to make CFI work?
> Would it be possible to backport them to LLVM 13?
I'm not sure what the LLVM backport policy is, but this specific fix
was quite simple:
https://reviews.llvm.org/rG7ce1c4da7726
Sami
On 8/23/21 10:20 AM, Sami Tolvanen wrote:
> On Mon, Aug 23, 2021 at 10:16 AM Tom Stellard <[email protected]> wrote:
>>
>> On 8/23/21 10:13 AM, 'Sami Tolvanen' via Clang Built Linux wrote:
>>> This series adds support for Clang's Control-Flow Integrity (CFI)
>>> checking to x86_64. With CFI, the compiler injects a runtime
>>> check before 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. For more details, see:
>>>
>>> https://clang.llvm.org/docs/ControlFlowIntegrity.html
>>>
>>> Version 2 depends on Clang >=14, where we fixed the issue with
>>> referencing static functions from inline assembly. Based on the
>>> feedback for v1, this version also changes the declaration of
>>> functions that are not callable from C to use an opaque type,
>>> which stops the compiler from replacing references to them. This
>>> avoids the need to sprinkle function_nocfi() macros in the kernel
>>> code.
>>
>> How invasive are the changes in clang 14 necessary to make CFI work?
>> Would it be possible to backport them to LLVM 13?
>
> I'm not sure what the LLVM backport policy is, but this specific fix
> was quite simple:
>
> https://reviews.llvm.org/rG7ce1c4da7726
>
That looks like something we could backport, I filed a bug to track
the backport: https://bugs.llvm.org/show_bug.cgi?id=51588.
Do you have any concerns about backporting it or do you think it's pretty
safe?
-Tom
> Sami
>
On Tue, Aug 24, 2021 at 10:26 AM Tom Stellard <[email protected]> wrote:
>
> On 8/23/21 10:20 AM, Sami Tolvanen wrote:
> > On Mon, Aug 23, 2021 at 10:16 AM Tom Stellard <[email protected]> wrote:
> >>
> >> On 8/23/21 10:13 AM, 'Sami Tolvanen' via Clang Built Linux wrote:
> >>> This series adds support for Clang's Control-Flow Integrity (CFI)
> >>> checking to x86_64. With CFI, the compiler injects a runtime
> >>> check before 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. For more details, see:
> >>>
> >>> https://clang.llvm.org/docs/ControlFlowIntegrity.html
> >>>
> >>> Version 2 depends on Clang >=14, where we fixed the issue with
> >>> referencing static functions from inline assembly. Based on the
> >>> feedback for v1, this version also changes the declaration of
> >>> functions that are not callable from C to use an opaque type,
> >>> which stops the compiler from replacing references to them. This
> >>> avoids the need to sprinkle function_nocfi() macros in the kernel
> >>> code.
> >>
> >> How invasive are the changes in clang 14 necessary to make CFI work?
> >> Would it be possible to backport them to LLVM 13?
> >
> > I'm not sure what the LLVM backport policy is, but this specific fix
> > was quite simple:
> >
> > https://reviews.llvm.org/rG7ce1c4da7726
> >
>
> That looks like something we could backport, I filed a bug to track
> the backport: https://bugs.llvm.org/show_bug.cgi?id=51588.
Great, thanks!
> Do you have any concerns about backporting it or do you think it's pretty
> safe?
No concerns, it should be safe to backport.
Sami
On Mon, Aug 23, 2021 at 10:13:04AM -0700, Sami Tolvanen wrote:
> This series adds support for Clang's Control-Flow Integrity (CFI)
> checking to x86_64. With CFI, the compiler injects a runtime
> check before 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. For more details, see:
If I understand this right; tp_stub_func() in kernel/tracepoint.c
violates this (as would much of the HAVE_STATIC_CALL=n code, luckily
that is not a valid x86_64 configuration).
Specifically, we assign &tp_stub_func to tracepoint_func::func, but that
function pointer is only ever indirectly called when cast to the
tracepoint prototype:
((void(*)(void *, proto))(it_func))(__data, args);
(see DEFINE_TRACE_FN() in linux/tracepoint.h)
This means the indirect function type and the target function type
mismatch, resulting in that runtime check you added to trigger.
Hitting tp_stub_func() at runtime is exceedingly rare, but possible.
I realize this is strictly UB per C, but realistically any CDECL ABI
requires that any function with arbitrary signature:
void foo(...)
{
}
translates to the exact same code. Specifically on x86-64, the super
impressive:
foo:
RET
And as such this works just fine. Except now you wrecked it.
On Tue, Aug 24, 2021 at 12:47 PM Peter Zijlstra <[email protected]> wrote:
>
> On Mon, Aug 23, 2021 at 10:13:04AM -0700, Sami Tolvanen wrote:
> > This series adds support for Clang's Control-Flow Integrity (CFI)
> > checking to x86_64. With CFI, the compiler injects a runtime
> > check before 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. For more details, see:
>
> If I understand this right; tp_stub_func() in kernel/tracepoint.c
> violates this (as would much of the HAVE_STATIC_CALL=n code, luckily
> that is not a valid x86_64 configuration).
>
> Specifically, we assign &tp_stub_func to tracepoint_func::func, but that
> function pointer is only ever indirectly called when cast to the
> tracepoint prototype:
>
> ((void(*)(void *, proto))(it_func))(__data, args);
>
> (see DEFINE_TRACE_FN() in linux/tracepoint.h)
>
> This means the indirect function type and the target function type
> mismatch, resulting in that runtime check you added to trigger.
Thanks for pointing this out. Yes, that would clearly trip CFI.
Any concerns about just writing a magic value to the slot instead of
pointing it to a stub function, and checking for it before the call?
> Hitting tp_stub_func() at runtime is exceedingly rare, but possible.
>
> I realize this is strictly UB per C, but realistically any CDECL ABI
> requires that any function with arbitrary signature:
>
> void foo(...)
> {
> }
>
> translates to the exact same code. Specifically on x86-64, the super
> impressive:
>
> foo:
> RET
>
> And as such this works just fine. Except now you wrecked it.
Sure. Another option is to disable CFI for the functions that perform
the call, but I would rather avoid that whenever possible.
Sami
On Wed, Aug 25, 2021 at 08:49:36AM -0700, Sami Tolvanen wrote:
> On Tue, Aug 24, 2021 at 12:47 PM Peter Zijlstra <[email protected]> wrote:
> >
> > On Mon, Aug 23, 2021 at 10:13:04AM -0700, Sami Tolvanen wrote:
> > > This series adds support for Clang's Control-Flow Integrity (CFI)
> > > checking to x86_64. With CFI, the compiler injects a runtime
> > > check before 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. For more details, see:
> >
> > If I understand this right; tp_stub_func() in kernel/tracepoint.c
> > violates this (as would much of the HAVE_STATIC_CALL=n code, luckily
> > that is not a valid x86_64 configuration).
> >
> > Specifically, we assign &tp_stub_func to tracepoint_func::func, but that
> > function pointer is only ever indirectly called when cast to the
> > tracepoint prototype:
> >
> > ((void(*)(void *, proto))(it_func))(__data, args);
> >
> > (see DEFINE_TRACE_FN() in linux/tracepoint.h)
> >
> > This means the indirect function type and the target function type
> > mismatch, resulting in that runtime check you added to trigger.
>
> Thanks for pointing this out. Yes, that would clearly trip CFI.
>
> Any concerns about just writing a magic value to the slot instead of
> pointing it to a stub function, and checking for it before the call?
Performance :-) that compare is going to be useless roughly 100% of the
time.
> > Hitting tp_stub_func() at runtime is exceedingly rare, but possible.
> >
> > I realize this is strictly UB per C, but realistically any CDECL ABI
> > requires that any function with arbitrary signature:
> >
> > void foo(...)
> > {
> > }
> >
> > translates to the exact same code. Specifically on x86-64, the super
> > impressive:
> >
> > foo:
> > RET
> >
> > And as such this works just fine. Except now you wrecked it.
>
> Sure. Another option is to disable CFI for the functions that perform
> the call, but I would rather avoid that whenever possible.
Is there no means of teaching the compiler about these magical
functions? There's only two possible stubs:
void foo(...)
{
}
and
unsigned long bar(...)
{
return 0;
}
Both exist in the kernel. We can easily give them a special function
attribute to call them out.
On 8/23/21 10:13 AM, Sami Tolvanen wrote:
> The kernel has several assembly functions that are not directly callable
> from C. Use an opaque type for these function prototypes to make misuse
> harder, and to avoid the need to annotate references to these functions
> for Clang's Control-Flow Integrity (CFI).
You have:
typedef const u8 *asm_func_t;
This is IMO a bit confusing. asm_func_t like this is an *address* of a
function, not a function.
To be fair, C is obnoxious, but I think this will lead to more confusion
than is idea. For example:
> -extern void __fentry__(void);
> +DECLARE_ASM_FUNC_SYMBOL(__fentry__);
Okay, __fentry__ is the name of a symbol, and the expression __fentry__
is a pointer (or an array that decays to a pointer, thanks C), which is
at least somewhat sensible. But:
> -extern void (*paravirt_iret)(void);
> +extern asm_func_t paravirt_iret;
Now paravirt_iret is a global variable that points to an asm func. I
bet people will read this wrong and, worse, type it wrong.
I think that there a couple ways to change this that would be a bit nicer.
1. typedef const u8 asm_func_t[];
This is almost nice, but asm_func_t will still be accepted as a function
argument, and the automatic decay rules will probably be confusing.
2. Rename asm_func_t to asm_func_ptr. Then it's at least a bit more clear.
3. Use an incomplete struct:
struct asm_func;
typedef struct asm_func asm_func;
extern asm_func some_func;
void *get_ptr(void)
{
return &some_func;
}
No macros required, and I think it's quite hard to misuse this by
accident. asm_func can't be passed as an argument or used as a variable
because it has incomplete type, and there are no arrays so the decay
rules aren't in effect.
--Andy
On Thu, Aug 26, 2021 at 4:44 AM Peter Zijlstra <[email protected]> wrote:
>
> On Wed, Aug 25, 2021 at 08:49:36AM -0700, Sami Tolvanen wrote:
> > On Tue, Aug 24, 2021 at 12:47 PM Peter Zijlstra <[email protected]> wrote:
> > >
> > > On Mon, Aug 23, 2021 at 10:13:04AM -0700, Sami Tolvanen wrote:
> > > > This series adds support for Clang's Control-Flow Integrity (CFI)
> > > > checking to x86_64. With CFI, the compiler injects a runtime
> > > > check before 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. For more details, see:
> > >
> > > If I understand this right; tp_stub_func() in kernel/tracepoint.c
> > > violates this (as would much of the HAVE_STATIC_CALL=n code, luckily
> > > that is not a valid x86_64 configuration).
> > >
> > > Specifically, we assign &tp_stub_func to tracepoint_func::func, but that
> > > function pointer is only ever indirectly called when cast to the
> > > tracepoint prototype:
> > >
> > > ((void(*)(void *, proto))(it_func))(__data, args);
> > >
> > > (see DEFINE_TRACE_FN() in linux/tracepoint.h)
> > >
> > > This means the indirect function type and the target function type
> > > mismatch, resulting in that runtime check you added to trigger.
> >
> > Thanks for pointing this out. Yes, that would clearly trip CFI.
> >
> > Any concerns about just writing a magic value to the slot instead of
> > pointing it to a stub function, and checking for it before the call?
>
> Performance :-) that compare is going to be useless roughly 100% of the
> time.
Makes sense. I suppose we could move the call into a macro and do the
comparison only when CFI is enabled to avoid a performance hit with
other configs.
> > > Hitting tp_stub_func() at runtime is exceedingly rare, but possible.
> > >
> > > I realize this is strictly UB per C, but realistically any CDECL ABI
> > > requires that any function with arbitrary signature:
> > >
> > > void foo(...)
> > > {
> > > }
> > >
> > > translates to the exact same code. Specifically on x86-64, the super
> > > impressive:
> > >
> > > foo:
> > > RET
> > >
> > > And as such this works just fine. Except now you wrecked it.
> >
> > Sure. Another option is to disable CFI for the functions that perform
> > the call, but I would rather avoid that whenever possible.
>
> Is there no means of teaching the compiler about these magical
> functions? There's only two possible stubs:
>
> void foo(...)
> {
> }
>
> and
>
> unsigned long bar(...)
> {
> return 0;
> }
>
> Both exist in the kernel. We can easily give them a special function
> attribute to call them out.
Clang doesn't have a way to always allow calls to specific functions,
but it might be feasible to implement this in the slowpath handler
without explicit compiler support. I'll see if I can come up with
something reasonable for v3.
Sami
On Thu, Aug 26, 2021 at 9:54 AM Andy Lutomirski <[email protected]> wrote:
>
> On 8/23/21 10:13 AM, Sami Tolvanen wrote:
> > The kernel has several assembly functions that are not directly callable
> > from C. Use an opaque type for these function prototypes to make misuse
> > harder, and to avoid the need to annotate references to these functions
> > for Clang's Control-Flow Integrity (CFI).
>
> You have:
>
> typedef const u8 *asm_func_t;
>
> This is IMO a bit confusing. asm_func_t like this is an *address* of a
> function, not a function.
>
> To be fair, C is obnoxious, but I think this will lead to more confusion
> than is idea. For example:
>
> > -extern void __fentry__(void);
> > +DECLARE_ASM_FUNC_SYMBOL(__fentry__);
>
> Okay, __fentry__ is the name of a symbol, and the expression __fentry__
> is a pointer (or an array that decays to a pointer, thanks C), which is
> at least somewhat sensible. But:
>
> > -extern void (*paravirt_iret)(void);
> > +extern asm_func_t paravirt_iret;
>
> Now paravirt_iret is a global variable that points to an asm func. I
> bet people will read this wrong and, worse, type it wrong.
>
> I think that there a couple ways to change this that would be a bit nicer.
>
> 1. typedef const u8 asm_func_t[];
>
> This is almost nice, but asm_func_t will still be accepted as a function
> argument, and the automatic decay rules will probably be confusing.
>
> 2. Rename asm_func_t to asm_func_ptr. Then it's at least a bit more clear.
>
> 3. Use an incomplete struct:
>
> struct asm_func;
>
> typedef struct asm_func asm_func;
>
> extern asm_func some_func;
>
> void *get_ptr(void)
> {
> return &some_func;
> }
>
> No macros required, and I think it's quite hard to misuse this by
> accident. asm_func can't be passed as an argument or used as a variable
> because it has incomplete type, and there are no arrays so the decay
> rules aren't in effect.
I considered using an incomplete struct, but that would require an
explicit '&' when we take the address of these symbols, which I
thought would be unnecessary churn. Unless you strongly prefer this
one, I'll go with option 2 and rename asm_func_t to asm_func_ptr in
v3.
Sami
On Thu, Aug 26, 2021, at 3:11 PM, Sami Tolvanen wrote:
> On Thu, Aug 26, 2021 at 9:54 AM Andy Lutomirski <[email protected]> wrote:
> >
> > On 8/23/21 10:13 AM, Sami Tolvanen wrote:
> > > The kernel has several assembly functions that are not directly callable
> > > from C. Use an opaque type for these function prototypes to make misuse
> > > harder, and to avoid the need to annotate references to these functions
> > > for Clang's Control-Flow Integrity (CFI).
> >
> > You have:
> >
> > typedef const u8 *asm_func_t;
> >
> > This is IMO a bit confusing. asm_func_t like this is an *address* of a
> > function, not a function.
> >
> > To be fair, C is obnoxious, but I think this will lead to more confusion
> > than is idea. For example:
> >
> > > -extern void __fentry__(void);
> > > +DECLARE_ASM_FUNC_SYMBOL(__fentry__);
> >
> > Okay, __fentry__ is the name of a symbol, and the expression __fentry__
> > is a pointer (or an array that decays to a pointer, thanks C), which is
> > at least somewhat sensible. But:
> >
> > > -extern void (*paravirt_iret)(void);
> > > +extern asm_func_t paravirt_iret;
> >
> > Now paravirt_iret is a global variable that points to an asm func. I
> > bet people will read this wrong and, worse, type it wrong.
> >
> > I think that there a couple ways to change this that would be a bit nicer.
> >
> > 1. typedef const u8 asm_func_t[];
> >
> > This is almost nice, but asm_func_t will still be accepted as a function
> > argument, and the automatic decay rules will probably be confusing.
> >
> > 2. Rename asm_func_t to asm_func_ptr. Then it's at least a bit more clear.
> >
> > 3. Use an incomplete struct:
> >
> > struct asm_func;
> >
> > typedef struct asm_func asm_func;
> >
> > extern asm_func some_func;
> >
> > void *get_ptr(void)
> > {
> > return &some_func;
> > }
> >
> > No macros required, and I think it's quite hard to misuse this by
> > accident. asm_func can't be passed as an argument or used as a variable
> > because it has incomplete type, and there are no arrays so the decay
> > rules aren't in effect.
>
> I considered using an incomplete struct, but that would require an
> explicit '&' when we take the address of these symbols, which I
> thought would be unnecessary churn. Unless you strongly prefer this
> one, I'll go with option 2 and rename asm_func_t to asm_func_ptr in
> v3.
>
Do you have a sense for how many occurrences there are that would need an &?
> Sami
>
On Thu, Aug 26, 2021 at 4:24 PM Andy Lutomirski <[email protected]> wrote:
>
>
>
> On Thu, Aug 26, 2021, at 3:11 PM, Sami Tolvanen wrote:
> > On Thu, Aug 26, 2021 at 9:54 AM Andy Lutomirski <[email protected]> wrote:
> > >
> > > On 8/23/21 10:13 AM, Sami Tolvanen wrote:
> > > > The kernel has several assembly functions that are not directly callable
> > > > from C. Use an opaque type for these function prototypes to make misuse
> > > > harder, and to avoid the need to annotate references to these functions
> > > > for Clang's Control-Flow Integrity (CFI).
> > >
> > > You have:
> > >
> > > typedef const u8 *asm_func_t;
> > >
> > > This is IMO a bit confusing. asm_func_t like this is an *address* of a
> > > function, not a function.
> > >
> > > To be fair, C is obnoxious, but I think this will lead to more confusion
> > > than is idea. For example:
> > >
> > > > -extern void __fentry__(void);
> > > > +DECLARE_ASM_FUNC_SYMBOL(__fentry__);
> > >
> > > Okay, __fentry__ is the name of a symbol, and the expression __fentry__
> > > is a pointer (or an array that decays to a pointer, thanks C), which is
> > > at least somewhat sensible. But:
> > >
> > > > -extern void (*paravirt_iret)(void);
> > > > +extern asm_func_t paravirt_iret;
> > >
> > > Now paravirt_iret is a global variable that points to an asm func. I
> > > bet people will read this wrong and, worse, type it wrong.
> > >
> > > I think that there a couple ways to change this that would be a bit nicer.
> > >
> > > 1. typedef const u8 asm_func_t[];
> > >
> > > This is almost nice, but asm_func_t will still be accepted as a function
> > > argument, and the automatic decay rules will probably be confusing.
> > >
> > > 2. Rename asm_func_t to asm_func_ptr. Then it's at least a bit more clear.
> > >
> > > 3. Use an incomplete struct:
> > >
> > > struct asm_func;
> > >
> > > typedef struct asm_func asm_func;
> > >
> > > extern asm_func some_func;
> > >
> > > void *get_ptr(void)
> > > {
> > > return &some_func;
> > > }
> > >
> > > No macros required, and I think it's quite hard to misuse this by
> > > accident. asm_func can't be passed as an argument or used as a variable
> > > because it has incomplete type, and there are no arrays so the decay
> > > rules aren't in effect.
> >
> > I considered using an incomplete struct, but that would require an
> > explicit '&' when we take the address of these symbols, which I
> > thought would be unnecessary churn. Unless you strongly prefer this
> > one, I'll go with option 2 and rename asm_func_t to asm_func_ptr in
> > v3.
> >
>
> Do you have a sense for how many occurrences there are that would need an &?
Quick grepping suggests there are ~80 occurrences in arch/x86. ~40 of
these are in kernel/idt.c.
Sami