KCFI is a proposed forward-edge control-flow integrity scheme for
Clang, which is more suitable for kernel use than the existing CFI
scheme used by CONFIG_CFI_CLANG. KCFI doesn't require LTO, doesn't
alter function references to point to a jump table, and won't break
function address equality. The latest LLVM patches are here:
https://reviews.llvm.org/D119296
https://reviews.llvm.org/D124211
This RFC series replaces the current arm64 CFI implementation with
KCFI and adds support for x86_64.
The proposed compiler patches add a built-in function that allows
CFI checks to be disabled for specific indirect calls. This is
necessary to prevent unnecessary checks from being emitted for
static_call trampoline calls that are later patched into direct
calls. However, as the call expression must be passed as an argument
to the built-in, this requires changing the static_call macro API to
include the call arguments. Patch 14 changes the macros to accept
arguments and patch 15 disables checks for the generated calls.
KCFI also requires assembly functions that are indirectly called
from C code to be annotated with type identifiers. As type
information is only available in C, the compiler emits expected
type identifiers into the symbol table, so they can be referenced
from assembly without having to hardcode type hashes. Patch 7 adds
helper macros for annotating functions, and patches 8 and 18 add
annotations.
In case of a type mismatch, KCFI always traps. To support error
handling, the compiler generates a .kcfi_traps section that contains
the locations of each trap. Patches 9 and 21 add arch-specific error
handlers. In addition, to support x86_64, objtool must be able to
identify KCFI type identifiers that are emitted before function
entries. The compiler generates an additional .kcfi_types section,
which points to each emitted type identifier. Patch 16 adds objtool
support.
To test this series, you'll need to compile your own Clang toolchain
with the patches linked above. You can also find the complete source
tree here:
https://github.com/samitolvanen/llvm-project/commits/kcfi-rfc
This series is also available in GitHub:
https://github.com/samitolvanen/linux/commits/kcfi-rfc
Sami Tolvanen (21):
efi/libstub: Filter out CC_FLAGS_CFI
arm64/vdso: Filter out CC_FLAGS_CFI
kallsyms: Ignore __kcfi_typeid_
cfi: Remove CONFIG_CFI_CLANG_SHADOW
cfi: Drop __CFI_ADDRESSABLE
cfi: Switch to -fsanitize=kcfi
cfi: Add type helper macros
arm64/crypto: Add types to indirect called assembly functions
arm64: Add CFI error handling
treewide: Drop function_nocfi
treewide: Drop WARN_ON_FUNCTION_MISMATCH
treewide: Drop __cficanonical
cfi: Add the cfi_unchecked macro
treewide: static_call: Pass call arguments to the macro
static_call: Use cfi_unchecked
objtool: Add support for CONFIG_CFI_CLANG
x86/tools/relocs: Ignore __kcfi_typeid_ relocations
x86: Add types to indirect called assembly functions
x86/purgatory: Disable CFI
x86/vdso: Disable CFI
x86: Add support for CONFIG_CFI_CLANG
Makefile | 13 +-
arch/Kconfig | 18 +-
arch/arm/include/asm/paravirt.h | 2 +-
arch/arm64/crypto/ghash-ce-core.S | 5 +-
arch/arm64/crypto/sm3-ce-core.S | 3 +-
arch/arm64/include/asm/brk-imm.h | 2 +
arch/arm64/include/asm/compiler.h | 16 -
arch/arm64/include/asm/ftrace.h | 2 +-
arch/arm64/include/asm/insn.h | 1 +
arch/arm64/include/asm/mmu_context.h | 2 +-
arch/arm64/include/asm/paravirt.h | 2 +-
arch/arm64/kernel/acpi_parking_protocol.c | 2 +-
arch/arm64/kernel/cpufeature.c | 2 +-
arch/arm64/kernel/ftrace.c | 2 +-
arch/arm64/kernel/machine_kexec.c | 2 +-
arch/arm64/kernel/psci.c | 2 +-
arch/arm64/kernel/smp_spin_table.c | 2 +-
arch/arm64/kernel/traps.c | 57 ++++
arch/arm64/kernel/vdso/Makefile | 3 +-
arch/x86/Kconfig | 1 +
arch/x86/crypto/aesni-intel_glue.c | 7 +-
arch/x86/crypto/blowfish-x86_64-asm_64.S | 5 +-
arch/x86/entry/vdso/Makefile | 3 +-
arch/x86/events/core.c | 40 +--
arch/x86/include/asm/kvm_host.h | 6 +-
arch/x86/include/asm/linkage.h | 7 +
arch/x86/include/asm/paravirt.h | 4 +-
arch/x86/kernel/traps.c | 39 ++-
arch/x86/kvm/cpuid.c | 2 +-
arch/x86/kvm/hyperv.c | 4 +-
arch/x86/kvm/irq.c | 2 +-
arch/x86/kvm/kvm_cache_regs.h | 10 +-
arch/x86/kvm/lapic.c | 32 +-
arch/x86/kvm/mmu.h | 4 +-
arch/x86/kvm/mmu/mmu.c | 8 +-
arch/x86/kvm/mmu/spte.c | 4 +-
arch/x86/kvm/pmu.c | 4 +-
arch/x86/kvm/trace.h | 4 +-
arch/x86/kvm/x86.c | 326 ++++++++++-----------
arch/x86/kvm/x86.h | 4 +-
arch/x86/kvm/xen.c | 4 +-
arch/x86/lib/memcpy_64.S | 3 +-
arch/x86/purgatory/Makefile | 4 +
arch/x86/tools/relocs.c | 1 +
drivers/cpufreq/amd-pstate.c | 8 +-
drivers/firmware/efi/libstub/Makefile | 2 +
drivers/firmware/psci/psci.c | 4 +-
drivers/misc/lkdtm/usercopy.c | 2 +-
include/asm-generic/bug.h | 16 -
include/asm-generic/vmlinux.lds.h | 38 +--
include/linux/cfi.h | 50 ++--
include/linux/cfi_types.h | 57 ++++
include/linux/compiler-clang.h | 10 +-
include/linux/compiler.h | 16 +-
include/linux/compiler_types.h | 4 +-
include/linux/entry-common.h | 2 +-
include/linux/init.h | 4 +-
include/linux/kernel.h | 2 +-
include/linux/module.h | 8 +-
include/linux/pci.h | 4 +-
include/linux/perf_event.h | 6 +-
include/linux/sched.h | 2 +-
include/linux/static_call.h | 18 +-
include/linux/static_call_types.h | 13 +-
include/linux/tracepoint.h | 2 +-
kernel/cfi.c | 340 ++++------------------
kernel/kthread.c | 3 +-
kernel/module.c | 49 +---
kernel/static_call_inline.c | 2 +-
kernel/trace/bpf_trace.c | 2 +-
kernel/workqueue.c | 2 +-
scripts/Makefile.build | 3 +-
scripts/kallsyms.c | 1 +
scripts/link-vmlinux.sh | 3 +
scripts/module.lds.S | 24 +-
security/keys/trusted-keys/trusted_core.c | 14 +-
tools/include/linux/static_call_types.h | 13 +-
tools/objtool/arch/x86/include/arch/elf.h | 2 +
tools/objtool/builtin-check.c | 3 +-
tools/objtool/check.c | 128 +++++++-
tools/objtool/elf.c | 13 +
tools/objtool/include/objtool/arch.h | 1 +
tools/objtool/include/objtool/builtin.h | 2 +-
tools/objtool/include/objtool/elf.h | 2 +
84 files changed, 748 insertions(+), 793 deletions(-)
create mode 100644 include/linux/cfi_types.h
--
2.36.0.464.gb9c8b46e94-goog
On Fri, Apr 29, 2022 at 01:36:23PM -0700, Sami Tolvanen wrote:
> KCFI is a proposed forward-edge control-flow integrity scheme for
> Clang, which is more suitable for kernel use than the existing CFI
> scheme used by CONFIG_CFI_CLANG. KCFI doesn't require LTO, doesn't
> alter function references to point to a jump table, and won't break
> function address equality.
???? :)
> The latest LLVM patches are here:
>
> https://reviews.llvm.org/D119296
> https://reviews.llvm.org/D124211
>
> [...]
> To test this series, you'll need to compile your own Clang toolchain
> with the patches linked above. You can also find the complete source
> tree here:
>
> https://github.com/samitolvanen/llvm-project/commits/kcfi-rfc
And note that this RFC is seeking to break a bit of a circular dependency
with regard to the design of __builtin_kcfi_call_unchecked (D124211
above), as the implementation has gone around a few times in review within
LLVM, and we want to make sure that kernel folks are okay with what was
settled on. If there are no objections on the kernel side, then we can
land the KCFI patches, as this is basically the only remaining blocker.
-Kees
--
Kees Cook
With CONFIG_CFI_CLANG, assembly functions called indirectly
from C code must be annotated with type identifiers to pass CFI
checking. The compiler emits a __kcfi_typeid_<function> symbol for
each address-taken function declaration in C, which contains the
expected type identifier. Add typed versions of SYM_FUNC_START and
SYM_FUNC_START_ALIAS, which emit the type identifier before the
function.
Signed-off-by: Sami Tolvanen <[email protected]>
---
include/linux/cfi_types.h | 57 +++++++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
create mode 100644 include/linux/cfi_types.h
diff --git a/include/linux/cfi_types.h b/include/linux/cfi_types.h
new file mode 100644
index 000000000000..dd16e755a197
--- /dev/null
+++ b/include/linux/cfi_types.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Clang Control Flow Integrity (CFI) type definitions.
+ */
+#ifndef _LINUX_CFI_TYPES_H
+#define _LINUX_CFI_TYPES_H
+
+#ifdef CONFIG_CFI_CLANG
+#include <linux/linkage.h>
+
+#ifdef __ASSEMBLY__
+/*
+ * Use the __kcfi_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
+#define __CFI_TYPE(name) \
+ .4byte __kcfi_typeid_##name
+#endif
+
+#define SYM_TYPED_ENTRY(name, fname, linkage, align...) \
+ linkage(name) ASM_NL \
+ align ASM_NL \
+ __CFI_TYPE(fname) ASM_NL \
+ name:
+
+#define __SYM_TYPED_FUNC_START_ALIAS(name, fname) \
+ SYM_TYPED_ENTRY(name, fname, SYM_L_GLOBAL, SYM_A_ALIGN)
+
+#define __SYM_TYPED_FUNC_START(name, fname) \
+ SYM_TYPED_ENTRY(name, fname, SYM_L_GLOBAL, SYM_A_ALIGN)
+
+#endif /* __ASSEMBLY__ */
+
+#else /* CONFIG_CFI_CLANG */
+
+#ifdef __ASSEMBLY__
+#define __SYM_TYPED_FUNC_START_ALIAS(name, fname) \
+ SYM_FUNC_START_ALIAS(name)
+
+#define __SYM_TYPED_FUNC_START(name, fname) \
+ SYM_FUNC_START(name)
+#endif /* __ASSEMBLY__ */
+
+#endif /* CONFIG_CFI_CLANG */
+
+#ifdef __ASSEMBLY__
+#define SYM_TYPED_FUNC_START_ALIAS(name) \
+ __SYM_TYPED_FUNC_START_ALIAS(name, name)
+
+#define SYM_TYPED_FUNC_START(name) \
+ __SYM_TYPED_FUNC_START(name, name)
+#endif /* __ASSEMBLY__ */
+
+#endif /* _LINUX_CFI_TYPES_H */
--
2.36.0.464.gb9c8b46e94-goog
On 22/04/29 01:36PM, Sami Tolvanen wrote:
> KCFI is a proposed forward-edge control-flow integrity scheme for
> Clang, which is more suitable for kernel use than the existing CFI
> scheme used by CONFIG_CFI_CLANG. KCFI doesn't require LTO, doesn't
> alter function references to point to a jump table, and won't break
> function address equality. The latest LLVM patches are here:
>
> https://reviews.llvm.org/D119296
> https://reviews.llvm.org/D124211
Many thanks for continuing to work on this! As a user who has been
following the evolution of this patch series for a while now, I have a
couple of burning questions:
1) The LLVM patch says that kCFI is not compatible with execute-only
memory. Is there a plan ahead for kCFI if and when execute-only memory
is implemented?
2) kCFI only checks indirect calls while Clang's traditional CFI has
more schemes like bad cast checking and so on. Are there any major
security tradeoffs as a result of this?
V/R
Kenton Groombridge
The __CFI_ADDRESSABLE macro is used for init_module and cleanup_module
to ensure we have the address of the CFI jump table, and with
CONFIG_X86_KERNEL_IBT to ensure LTO won't optimize away the symbols.
As __CFI_ADDRESSABLE is no longer necessary with -fsanitize=kcfi, add
a more flexible version of the __ADDRESSABLE macro and always ensure
these symbols won't be dropped.
Signed-off-by: Sami Tolvanen <[email protected]>
---
include/linux/cfi.h | 20 --------------------
include/linux/compiler.h | 6 ++++--
include/linux/module.h | 4 ++--
3 files changed, 6 insertions(+), 24 deletions(-)
diff --git a/include/linux/cfi.h b/include/linux/cfi.h
index 4ab51c067007..2cdbc0fbd0ab 100644
--- a/include/linux/cfi.h
+++ b/include/linux/cfi.h
@@ -13,26 +13,6 @@ typedef void (*cfi_check_fn)(uint64_t id, void *ptr, void *diag);
/* Compiler-generated function in each module, and the kernel */
extern void __cfi_check(uint64_t id, void *ptr, void *diag);
-/*
- * Force the compiler to generate a CFI jump table entry for a function
- * and store the jump table address to __cfi_jt_<function>.
- */
-#define __CFI_ADDRESSABLE(fn, __attr) \
- const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn
-
-#else /* !CONFIG_CFI_CLANG */
-
-#ifdef CONFIG_X86_KERNEL_IBT
-
-#define __CFI_ADDRESSABLE(fn, __attr) \
- const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn
-
-#endif /* CONFIG_X86_KERNEL_IBT */
-
#endif /* CONFIG_CFI_CLANG */
-#ifndef __CFI_ADDRESSABLE
-#define __CFI_ADDRESSABLE(fn, __attr)
-#endif
-
#endif /* _LINUX_CFI_H */
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 219aa5ddbc73..9303f5fe5d89 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -221,9 +221,11 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
* otherwise, or eliminated entirely due to lack of references that are
* visible to the compiler.
*/
-#define __ADDRESSABLE(sym) \
- static void * __section(".discard.addressable") __used \
+#define ___ADDRESSABLE(sym, __attrs) \
+ static void * __used __attrs \
__UNIQUE_ID(__PASTE(__addressable_,sym)) = (void *)&sym;
+#define __ADDRESSABLE(sym) \
+ ___ADDRESSABLE(sym, __section(".discard.addressable"))
/**
* offset_to_ptr - convert a relative memory offset to an absolute pointer
diff --git a/include/linux/module.h b/include/linux/module.h
index 1e135fd5c076..87857275c047 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -132,7 +132,7 @@ extern void cleanup_module(void);
{ return initfn; } \
int init_module(void) __copy(initfn) \
__attribute__((alias(#initfn))); \
- __CFI_ADDRESSABLE(init_module, __initdata);
+ ___ADDRESSABLE(init_module, __initdata);
/* This is only required if you want to be unloadable. */
#define module_exit(exitfn) \
@@ -140,7 +140,7 @@ extern void cleanup_module(void);
{ return exitfn; } \
void cleanup_module(void) __copy(exitfn) \
__attribute__((alias(#exitfn))); \
- __CFI_ADDRESSABLE(cleanup_module, __exitdata);
+ ___ADDRESSABLE(cleanup_module, __exitdata);
#endif
--
2.36.0.464.gb9c8b46e94-goog
The compiler generates CFI type identifier symbols for annotating
assembly functions at link time. Ignore them in kallsyms.
Signed-off-by: Sami Tolvanen <[email protected]>
---
scripts/kallsyms.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 8caabddf817c..eebd02e4b832 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -118,6 +118,7 @@ static bool is_ignored_symbol(const char *name, char type)
"__ThumbV7PILongThunk_",
"__LA25Thunk_", /* mips lld */
"__microLA25Thunk_",
+ "__kcfi_typeid_", /* CFI type identifiers */
NULL
};
--
2.36.0.464.gb9c8b46e94-goog
With -fsanitize=kcfi, we no longer need function_nocfi() as
the compiler won't change function references to point to a
jump table. Remove all implementations and uses of the macro.
Signed-off-by: Sami Tolvanen <[email protected]>
---
arch/arm64/include/asm/compiler.h | 16 ----------------
arch/arm64/include/asm/ftrace.h | 2 +-
arch/arm64/include/asm/mmu_context.h | 2 +-
arch/arm64/kernel/acpi_parking_protocol.c | 2 +-
arch/arm64/kernel/cpufeature.c | 2 +-
arch/arm64/kernel/ftrace.c | 2 +-
arch/arm64/kernel/machine_kexec.c | 2 +-
arch/arm64/kernel/psci.c | 2 +-
arch/arm64/kernel/smp_spin_table.c | 2 +-
drivers/firmware/psci/psci.c | 4 ++--
drivers/misc/lkdtm/usercopy.c | 2 +-
include/linux/compiler.h | 10 ----------
12 files changed, 11 insertions(+), 37 deletions(-)
diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
index dc3ea4080e2e..6fb2e6bcc392 100644
--- a/arch/arm64/include/asm/compiler.h
+++ b/arch/arm64/include/asm/compiler.h
@@ -23,20 +23,4 @@
#define __builtin_return_address(val) \
(void *)(ptrauth_clear_pac((unsigned long)__builtin_return_address(val)))
-#ifdef CONFIG_CFI_CLANG
-/*
- * With CONFIG_CFI_CLANG, the compiler replaces function address
- * references with the address of the function's CFI jump table
- * entry. The function_nocfi macro always returns the address of the
- * actual function instead.
- */
-#define function_nocfi(x) ({ \
- void *addr; \
- asm("adrp %0, " __stringify(x) "\n\t" \
- "add %0, %0, :lo12:" __stringify(x) \
- : "=r" (addr)); \
- addr; \
-})
-#endif
-
#endif /* __ASM_COMPILER_H */
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 1494cfa8639b..c96d47cb8f46 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -26,7 +26,7 @@
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
#define ARCH_SUPPORTS_FTRACE_OPS 1
#else
-#define MCOUNT_ADDR ((unsigned long)function_nocfi(_mcount))
+#define MCOUNT_ADDR ((unsigned long)_mcount)
#endif
/* The BL at the callsite's adjusted rec->ip */
diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
index 6770667b34a3..c9df5ab2c448 100644
--- a/arch/arm64/include/asm/mmu_context.h
+++ b/arch/arm64/include/asm/mmu_context.h
@@ -164,7 +164,7 @@ static inline void __nocfi cpu_replace_ttbr1(pgd_t *pgdp)
ttbr1 |= TTBR_CNP_BIT;
}
- replace_phys = (void *)__pa_symbol(function_nocfi(idmap_cpu_replace_ttbr1));
+ replace_phys = (void *)__pa_symbol(idmap_cpu_replace_ttbr1);
cpu_install_idmap();
replace_phys(ttbr1);
diff --git a/arch/arm64/kernel/acpi_parking_protocol.c b/arch/arm64/kernel/acpi_parking_protocol.c
index bfeeb5319abf..b1990e38aed0 100644
--- a/arch/arm64/kernel/acpi_parking_protocol.c
+++ b/arch/arm64/kernel/acpi_parking_protocol.c
@@ -99,7 +99,7 @@ static int acpi_parking_protocol_cpu_boot(unsigned int cpu)
* that read this address need to convert this address to the
* Boot-Loader's endianness before jumping.
*/
- writeq_relaxed(__pa_symbol(function_nocfi(secondary_entry)),
+ writeq_relaxed(__pa_symbol(secondary_entry),
&mailbox->entry_point);
writel_relaxed(cpu_entry->gic_cpu_id, &mailbox->cpu_id);
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index d72c4b4d389c..dae07d99508b 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1619,7 +1619,7 @@ kpti_install_ng_mappings(const struct arm64_cpu_capabilities *__unused)
if (arm64_use_ng_mappings)
return;
- remap_fn = (void *)__pa_symbol(function_nocfi(idmap_kpti_install_ng_mappings));
+ remap_fn = (void *)__pa_symbol(idmap_kpti_install_ng_mappings);
cpu_install_idmap();
remap_fn(cpu, num_online_cpus(), __pa_symbol(swapper_pg_dir));
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 4506c4a90ac1..4128ca6ed485 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -56,7 +56,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
unsigned long pc;
u32 new;
- pc = (unsigned long)function_nocfi(ftrace_call);
+ pc = (unsigned long)ftrace_call;
new = aarch64_insn_gen_branch_imm(pc, (unsigned long)func,
AARCH64_INSN_BRANCH_LINK);
diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index e16b248699d5..4eb5388aa5a6 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c
@@ -204,7 +204,7 @@ void machine_kexec(struct kimage *kimage)
typeof(cpu_soft_restart) *restart;
cpu_install_idmap();
- restart = (void *)__pa_symbol(function_nocfi(cpu_soft_restart));
+ restart = (void *)__pa_symbol(cpu_soft_restart);
restart(is_hyp_nvhe(), kimage->start, kimage->arch.dtb_mem,
0, 0);
} else {
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index ab7f4c476104..29a8e444db83 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -38,7 +38,7 @@ static int __init cpu_psci_cpu_prepare(unsigned int cpu)
static int cpu_psci_cpu_boot(unsigned int cpu)
{
- phys_addr_t pa_secondary_entry = __pa_symbol(function_nocfi(secondary_entry));
+ phys_addr_t pa_secondary_entry = __pa_symbol(secondary_entry);
int err = psci_ops.cpu_on(cpu_logical_map(cpu), pa_secondary_entry);
if (err)
pr_err("failed to boot CPU%d (%d)\n", cpu, err);
diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c
index 7e1624ecab3c..49029eace3ad 100644
--- a/arch/arm64/kernel/smp_spin_table.c
+++ b/arch/arm64/kernel/smp_spin_table.c
@@ -66,7 +66,7 @@ static int smp_spin_table_cpu_init(unsigned int cpu)
static int smp_spin_table_cpu_prepare(unsigned int cpu)
{
__le64 __iomem *release_addr;
- phys_addr_t pa_holding_pen = __pa_symbol(function_nocfi(secondary_holding_pen));
+ phys_addr_t pa_holding_pen = __pa_symbol(secondary_holding_pen);
if (!cpu_release_addr[cpu])
return -ENODEV;
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index cfb448eabdaa..aa3133cafced 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -334,7 +334,7 @@ static int __init psci_features(u32 psci_func_id)
static int psci_suspend_finisher(unsigned long state)
{
u32 power_state = state;
- phys_addr_t pa_cpu_resume = __pa_symbol(function_nocfi(cpu_resume));
+ phys_addr_t pa_cpu_resume = __pa_symbol(cpu_resume);
return psci_ops.cpu_suspend(power_state, pa_cpu_resume);
}
@@ -359,7 +359,7 @@ int psci_cpu_suspend_enter(u32 state)
static int psci_system_suspend(unsigned long unused)
{
- phys_addr_t pa_cpu_resume = __pa_symbol(function_nocfi(cpu_resume));
+ phys_addr_t pa_cpu_resume = __pa_symbol(cpu_resume);
return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
pa_cpu_resume, 0, 0);
diff --git a/drivers/misc/lkdtm/usercopy.c b/drivers/misc/lkdtm/usercopy.c
index 9161ce7ed47a..79a17b1c4885 100644
--- a/drivers/misc/lkdtm/usercopy.c
+++ b/drivers/misc/lkdtm/usercopy.c
@@ -318,7 +318,7 @@ void lkdtm_USERCOPY_KERNEL(void)
pr_info("attempting bad copy_to_user from kernel text: %px\n",
vm_mmap);
- if (copy_to_user((void __user *)user_addr, function_nocfi(vm_mmap),
+ if (copy_to_user((void __user *)user_addr, vm_mmap,
unconst + PAGE_SIZE)) {
pr_warn("copy_to_user failed, but lacked Oops\n");
goto free_user;
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 9303f5fe5d89..80ed9644d129 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -203,16 +203,6 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
__v; \
})
-/*
- * With CONFIG_CFI_CLANG, the compiler replaces function addresses in
- * instrumented C code with jump table addresses. Architectures that
- * support CFI can define this macro to return the actual function address
- * when needed.
- */
-#ifndef function_nocfi
-#define function_nocfi(x) (x)
-#endif
-
#endif /* __KERNEL__ */
/*
--
2.36.0.464.gb9c8b46e94-goog
CC_FLAGS_LTO no longer includes CC_FLAGS_CFI, so filter these flags
out as well.
Signed-off-by: Sami Tolvanen <[email protected]>
---
arch/x86/entry/vdso/Makefile | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
index 693f8b9031fb..abf41ef0f89e 100644
--- a/arch/x86/entry/vdso/Makefile
+++ b/arch/x86/entry/vdso/Makefile
@@ -91,7 +91,7 @@ ifneq ($(RETPOLINE_VDSO_CFLAGS),)
endif
endif
-$(vobjs): KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO) $(GCC_PLUGINS_CFLAGS) $(RETPOLINE_CFLAGS),$(KBUILD_CFLAGS)) $(CFL)
+$(vobjs): KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO) $(CC_FLAGS_CFI) $(GCC_PLUGINS_CFLAGS) $(RETPOLINE_CFLAGS),$(KBUILD_CFLAGS)) $(CFL)
#
# vDSO code runs in userspace and -pg doesn't help with profiling anyway.
@@ -151,6 +151,7 @@ KBUILD_CFLAGS_32 := $(filter-out -mfentry,$(KBUILD_CFLAGS_32))
KBUILD_CFLAGS_32 := $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS_32))
KBUILD_CFLAGS_32 := $(filter-out $(RETPOLINE_CFLAGS),$(KBUILD_CFLAGS_32))
KBUILD_CFLAGS_32 := $(filter-out $(CC_FLAGS_LTO),$(KBUILD_CFLAGS_32))
+KBUILD_CFLAGS_32 := $(filter-out $(CC_FLAGS_CFI),$(KBUILD_CFLAGS_32))
KBUILD_CFLAGS_32 += -m32 -msoft-float -mregparm=0 -fpic
KBUILD_CFLAGS_32 += -fno-stack-protector
KBUILD_CFLAGS_32 += $(call cc-option, -foptimize-sibling-calls)
--
2.36.0.464.gb9c8b46e94-goog
Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro.
Signed-off-by: Sami Tolvanen <[email protected]>
Reviewed-by: Nick Desaulniers <[email protected]>
Tested-by: Nick Desaulniers <[email protected]>
Tested-by: Sedat Dilek <[email protected]>
---
arch/x86/purgatory/Makefile | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index ae53d54d7959..b3fa947fa38b 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -55,6 +55,10 @@ ifdef CONFIG_RETPOLINE
PURGATORY_CFLAGS_REMOVE += $(RETPOLINE_CFLAGS)
endif
+ifdef CONFIG_CFI_CLANG
+PURGATORY_CFLAGS_REMOVE += $(CC_FLAGS_CFI)
+endif
+
CFLAGS_REMOVE_purgatory.o += $(PURGATORY_CFLAGS_REMOVE)
CFLAGS_purgatory.o += $(PURGATORY_CFLAGS)
--
2.36.0.464.gb9c8b46e94-goog
On Tue, May 03, 2022 at 03:35:34PM -0700, Peter Collingbourne wrote:
> On Mon, May 2, 2022 at 1:02 PM Peter Zijlstra <[email protected]> wrote:
> >
> > On Mon, May 02, 2022 at 08:22:57AM -0700, Sami Tolvanen wrote:
> >
> > > > Anyway, I think I hate that __builtin, I'd *much* rather see a variable
> > > > attribute or qualifier for this, such that one can mark a function
> > > > pointer as not doing CFI.
> > > >
> > > > I simply doesn't make sense to have a builtin that operates on an
> > > > expression. The whole thing is about indirect calls, IOW function
> > > > pointers.
> > >
> > > I also thought an attribute would be more convenient, but the compiler
> > > folks prefer a built-in:
> > >
> > > https://reviews.llvm.org/D122673
> >
> > That seems to mostly worry about C++ things (overload sets, template
> > specialization, name mangling) we kernel folks don't seem to much care
> > about.
> >
> > I'll stick with saying type system makes more sense to me though.
>
> I'd say it's not only the C++ issues but more the "action at a
> distance" that's implied by having this be part of the type system.
> With this being in the function type it's hard to tell whether any
> particular call will have CFI disabled, without needing to go and look
> at how the function pointer is defined.
Look at how we use volatile:
*(volatile int *)(&foo)
we don't use volatile on actual variable definitions (much), but instead
cast it in at the usage site. Same can be done with this if so desired.
> On the other hand, if we
> explicitly mark up the calls with CFI disabled, the code becomes
> easier to audit (think Rust "unsafe" blocks).
I don't know any Rust. To me Rust still looks like line noise.
> Does it seem any better to you to have this be marked up via the
> function expression, rather than the call? The idea is that this would
> always compile to a check-free function call, no matter what "func"
> is:
>
> __builtin_kcfi_call_unchecked(func)(args)
>
> We already have this, to some degree, with KCFI as implemented: CFI
> checks are disabled if the function expression refers to a declared
> function. The builtin would allow overriding the decision to also
> disable CFI checks for function expressions that use the builtin. It
> also wouldn't preclude a type based system later on (the builtin would
> become effectively a cast to the "unchecked" type).
That's still a bit naf; you've effectively made that builtin a type-cast.
On Wed, May 4, 2022 at 9:41 AM Sami Tolvanen <[email protected]> wrote:
>
> Hi Mark,
>
> On Wed, May 4, 2022 at 9:18 AM Mark Rutland <[email protected]> wrote:
> > I wanted to give this a spin on arm64, but I'm seeing some very odd toolchain
> > behaviour. I'm not sure if I've done something wrong, or if I'm just hitting an
> > edge-case, but it looks like using -fsanitize=kcfi causes the toolchain to hit
> > out-of-memory errors and other issues which look like they could be memory
> > corruption.
>
> Thanks for the detailed bug report! It definitely looks like something
> is wrong with the recent switch from std::string to Twine in the Clang
> code. I didn't see this issue when compiling the arm64 kernel, but
> I'll take a closer look and see if I can reproduce it.
I was able to reproduce this by turning off assertions in Clang. It
seems to work fine with -DLLVM_ENABLE_ASSERTIONS=ON. I'll go fix.
Sami
Hi Mark,
On Wed, May 4, 2022 at 9:18 AM Mark Rutland <[email protected]> wrote:
> I wanted to give this a spin on arm64, but I'm seeing some very odd toolchain
> behaviour. I'm not sure if I've done something wrong, or if I'm just hitting an
> edge-case, but it looks like using -fsanitize=kcfi causes the toolchain to hit
> out-of-memory errors and other issues which look like they could be memory
> corruption.
Thanks for the detailed bug report! It definitely looks like something
is wrong with the recent switch from std::string to Twine in the Clang
code. I didn't see this issue when compiling the arm64 kernel, but
I'll take a closer look and see if I can reproduce it.
Sami
Hi Sami,
On Fri, Apr 29, 2022 at 01:36:23PM -0700, Sami Tolvanen wrote:
> KCFI is a proposed forward-edge control-flow integrity scheme for
> Clang, which is more suitable for kernel use than the existing CFI
> scheme used by CONFIG_CFI_CLANG. KCFI doesn't require LTO, doesn't
> alter function references to point to a jump table, and won't break
> function address equality. The latest LLVM patches are here:
>
> https://reviews.llvm.org/D119296
> https://reviews.llvm.org/D124211
This is really exciting to see!
I wanted to give this a spin on arm64, but I'm seeing some very odd toolchain
behaviour. I'm not sure if I've done something wrong, or if I'm just hitting an
edge-case, but it looks like using -fsanitize=kcfi causes the toolchain to hit
out-of-memory errors and other issues which look like they could be memory
corruption.
Setup-wise:
* My build machine is a "Intel(R) Xeon(R) CPU E5-2660 v4" with 56 HW threads
and 64GB of RAM, running x86_64 Debian 11.3.
* I applied D119296 atop LLVM commit 11d3e31c60bd (per the "Parents" part of
the Revision Contents on https://reviews.llvm.org/D119296), and built that
with:
cmake -S llvm -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS='clang;lld
cmake --build build
Aside: I'll go build a Debug release to compare this to.
* I applied this series atop v5.18-rc4.
* I normally build with -j50, and LLVM=1.
Aside from a single ifdef issue in compiler-clang.h, defconfig builds cleanly,
but defconfig + CONFIG_CFI_CLANG produces lots of out of memory errors and some
other errors which look erroneous. I see a bunch of errors even when I
significantly reduce my build parallelism (e.g. down to -j10, a reduction of
5x).
Some of these don't look right at all, e.g.
| make: *** [Makefile:1823: fs] Error 2
| ^[^[<inline asm>:5:1: error: unexpected token at start of statement
| 93825275602704
| ^
| 1 error generated.
| make[2]: *** [scripts/Makefile.build:289: arch/arm64/kernel/suspend.o] Error 1
| make[2]: *** Waiting for unfinished jobs....
| make[1]: *** [scripts/Makefile.build:551: arch/arm64/kernel] Error 2
| <inline asm>:5:1: error: unexpected token at start of statement
| @<U+001D><U+001A>8DV
| ^
| 1 error generated.
| make[3]: *** [scripts/Makefile.build:289: drivers/phy/amlogic/phy-meson8b-usb2.o] Error 1
| make[3]: *** Waiting for unfinished jobs....
| make[2]: *** [scripts/Makefile.build:551: drivers/phy/amlogic] Error 2
| make[2]: *** Waiting for unfinished jobs....
| make[1]: *** [scripts/Makefile.build:551: kernel/sched] Error 2
| make: *** [Makefile:1823: kernel] Error 2
| make: *** Waiting for unfinished jobs....
... maybe those are due to memory corruption / bad out-of-memory handling?
Some are out-of-memory errors:
| LLVM ERROR: out of memory
| Allocation failed
| PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
| Stack dump:
| 0. Program arguments: clang -Wp,-MMD,kernel/dma/.pool.o.d -nostdinc -I./arch/arm64/include -I./arch/arm64/include/generated -I./include -I./arch/arm64/include/uapi -I./arch/arm64/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -mlittle-endian -DKASAN_SHADOW_SCALE_SHIFT= -Qunused-arguments -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu11 --target=aarch64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -mgeneral-regs-only -DCONFIG_CC_HAS_K_CONSTRAINT=1 -Wno-psabi -fno-asynchronous-unwind-tables -fno-unwind-tables -mbranch-protection=pac-ret+leaf+bti -Wa,-march=armv8.5-a -DARM64_ASM_ARCH=\"armv8.5-a\" -DKASAN_SHADOW_SCALE_SHIFT= -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector-strong -Wimplicit-fallthrough -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang -fno-stack-clash-protection -fsanitize=kcfi -fno-sanitize-blacklist -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -fno-strict-overflow -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-format-zero-length -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access -mstack-protector-guard=sysreg -mstack-protector-guard-reg=sp_el0 -mstack-protector-guard-offset=1184 -DKBUILD_MODFILE=\"kernel/dma/pool\" -DKBUILD_BASENAME=\"pool\" -DKBUILD_MODNAME=\"pool\" -D__KBUILD_MODNAME=kmod_pool -c -o kernel/dma/pool.o kernel/dma/pool.c
| 1. <eof> parser at end of file
| 2. Per-file LLVM IR generation
| #0 0x00005559ef670830 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
| #1 0x00005559ef66e6e4 llvm::sys::CleanupOnSignal(unsigned long) (/home/mark/src/llvm-project/build/bin/clang-15+0x36136e4)
| #2 0x00005559ef5ab3f8 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
| #3 0x00007f5ac3547140 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14140)
| #4 0x00007f5ac302ace1 raise (/lib/x86_64-linux-gnu/libc.so.6+0x3bce1)
| #5 0x00007f5ac3014537 abort (/lib/x86_64-linux-gnu/libc.so.6+0x25537)
| #6 0x00005559ef5b2389 (/home/mark/src/llvm-project/build/bin/clang-15+0x3557389)
| #7 0x00005559ef5e00f7 (/home/mark/src/llvm-project/build/bin/clang-15+0x35850f7)
| #8 0x00005559ef641191 llvm::raw_svector_ostream::write_impl(char const*, unsigned long) (/home/mark/src/llvm-project/build/bin/clang-15+0x35e6191)
| #9 0x00005559ef64325e llvm::raw_ostream::write(char const*, unsigned long) (/home/mark/src/llvm-project/build/bin/clang-15+0x35e825e)
| #10 0x00005559ef611dae llvm::Twine::str[abi:cxx11]() const (/home/mark/src/llvm-project/build/bin/clang-15+0x35b6dae)
| #11 0x00005559efac97be clang::CodeGen::CodeGenModule::FinalizeKCFITypePrefixes() (/home/mark/src/llvm-project/build/bin/clang-15+0x3a6e7be)
| #12 0x00005559efafd53c clang::CodeGen::CodeGenModule::Release() (/home/mark/src/llvm-project/build/bin/clang-15+0x3aa253c)
| #13 0x00005559f07564aa (anonymous namespace)::CodeGeneratorImpl::HandleTranslationUnit(clang::ASTContext&) ModuleBuilder.cpp:0:0
| #14 0x00005559f07543e5 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/home/mark/src/llvm-project/build/bin/clang-15+0x46f93e5)
| #15 0x00005559f11b85a9 clang::ParseAST(clang::Sema&, bool, bool) (/home/mark/src/llvm-project/build/bin/clang-15+0x515d5a9)
| #16 0x00005559f00cf419 clang::FrontendAction::Execute() (/home/mark/src/llvm-project/build/bin/clang-15+0x4074419)
| #17 0x00005559f005a85b clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/home/mark/src/llvm-project/build/bin/clang-15+0x3fff85b)
| #18 0x00005559f0183860 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/home/mark/src/llvm-project/build/bin/clang-15+0x4128860)
| #19 0x00005559ed0f051c cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/home/mark/src/llvm-project/build/bin/clang-15+0x109551c)
| #20 0x00005559ed0ed3f9 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) driver.cpp:0:0
| #21 0x00005559efed5fa5 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const::'lambda'()>(long) Job.cpp:0:0
| #22 0x00005559ef5ab4f3 llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (/home/mark/src/llvm-project/build/bin/clang-15+0x35504f3)
| #23 0x00005559efed6304 clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const (.part.0) Job.cpp:0:0
| #24 0x00005559efea7b36 clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&) const (/home/mark/src/llvm-project/build/bin/clang-15+0x3e4cb36)
| #25 0x00005559efea84e9 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) const (/home/mark/src/llvm-project/build/bin/clang-15+0x3e4d4e9)
| #26 0x00005559efeb6619 clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) (/home/mark/src/llvm-project/build/bin/clang-15+0x3e5b619)
| #27 0x00005559ed033793 main (/home/mark/src/llvm-project/build/bin/clang-15+0xfd8793)
| #28 0x00007f5ac3015d0a __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26d0a)
| #29 0x00005559ed0ecdaa _start (/home/mark/src/llvm-project/build/bin/clang-15+0x1091daa)
| clang-15: error: clang frontend command failed with exit code 134 (use -v to see invocation)
| clang version 15.0.0 (https://github.com/llvm/llvm-project.git 1e3994ce3cd7b217678edd589392c3c3c1575880)
| Target: aarch64-unknown-linux-gnu
| Thread model: posix
| InstalledDir: /home/mark/src/llvm-project/build/bin
| clang-15: note: diagnostic msg:
| ********************
|
| PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
| Preprocessed source(s) and associated run script(s) are located at:
| clang-15: note: diagnostic msg: /tmp/pool-b4bab3.c
| clang-15: note: diagnostic msg: /tmp/pool-b4bab3.sh
| clang-15: note: diagnostic msg:
|
| ********************
| make[2]: *** [scripts/Makefile.build:289: kernel/dma/pool.o] Error 134
| make[1]: *** [scripts/Makefile.build:551: kernel/dma] Error 2
| make[1]: *** Waiting for unfinished jobs....
Note: I've kept those files, but as the c file is 3.9M I have not included that here.
There appar to be other failures too:
| [mark@lakrids:~/src/linux]% PATH=/home/mark/src/llvm-project/build/bin/:$PATH make LLVM=1 ARCH=arm64 -j10 Image -s
| PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
| Stack dump:
| 0. Program arguments: clang -Wp,-MMD,mm/.util.o.d -nostdinc -I./arch/arm64/include -I./arch/arm64/include/generated -I./include -I./arch/arm64/include/uapi -I./arch/arm64/include/generated/uapi -I./i
| nclude/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -mlittle-endian -DKASAN_SHADOW_SCALE_SHIFT= -Qunused-arguments -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu11 --target=aarch64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -mgeneral-regs-only -DCONFIG_CC_HAS_K_CONSTRAINT=1 -Wno-psabi -fno-asynchronous-unwind-tables -fno-unwind-tables -mbranch-protection=pac-ret+leaf+bti -Wa,-march=armv8.5-a -DARM64_ASM_ARCH=\"armv8.5-a\" -DKASAN_SHADOW_SCALE_SHIFT= -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector-strong -Wimplicit-fallthrough -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang -fno-stack-clash-protection -fsanitize=kcfi -fno-sanitize-blacklist -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -fno-strict-overflow -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-format-zero-length -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access -mstack-protector-guard=sysreg -mstack-protector-guard-reg=sp_el0 -mstack-protector-guard-offset=1184 -DKBUILD_MODFILE=\"mm/util\" -DKBUILD_BASENAME=\"util\" -DKBUILD_MODNAME=\"util\" -D__KBUILD_MODNAME=kmod_util -c -o mm/util.o mm/util.c
| 1. <eof> parser at end of file
| 2. Per-file LLVM IR generation
| #0 0x0000559484667830 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
| #1 0x00005594846656e4 llvm::sys::CleanupOnSignal(unsigned long) (/home/mark/src/llvm-project/build/bin/clang-15+0x36136e4)
| #2 0x00005594845a23f8 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
| #3 0x00007f490bbd1140 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14140)
| #4 0x0000559484608ca8 llvm::Twine::printOneChild(llvm::raw_ostream&, llvm::Twine::Child, llvm::Twine::NodeKind) const (/home/mark/src/llvm-project/build/bin/clang-15+0x35b6ca8)
| #5 0x0000559484608dae llvm::Twine::str[abi:cxx11]() const (/home/mark/src/llvm-project/build/bin/clang-15+0x35b6dae)
| #6 0x0000559484ac07be clang::CodeGen::CodeGenModule::FinalizeKCFITypePrefixes() (/home/mark/src/llvm-project/build/bin/clang-15+0x3a6e7be)
| #7 0x0000559484af453c clang::CodeGen::CodeGenModule::Release() (/home/mark/src/llvm-project/build/bin/clang-15+0x3aa253c)
| #8 0x000055948574d4aa (anonymous namespace)::CodeGeneratorImpl::HandleTranslationUnit(clang::ASTContext&) ModuleBuilder.cpp:0:0
| #9 0x000055948574b3e5 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/home/mark/src/llvm-project/build/bin/clang-15+0x46f93e5)
| #10 0x00005594861af5a9 clang::ParseAST(clang::Sema&, bool, bool) (/home/mark/src/llvm-project/build/bin/clang-15+0x515d5a9)
| #11 0x00005594850c6419 clang::FrontendAction::Execute() (/home/mark/src/llvm-project/build/bin/clang-15+0x4074419)
| #12 0x000055948505185b clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/home/mark/src/llvm-project/build/bin/clang-15+0x3fff85b)
| #13 0x000055948517a860 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/home/mark/src/llvm-project/build/bin/clang-15+0x4128860)
| #14 0x00005594820e751c cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/home/mark/src/llvm-project/build/bin/clang-15+0x109551c)
| #15 0x00005594820e43f9 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) driver.cpp:0:0
| #16 0x0000559484eccfa5 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const::'lambda'()>(long) Job.cpp:0:0
| #17 0x00005594845a24f3 llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (/home/mark/src/llvm-project/build/bin/clang-15+0x35504f3)
| #18 0x0000559484ecd304 clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const (.part.0) Job.cpp:0:0
| #19 0x0000559484e9eb36 clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&) const (/home/mark/src/llvm-project/build/bin/clang-15+0x3e4cb36)
| #20 0x0000559484e9f4e9 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) const (/home/mark/src/llvm-project/build/bin/clang-15+0x3e4d4e9)
| #21 0x0000559484ead619 clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) (/home/mark/src/llvm-project/build/bin/clang-15+0x3e5b619)
| #22 0x000055948202a793 main (/home/mark/src/llvm-project/build/bin/clang-15+0xfd8793)
| #23 0x00007f490b69fd0a __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26d0a)
| #24 0x00005594820e3daa _start (/home/mark/src/llvm-project/build/bin/clang-15+0x1091daa)
| clang-15: error: clang frontend command failed with exit code 135 (use -v to see invocation)
| clang version 15.0.0 (https://github.com/llvm/llvm-project.git 1e3994ce3cd7b217678edd589392c3c3c1575880)
| Target: aarch64-unknown-linux-gnu
| Thread model: posix
| InstalledDir: /home/mark/src/llvm-project/build/bin
| clang-15: note: diagnostic msg:
| ********************
|
| PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
| Preprocessed source(s) and associated run script(s) are located at:
| clang-15: note: diagnostic msg: /tmp/util-30a0f2.c
| clang-15: note: diagnostic msg: /tmp/util-30a0f2.sh
| clang-15: note: diagnostic msg:
|
| ********************
Note: I've saved those files for now, but the c file is 4.8M, so I haven't included it
inline or attached it here.
Thanks,
Mark.
On Wed, May 04, 2022 at 01:17:25PM -0700, Sami Tolvanen wrote:
> On Wed, May 4, 2022 at 9:41 AM Sami Tolvanen <[email protected]> wrote:
> >
> > Hi Mark,
> >
> > On Wed, May 4, 2022 at 9:18 AM Mark Rutland <[email protected]> wrote:
> > > I wanted to give this a spin on arm64, but I'm seeing some very odd toolchain
> > > behaviour. I'm not sure if I've done something wrong, or if I'm just hitting an
> > > edge-case, but it looks like using -fsanitize=kcfi causes the toolchain to hit
> > > out-of-memory errors and other issues which look like they could be memory
> > > corruption.
> >
> > Thanks for the detailed bug report! It definitely looks like something
> > is wrong with the recent switch from std::string to Twine in the Clang
> > code. I didn't see this issue when compiling the arm64 kernel, but
> > I'll take a closer look and see if I can reproduce it.
>
> I was able to reproduce this by turning off assertions in Clang. It
> seems to work fine with -DLLVM_ENABLE_ASSERTIONS=ON. I'll go fix.
FWIW, a `-DLLVM_ENABLE_ASSERTIONS=ON` build also seems to work for me when
building a kernel with CONFIG_CFI_CLANG=y. It's much slower than a regular
Release build, so I'm still waiting for that to finish building a kernel, but
it has gotten much further through the build without issues.
Thanks,
Mark.
On Thu, May 5, 2022 at 9:30 AM Mark Rutland <[email protected]> wrote:
> I also believe that in most cases we can drop the __nocfi annotation on callers
> now that we can mark the called assembly function with SYM_TYPED_FUNC_START().
Good point, thanks for pointing that out. I'll add these to the next
version of the series.
> There' a latent bug here with the existing CFI scheme, since
> `kpti_install_ng_mappings` isn't marked with __nocfi, and should explode when
> calling `idmap_kpti_install_ng_mappings` via the idmap.
The CONFIG_UNMAP_KERNEL_AT_EL0 version of kpti_install_ng_mappings is
marked __nocfi
> There' a latent bug here with the existing CFI scheme, since
> `machine_kexec` isn't marked with __nocfi, and should explode when calling
> `cpu_soft_restart` via the idmap.
But it's indeed missing from this one.
Sami
On Thu, May 5, 2022 at 5:36 AM Mark Rutland <[email protected]> wrote:
> FWIW, a `-DLLVM_ENABLE_ASSERTIONS=ON` build also seems to work for me when
> building a kernel with CONFIG_CFI_CLANG=y. It's much slower than a regular
> Release build, so I'm still waiting for that to finish building a kernel, but
> it has gotten much further through the build without issues.
Thanks for confirming. This issue should be fixed here if you want to
give it another try:
https://github.com/samitolvanen/llvm-project/commits/kcfi
Sami
On Fri, Apr 29, 2022 at 01:36:33PM -0700, Sami Tolvanen wrote:
> With -fsanitize=kcfi, we no longer need function_nocfi() as
> the compiler won't change function references to point to a
> jump table. Remove all implementations and uses of the macro.
>
> Signed-off-by: Sami Tolvanen <[email protected]>
> ---
> arch/arm64/include/asm/compiler.h | 16 ----------------
> arch/arm64/include/asm/ftrace.h | 2 +-
> arch/arm64/include/asm/mmu_context.h | 2 +-
> arch/arm64/kernel/acpi_parking_protocol.c | 2 +-
> arch/arm64/kernel/cpufeature.c | 2 +-
> arch/arm64/kernel/ftrace.c | 2 +-
> arch/arm64/kernel/machine_kexec.c | 2 +-
> arch/arm64/kernel/psci.c | 2 +-
> arch/arm64/kernel/smp_spin_table.c | 2 +-
> drivers/firmware/psci/psci.c | 4 ++--
> drivers/misc/lkdtm/usercopy.c | 2 +-
> include/linux/compiler.h | 10 ----------
> 12 files changed, 11 insertions(+), 37 deletions(-)
Nice!
I also believe that in most cases we can drop the __nocfi annotation on callers
now that we can mark the called assembly function with SYM_TYPED_FUNC_START().
In most cases we needed the __nocfi annotation on a caller because it was
invoking an assembly function at an unusual virtual address (which differed
from the link address), and the existing CFI scheme couldn't handle that. The
kCFI scheme should handle that fine so long as the type ID before the function
is accessible.
The other odd case was where we had the non-cfi address of a target function
(e.g. for callback structures populated in assembly), and that doesn't matter
with kCFI.
In looking at the below I spotted some latent issues. I'll prepare some patches
for those.
> diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
> index dc3ea4080e2e..6fb2e6bcc392 100644
> --- a/arch/arm64/include/asm/compiler.h
> +++ b/arch/arm64/include/asm/compiler.h
> @@ -23,20 +23,4 @@
> #define __builtin_return_address(val) \
> (void *)(ptrauth_clear_pac((unsigned long)__builtin_return_address(val)))
>
> -#ifdef CONFIG_CFI_CLANG
> -/*
> - * With CONFIG_CFI_CLANG, the compiler replaces function address
> - * references with the address of the function's CFI jump table
> - * entry. The function_nocfi macro always returns the address of the
> - * actual function instead.
> - */
> -#define function_nocfi(x) ({ \
> - void *addr; \
> - asm("adrp %0, " __stringify(x) "\n\t" \
> - "add %0, %0, :lo12:" __stringify(x) \
> - : "=r" (addr)); \
> - addr; \
> -})
> -#endif
> -
> #endif /* __ASM_COMPILER_H */
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 1494cfa8639b..c96d47cb8f46 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -26,7 +26,7 @@
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> #define ARCH_SUPPORTS_FTRACE_OPS 1
> #else
> -#define MCOUNT_ADDR ((unsigned long)function_nocfi(_mcount))
> +#define MCOUNT_ADDR ((unsigned long)_mcount)
> #endif
>
> /* The BL at the callsite's adjusted rec->ip */
> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> index 6770667b34a3..c9df5ab2c448 100644
> --- a/arch/arm64/include/asm/mmu_context.h
> +++ b/arch/arm64/include/asm/mmu_context.h
> @@ -164,7 +164,7 @@ static inline void __nocfi cpu_replace_ttbr1(pgd_t *pgdp)
> ttbr1 |= TTBR_CNP_BIT;
> }
>
> - replace_phys = (void *)__pa_symbol(function_nocfi(idmap_cpu_replace_ttbr1));
> + replace_phys = (void *)__pa_symbol(idmap_cpu_replace_ttbr1);
>
> cpu_install_idmap();
> replace_phys(ttbr1);
As long as we create `idmap_cpu_replace_ttbr1` with SYM_TYPED_FUNC_START(), we
can drop `__nocfi` from `cpu_replace_ttbr1`
[...]
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index d72c4b4d389c..dae07d99508b 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1619,7 +1619,7 @@ kpti_install_ng_mappings(const struct arm64_cpu_capabilities *__unused)
> if (arm64_use_ng_mappings)
> return;
>
> - remap_fn = (void *)__pa_symbol(function_nocfi(idmap_kpti_install_ng_mappings));
> + remap_fn = (void *)__pa_symbol(idmap_kpti_install_ng_mappings);
>
> cpu_install_idmap();
> remap_fn(cpu, num_online_cpus(), __pa_symbol(swapper_pg_dir));
There' a latent bug here with the existing CFI scheme, since
`kpti_install_ng_mappings` isn't marked with __nocfi, and should explode when
calling `idmap_kpti_install_ng_mappings` via the idmap.
With the kCFI scheme we instead need to mark `idmap_kpti_install_ng_mappings`
with SYM_TYPED_FUNC_START().
[...]
> diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
> index e16b248699d5..4eb5388aa5a6 100644
> --- a/arch/arm64/kernel/machine_kexec.c
> +++ b/arch/arm64/kernel/machine_kexec.c
> @@ -204,7 +204,7 @@ void machine_kexec(struct kimage *kimage)
> typeof(cpu_soft_restart) *restart;
>
> cpu_install_idmap();
> - restart = (void *)__pa_symbol(function_nocfi(cpu_soft_restart));
> + restart = (void *)__pa_symbol(cpu_soft_restart);
> restart(is_hyp_nvhe(), kimage->start, kimage->arch.dtb_mem,
> 0, 0);
> } else {
There' a latent bug here with the existing CFI scheme, since
`machine_kexec` isn't marked with __nocfi, and should explode when calling
`cpu_soft_restart` via the idmap.
With the kCFI scheme we instead need to mark `cpu_soft_restart` with
SYM_TYPED_FUNC_START(). It's currently marked as SYM_CODE() because it doesn't
follow the usual function call conventions, but that also means it's broken for
BTI, and for now (without something like objtool caring about function calling
conventions) SYM_FUNC_START() is fine.
Thanks,
Mark.
On Thu, May 05, 2022 at 09:00:39AM -0700, Sami Tolvanen wrote:
> On Thu, May 5, 2022 at 5:36 AM Mark Rutland <[email protected]> wrote:
> > FWIW, a `-DLLVM_ENABLE_ASSERTIONS=ON` build also seems to work for me when
> > building a kernel with CONFIG_CFI_CLANG=y. It's much slower than a regular
> > Release build, so I'm still waiting for that to finish building a kernel, but
> > it has gotten much further through the build without issues.
>
> Thanks for confirming. This issue should be fixed here if you want to
> give it another try:
>
> https://github.com/samitolvanen/llvm-project/commits/kcfi
That works for me, building in Release mode. A defconfig + CFI_CLANG kernel
built with that builds and boots clenaly.
Thanks!
Mark.
On Thu, May 05, 2022 at 09:51:39AM -0700, Sami Tolvanen wrote:
> On Thu, May 5, 2022 at 9:30 AM Mark Rutland <[email protected]> wrote:
> > I also believe that in most cases we can drop the __nocfi annotation on callers
> > now that we can mark the called assembly function with SYM_TYPED_FUNC_START().
>
> Good point, thanks for pointing that out. I'll add these to the next
> version of the series.
Also, I *think* we can drop __nocfi from __init, and always check calls to
functions in .init.text. IIUC we made those __nocfi because it leads to section
mismatches, and dangling entries in the jump tables after we discarded the init
text, neither of which should be a problem with kCFI.
Unfortuantely, that appears to be masking some existing type mismatches; e.g.
psci_dt_init() blows up because it uses the wrong type for its callees (a
mismatched `const`). With that fixed up, arm64 boots fine.
> > There' a latent bug here with the existing CFI scheme, since
> > `kpti_install_ng_mappings` isn't marked with __nocfi, and should explode when
> > calling `idmap_kpti_install_ng_mappings` via the idmap.
>
> The CONFIG_UNMAP_KERNEL_AT_EL0 version of kpti_install_ng_mappings is
> marked __nocfi
Ah, so it is. Sorry for the noise!
> > There' a latent bug here with the existing CFI scheme, since
> > `machine_kexec` isn't marked with __nocfi, and should explode when calling
> > `cpu_soft_restart` via the idmap.
>
> But it's indeed missing from this one.
Cool; I'll prep a patch that fixes just this, then.
Thanks,
Mark.