arm64: ftrace: fix interop issues with module PLTs
v3 cover arm64: ftrace: fix interop issues with module PLTs
The arm64 ftrace code assumes that all core kernel symbols are within the range of an ordinary branch instruction from anywhere in the kernel, even in modules. While this is usually the case, it may not be true when running with KASLR enabled.
This series fixes two distinct but related issues involving ftrace under KASLR:
- the initial sweep over all _mcount() calls in newly loaded modules validates those calls by comparing each to a 'bl _mcount' opcode, which will fail if _mcount() [which is in the core kernel] is out of range, and which will result in false negatives, given that those call sites will contain a correct branch to _mcount(), but via a PLT entry (patch #1)
- patching up those NOPs into calls to ftrace_caller() [which is the only ftrace branch target that we ever use on arm64, IIUC] will fail in the same way, and needs to be redirected via a trampoline as well (patch #2)
v3:
- perform partial validation on 'bl _mcount' call sites, i.e., perform full validation unless the call site is in a module and is in fact too far away from the branch target, in which case we check that the instruction is a bl instruction pointing somewhere inside the module itself (#1). Note that this will correctly identify the trampolines added in #2 as valid branch targets, with the caveat that the 'mod' parameter is only supplied by the ftrace framework at module load time.
- disable preemption around __module_text_address(): this is not really needed given that ftrace_lock will prevent the module from being unloaded while we are using it, but the code in module.c contains an assert for it. (#1, #2)
- add barrier between updating the trampoline target field and patching the instruction that invokes it (#2)
- refactored the code so it is obviously identical to the old code when CONFIG_ARM64_MODULE_PLTS is disabled. (#1, #2)
v2:
- no changes to #1
- don't hardcode references to ftrace_caller() (#2)
- avoid a stack frame for the trampoline, given that it will confuse the ftrace code (#2)
Ard Biesheuvel (2):
arm64: ftrace: don't validate branch via PLT in ftrace_make_nop()
arm64: ftrace: add support for far branches to dynamic ftrace
arch/arm64/Kconfig | 2 +- arch/arm64/Makefile | 3 + arch/arm64/include/asm/module.h | 3 + arch/arm64/kernel/Makefile | 3 + arch/arm64/kernel/ftrace-mod.S | 18 ++++ arch/arm64/kernel/ftrace.c | 97 +++++++++++++++++++- arch/arm64/kernel/module.c | 6 +- 7 files changed, 127 insertions(+), 5 deletions(-) create mode 100644 arch/arm64/kernel/ftrace-mod.S
Signature · 1 line
2.9.3
PATCH v3 1/2 arm64: ftrace: don't validate branch via PLT in ftrace_make_nop()
When turning branch instructions into NOPs, we attempt to validate the action by comparing the old value at the call site with the opcode of a direct relative branch instruction pointing at the old target.
However, these call sites are statically initialized to call _mcount(), and may be redirected via a PLT entry if the module is loaded far away from the kernel text, leading to false negatives and spurious errors.
So skip the validation if CONFIG_ARM64_MODULE_PLTS is configured.
--- arch/arm64/kernel/ftrace.c | 46 ++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-)
arch/arm64/kernel/ftrace.c +43 -3
@@ -84,12 +84,52 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, unsigned long addr) {unsigned long pc = rec->ip; -u32 old, new; +long offset = (long )pc - (long )addr; +bool validate = true; +u32 old =0 , new; + +if (IS_ENABLED (CONFIG_ARM64_MODULE_PLTS ) && + (offset < -SZ_128M || offset >=SZ_128M )) { +u32 replaced; + +/* +* 'mod' is only set at module load time, but if we end up +* dealing with an out-of-range condition, we can assume it +* is due to a module being loaded far away from the kernel. +*/ +if (!mod) { +preempt_disable (); + mod =__module_text_address (pc); +preempt_enable (); + +if (WARN_ON (!mod)) +return -EINVAL ; + } + +/* +* The instruction we are about to patch may be a branch and +* link instruction that was redirected via a PLT entry. In +* this case, the normal validation will fail, but we can at +* least check that we are dealing with a branch and link +* instruction that points into the right module. +*/ +if (aarch64_insn_read ((void *)pc, &replaced)) +return -EFAULT ; + +if (!aarch64_insn_is_bl (replaced) || + !within_module (pc +aarch64_get_branch_offset (replaced), + mod)) +return -EINVAL ; + + validate = false; + }else { + old =aarch64_insn_gen_branch_imm (pc, addr, +AARCH64_INSN_BRANCH_LINK ); + } - old =aarch64_insn_gen_branch_imm (pc, addr,AARCH64_INSN_BRANCH_LINK ); new =aarch64_insn_gen_nop (); -return ftrace_modify_code (pc, old, new, true); +return ftrace_modify_code (pc, old, new, validate); }void arch_ftrace_update_code (int command)
Signature · 1 line
2.9.3
PATCH v3 1/2 arm64: ftrace: don't validate branch via PLT in ftrace_make_nop()
Quoted 31 lines · Ard Biesheuvel
When turning branch instructions into NOPs, we attempt to validate the action by comparing the old value at the call site with the opcode of a direct relative branch instruction pointing at the old target.
However, these call sites are statically initialized to call
_mcount(), and may be redirected via a PLT entry if the module is loaded far away from the kernel text, leading to false negatives and spurious errors.So skip the validation if
CONFIG_ARM64_MODULE_PLTSis configured.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/kernel/ftrace.c | 46 ++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-)arch/arm64/kernel/ftrace.c +7
-1@@ -84,12 +84,52 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, unsigned long addr) {unsigned long pc = rec->ip; -u32 old, new; +long offset = (long )pc - (long )addr; +bool validate = true; +u32 old =0 , new; + +if (IS_ENABLED (CONFIG_ARM64_MODULE_PLTS ) && + (offset < -SZ_128M || offset >=SZ_128M )) { +u32 replaced;
arch/arm64/kernel/ftrace.c int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, · line 103
Quoted 10 lines
arch/arm64/kernel/ftrace.c +10
-0+ +/* +* 'mod' is only set at module load time, but if we end up +* dealing with an out-of-range condition, we can assume it +* is due to a module being loaded far away from the kernel. +*/ +if (!mod) { +preempt_disable (); + mod =__module_text_address (pc); +preempt_enable ();
The comment in __module_text_address says that preempt must be disabled so that the module doesn't get freed under its feet, but if that's a possibility here then it feels really dangerous to re-enable preemption before we've done the patching. Shouldn't we take module_mutex or something?
Other than that, this looks fine. Thanks.
Will
PATCH v3 1/2 arm64: ftrace: don't validate branch via PLT in ftrace_make_nop()
Quoted 39 lines · Will Deacon
Quoted 38 lines · Ard Biesheuvel
When turning branch instructions into NOPs, we attempt to validate the action by comparing the old value at the call site with the opcode of a direct relative branch instruction pointing at the old target.
However, these call sites are statically initialized to call
_mcount(), and may be redirected via a PLT entry if the module is loaded far away from the kernel text, leading to false negatives and spurious errors.So skip the validation if
CONFIG_ARM64_MODULE_PLTSis configured.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/kernel/ftrace.c | 46 ++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-)arch/arm64/kernel/ftrace.c +14
-1@@ -84,12 +84,52 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, unsigned long addr) {unsigned long pc = rec->ip; -u32 old, new; +long offset = (long )pc - (long )addr; +bool validate = true; +u32 old =0 , new; + +if (IS_ENABLED (CONFIG_ARM64_MODULE_PLTS ) && + (offset < -SZ_128M || offset >=SZ_128M )) { +u32 replaced; + +/* +* 'mod' is only set at module load time, but if we end up +* dealing with an out-of-range condition, we can assume it +* is due to a module being loaded far away from the kernel. +*/ +if (!mod) {
Quoted 9 lines
arch/arm64/kernel/ftrace.c int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, · line 103
Quoted 3 lines
arch/arm64/kernel/ftrace.c +3
-0+preempt_disable (); + mod =__module_text_address (pc); +preempt_enable ();The comment in
__module_text_addresssays that preempt must be disabled so that the module doesn't get freed under its feet, but if that's a possibility here then it feels really dangerous to re-enable preemption before we've done the patching. Shouldn't we takemodule_mutexor something?
Ah yes. I added a comment only in patch #2, on another instance in ftrace_make_call(), and I thought it was redundant to duplicate it
here: ftrace_lock is held here, which will prevent the module from being unloaded in the mean time, so disabling preemption is only necessary to prevent an assert from firing.
Quoted 3 lines
Other than that, this looks fine. Thanks.
Will
PATCH v3 1/2 arm64: ftrace: don't validate branch via PLT in ftrace_make_nop()
Quoted 50 lines · Ard Biesheuvel
Quoted 39 lines · Will Deacon
Quoted 38 lines · Ard Biesheuvel
When turning branch instructions into NOPs, we attempt to validate the action by comparing the old value at the call site with the opcode of a direct relative branch instruction pointing at the old target.
However, these call sites are statically initialized to call
_mcount(), and may be redirected via a PLT entry if the module is loaded far away from the kernel text, leading to false negatives and spurious errors.So skip the validation if
CONFIG_ARM64_MODULE_PLTSis configured.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/kernel/ftrace.c | 46 ++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-)arch/arm64/kernel/ftrace.c +14
-1@@ -84,12 +84,52 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, unsigned long addr) {unsigned long pc = rec->ip; -u32 old, new; +long offset = (long )pc - (long )addr; +bool validate = true; +u32 old =0 , new; + +if (IS_ENABLED (CONFIG_ARM64_MODULE_PLTS ) && + (offset < -SZ_128M || offset >=SZ_128M )) { +u32 replaced; + +/* +* 'mod' is only set at module load time, but if we end up +* dealing with an out-of-range condition, we can assume it +* is due to a module being loaded far away from the kernel. +*/ +if (!mod) {Quoted 9 lines
arch/arm64/kernel/ftrace.c int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, · line 103
Quoted 3 lines
arch/arm64/kernel/ftrace.c +3
-0+preempt_disable (); + mod =__module_text_address (pc); +preempt_enable ();The comment in
__module_text_addresssays that preempt must be disabled so that the module doesn't get freed under its feet, but if that's a possibility here then it feels really dangerous to re-enable preemption before we've done the patching. Shouldn't we takemodule_mutexor something?
Quoted 5 lines
Ah yes. I added a comment only in patch #2, on another instance in
ftrace_make_call(), and I thought it was redundant to duplicate it
here:ftrace_lockis held here, which will prevent the module from being unloaded in the mean time, so disabling preemption is only necessary to prevent an assert from firing.
I suppose !lockdep_is_held(&ftrace_lock) should be added to the WARN_ON_ONCE in module_assert_mutex_or_preempt, but that's a separate patch so I'll queue these as-is.
Thanks for the quick explanation!
Will
PATCH v3 2/2 arm64: ftrace: add support for far branches to dynamic ftrace
In reply to Ard BiesheuvelCurrently, dynamic ftrace support in the arm64 kernel assumes that all core kernel code is within range of ordinary branch instructions that occur in module code, which is usually the case, but is no longer guaranteed now that we have support for module PLTs and address space randomization.
Since on arm64, all patching of branch instructions involves function calls to the same entry point [ftrace_caller()], we can emit the modules with a trampoline that has unlimited range, and patch both the trampoline itself and the branch instruction to redirect the call via the trampoline.
--- arch/arm64/Kconfig | 2 +- arch/arm64/Makefile | 3 ++ arch/arm64/include/asm/module.h | 3 ++ arch/arm64/kernel/Makefile | 3 ++ arch/arm64/kernel/ftrace-mod.S | 18 +++++++ arch/arm64/kernel/ftrace.c | 51 ++++++++++++++++++++ arch/arm64/kernel/module.c | 6 ++- 7 files changed, 84 insertions(+), 2 deletions(-)
arch/arm64/Kconfig +1 -1
@@ -982,7 +982,7 @@ config RANDOMIZE_BASE config RANDOMIZE_MODULE_REGION_FULL bool "Randomize the module region independently from the core kernel" - depends on RANDOMIZE_BASE && !DYNAMIC_FTRACE + depends on RANDOMIZE_BASE default y help Randomizes the location of the module region without considering the
arch/arm64/Makefile +3 -0
@@ -70,6 +70,9 @@ endif ifeq ($(CONFIG_ARM64_MODULE_PLTS),y) KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/arm64/kernel/module.lds +ifeq ($(CONFIG_DYNAMIC_FTRACE),y) +KBUILD_LDFLAGS_MODULE += $(objtree)/arch/arm64/kernel/ftrace-mod.o +endif endif # Default value
arch/arm64/include/asm/module.h +3 -0
@@ -30,6 +30,9 @@ struct mod_plt_sec {struct mod_arch_specific {struct mod_plt_sec core;struct mod_plt_sec init; + +/* for CONFIG_DYNAMIC_FTRACE */ +void *ftrace_trampoline; };#endif
arch/arm64/kernel/Makefile +3 -0
@@ -62,3 +62,6 @@ extra-y += $(head-y) vmlinux.lds ifeq ($(CONFIG_DEBUG_EFI),y) AFLAGS_head.o += -DVMLINUX_PATH="\"$(realpath $(objtree)/vmlinux)\"" endif + +# will be included by each individual module but not by the core kernel itself +extra-$(CONFIG_DYNAMIC_FTRACE) += ftrace-mod.o
arch/arm64/kernel/ftrace-mod.S new file +18 -0
@@ -0,0 +1,18 @@ +/* + * Copyright (C) 2017 Linaro Ltd <[email protected]> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/linkage.h> +#include <asm/assembler.h> + + .section ".text.ftrace_trampoline", "ax" + .align 3 +0: .quad 0 +__ftrace_trampoline: + ldr x16, 0b + br x16 +ENDPROC(__ftrace_trampoline)
arch/arm64/kernel/ftrace.c +51 -0
69 lines not on this page · raw message
arch/arm64/kernel/module.c +5 -1
@@ -420,8 +420,12 @@ int module_finalize(const Elf_Ehdr *hdr,for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {if (strcmp (".altinstructions" , secstrs + s->sh_name) ==0 ) {apply_alternatives ((void *)s->sh_addr, s->sh_size); -return 0 ; } +#ifdef CONFIG_ARM64_MODULE_PLTS +if (IS_ENABLED (CONFIG_DYNAMIC_FTRACE ) && + !strcmp (".text.ftrace_trampoline" , secstrs + s->sh_name)) + me->arch.ftrace_trampoline = (void *)s->sh_addr; +#endif }return 0 ;
Signature · 1 line
2.9.3