| Message ID | 20220429203644.2868448-11-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | KCFI support | expand |
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 <samitolvanen@google.com> > --- > 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 5, 2022 at 9:30 AM Mark Rutland <mark.rutland@arm.com> 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 05, 2022 at 09:51:39AM -0700, Sami Tolvanen wrote: > On Thu, May 5, 2022 at 9:30 AM Mark Rutland <mark.rutland@arm.com> 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.
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__ */ /*
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 <samitolvanen@google.com> --- 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(-)