| Message ID | 20240208145916.2004154-1-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: kretprobes: acquire the regs via a BRK exception | expand |
Hi Mark, On Thu, 8 Feb 2024 14:59:16 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > On arm64, kprobes always take an exception and so create a struct > pt_regs through the usual exception entry logic. Similarly kretprobes > taskes and exception for function entry, but for function returns it > uses a trampoline which attempts to create a struct pt_regs without > taking an exception. > > This is problematic for a few reasons, including: > > 1) The kretprobes trampoline neither saves nor restores all of the > portions of PSTATE. Before invoking the handler it saves a number of > portions of PSTATE, and after returning from the handler it restores > NZCV before returning to the original return address provided by the > handler. > > 2) The kretprobe trampoline constructs the PSTATE value piecemeal from > special purpose registers as it cannot read all of PSTATE atomically > without taking an exception. This is somewhat fragile, and it's not > possible to reliably recover PSTATE information which only exists on > some physical CPUs (e.g. when SSBS support is mismatched). > > Today the kretprobes trampoline does not record: > > - BTYPE > - SSBS > - ALLINT > - SS > - PAN > - UAO > - DIT > - TCO > > ... and this will only get worse with future architecture extensions > which add more PSTATE bits. > > 3) The kretprobes trampoline doesn't store portions of struct pt_regs > (e.g. the PMR value when using pseudo-NMIs). Due to this, helpers > which operate on a struct pt_regs, such as interrupts_enabled(), may > not work correctly. > > 4) The function entry and function exit handlers run in different > contexts. The entry handler will always be run in a debug exception > context (which is currently treated as an NMI), but the return will > be treated as whatever context the instrumented function was executed > in. The differences between these contexts are liable to cause > problems (e.g. as the two can be differently interruptible or > preemptible, adversely affecting synchronization between the > handlers). > > 5) As the kretprobes trampoline runs in the same context as the code > being probed, it is subject to the same single-stepping context, > which may not be desirable if this is being driven by the kprobes > handlers. > > Overall, this is fragile, painful to maintain, and gets in the way of > supporting other things (e.g. RELIABLE_STACKTRACE, FEAT_NMI). I agree all of those reasons. > > This patch addresses these issues by replacing the kretprobes trampoline > with a `BRK` instruction, and using an exception boundary to acquire and > restore the regs, in the same way as the regular kprobes trampoline. This design and code looks good to me. > > Ive tested this atop v6.8-rc3: > > | KTAP version 1 > | 1..1 > | KTAP version 1 > | # Subtest: kprobes_test > | # module: test_kprobes > | 1..7 > | ok 1 test_kprobe > | ok 2 test_kprobes > | ok 3 test_kprobe_missed > | ok 4 test_kretprobe > | ok 5 test_kretprobes > | ok 6 test_stacktrace_on_kretprobe > | ok 7 test_stacktrace_on_nested_kretprobe > | # kprobes_test: pass:7 fail:0 skip:0 total:7 > | # Totals: pass:7 fail:0 skip:0 total:7 > | ok 1 kprobes_test Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thank you! > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Florent Revest <revest@chromium.org> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > --- > arch/arm64/include/asm/brk-imm.h | 2 + > arch/arm64/kernel/probes/kprobes.c | 21 +++-- > arch/arm64/kernel/probes/kprobes_trampoline.S | 78 ++----------------- > 3 files changed, 24 insertions(+), 77 deletions(-) > > diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h > index 1abdcd508a113..beb42c62b6acc 100644 > --- a/arch/arm64/include/asm/brk-imm.h > +++ b/arch/arm64/include/asm/brk-imm.h > @@ -11,6 +11,7 @@ > * 0x004: for installing kprobes > * 0x005: for installing uprobes > * 0x006: for kprobe software single-step > + * 0x007: for kretprobe return > * Allowed values for kgdb are 0x400 - 0x7ff > * 0x100: for triggering a fault on purpose (reserved) > * 0x400: for dynamic BRK instruction > @@ -23,6 +24,7 @@ > #define KPROBES_BRK_IMM 0x004 > #define UPROBES_BRK_IMM 0x005 > #define KPROBES_BRK_SS_IMM 0x006 > +#define KRETPROBES_BRK_IMM 0x007 > #define FAULT_BRK_IMM 0x100 > #define KGDB_DYN_DBG_BRK_IMM 0x400 > #define KGDB_COMPILED_DBG_BRK_IMM 0x401 > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c > index 70b91a8c6bb3f..327855a11df2f 100644 > --- a/arch/arm64/kernel/probes/kprobes.c > +++ b/arch/arm64/kernel/probes/kprobes.c > @@ -371,6 +371,21 @@ static struct break_hook kprobes_break_ss_hook = { > .fn = kprobe_breakpoint_ss_handler, > }; > > +static int __kprobes > +kretprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr) > +{ > + if (regs->pc != (unsigned long)__kretprobe_trampoline) > + return DBG_HOOK_ERROR; > + > + regs->pc = kretprobe_trampoline_handler(regs, (void *)regs->regs[29]); > + return DBG_HOOK_HANDLED; > +} > + > +static struct break_hook kretprobes_break_hook = { > + .imm = KRETPROBES_BRK_IMM, > + .fn = kretprobe_breakpoint_handler, > +}; > + > /* > * Provide a blacklist of symbols identifying ranges which cannot be kprobed. > * This blacklist is exposed to userspace via debugfs (kprobes/blacklist). > @@ -396,11 +411,6 @@ int __init arch_populate_kprobe_blacklist(void) > return ret; > } > > -void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs) > -{ > - return (void *)kretprobe_trampoline_handler(regs, (void *)regs->regs[29]); > -} > - > void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, > struct pt_regs *regs) > { > @@ -420,6 +430,7 @@ int __init arch_init_kprobes(void) > { > register_kernel_break_hook(&kprobes_break_hook); > register_kernel_break_hook(&kprobes_break_ss_hook); > + register_kernel_break_hook(&kretprobes_break_hook); > > return 0; > } > diff --git a/arch/arm64/kernel/probes/kprobes_trampoline.S b/arch/arm64/kernel/probes/kprobes_trampoline.S > index 9a6499bed58b0..a362f3dbb3d11 100644 > --- a/arch/arm64/kernel/probes/kprobes_trampoline.S > +++ b/arch/arm64/kernel/probes/kprobes_trampoline.S > @@ -4,83 +4,17 @@ > */ > > #include <linux/linkage.h> > -#include <asm/asm-offsets.h> > +#include <asm/asm-bug.h> > #include <asm/assembler.h> > > .text > > - .macro save_all_base_regs > - stp x0, x1, [sp, #S_X0] > - stp x2, x3, [sp, #S_X2] > - stp x4, x5, [sp, #S_X4] > - stp x6, x7, [sp, #S_X6] > - stp x8, x9, [sp, #S_X8] > - stp x10, x11, [sp, #S_X10] > - stp x12, x13, [sp, #S_X12] > - stp x14, x15, [sp, #S_X14] > - stp x16, x17, [sp, #S_X16] > - stp x18, x19, [sp, #S_X18] > - stp x20, x21, [sp, #S_X20] > - stp x22, x23, [sp, #S_X22] > - stp x24, x25, [sp, #S_X24] > - stp x26, x27, [sp, #S_X26] > - stp x28, x29, [sp, #S_X28] > - add x0, sp, #PT_REGS_SIZE > - stp lr, x0, [sp, #S_LR] > - /* > - * Construct a useful saved PSTATE > - */ > - mrs x0, nzcv > - mrs x1, daif > - orr x0, x0, x1 > - mrs x1, CurrentEL > - orr x0, x0, x1 > - mrs x1, SPSel > - orr x0, x0, x1 > - stp xzr, x0, [sp, #S_PC] > - .endm > - > - .macro restore_all_base_regs > - ldr x0, [sp, #S_PSTATE] > - and x0, x0, #(PSR_N_BIT | PSR_Z_BIT | PSR_C_BIT | PSR_V_BIT) > - msr nzcv, x0 > - ldp x0, x1, [sp, #S_X0] > - ldp x2, x3, [sp, #S_X2] > - ldp x4, x5, [sp, #S_X4] > - ldp x6, x7, [sp, #S_X6] > - ldp x8, x9, [sp, #S_X8] > - ldp x10, x11, [sp, #S_X10] > - ldp x12, x13, [sp, #S_X12] > - ldp x14, x15, [sp, #S_X14] > - ldp x16, x17, [sp, #S_X16] > - ldp x18, x19, [sp, #S_X18] > - ldp x20, x21, [sp, #S_X20] > - ldp x22, x23, [sp, #S_X22] > - ldp x24, x25, [sp, #S_X24] > - ldp x26, x27, [sp, #S_X26] > - ldp x28, x29, [sp, #S_X28] > - .endm > - > SYM_CODE_START(__kretprobe_trampoline) > - sub sp, sp, #PT_REGS_SIZE > - > - save_all_base_regs > - > - /* Setup a frame pointer. */ > - add x29, sp, #S_FP > - > - mov x0, sp > - bl trampoline_probe_handler > /* > - * Replace trampoline address in lr with actual orig_ret_addr return > - * address. > + * Trigger a breakpoint exception. The PC will be adjusted by > + * kretprobe_breakpoint_handler(), and no subsequent instructions will > + * be executed from the trampoline. > */ > - mov lr, x0 > - > - /* The frame pointer (x29) is restored with other registers. */ > - restore_all_base_regs > - > - add sp, sp, #PT_REGS_SIZE > - ret > - > + brk #KRETPROBES_BRK_IMM > + ASM_BUG() > SYM_CODE_END(__kretprobe_trampoline) > -- > 2.30.2 >
On Thu, 08 Feb 2024 14:59:16 +0000, Mark Rutland wrote: > On arm64, kprobes always take an exception and so create a struct > pt_regs through the usual exception entry logic. Similarly kretprobes > taskes and exception for function entry, but for function returns it > uses a trampoline which attempts to create a struct pt_regs without > taking an exception. > > This is problematic for a few reasons, including: > > [...] Applied to arm64 (for-next/misc), thanks! [1/1] arm64: kretprobes: acquire the regs via a BRK exception https://git.kernel.org/arm64/c/253751233b19
diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h index 1abdcd508a113..beb42c62b6acc 100644 --- a/arch/arm64/include/asm/brk-imm.h +++ b/arch/arm64/include/asm/brk-imm.h @@ -11,6 +11,7 @@ * 0x004: for installing kprobes * 0x005: for installing uprobes * 0x006: for kprobe software single-step + * 0x007: for kretprobe return * Allowed values for kgdb are 0x400 - 0x7ff * 0x100: for triggering a fault on purpose (reserved) * 0x400: for dynamic BRK instruction @@ -23,6 +24,7 @@ #define KPROBES_BRK_IMM 0x004 #define UPROBES_BRK_IMM 0x005 #define KPROBES_BRK_SS_IMM 0x006 +#define KRETPROBES_BRK_IMM 0x007 #define FAULT_BRK_IMM 0x100 #define KGDB_DYN_DBG_BRK_IMM 0x400 #define KGDB_COMPILED_DBG_BRK_IMM 0x401 diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index 70b91a8c6bb3f..327855a11df2f 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -371,6 +371,21 @@ static struct break_hook kprobes_break_ss_hook = { .fn = kprobe_breakpoint_ss_handler, }; +static int __kprobes +kretprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr) +{ + if (regs->pc != (unsigned long)__kretprobe_trampoline) + return DBG_HOOK_ERROR; + + regs->pc = kretprobe_trampoline_handler(regs, (void *)regs->regs[29]); + return DBG_HOOK_HANDLED; +} + +static struct break_hook kretprobes_break_hook = { + .imm = KRETPROBES_BRK_IMM, + .fn = kretprobe_breakpoint_handler, +}; + /* * Provide a blacklist of symbols identifying ranges which cannot be kprobed. * This blacklist is exposed to userspace via debugfs (kprobes/blacklist). @@ -396,11 +411,6 @@ int __init arch_populate_kprobe_blacklist(void) return ret; } -void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs) -{ - return (void *)kretprobe_trampoline_handler(regs, (void *)regs->regs[29]); -} - void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs) { @@ -420,6 +430,7 @@ int __init arch_init_kprobes(void) { register_kernel_break_hook(&kprobes_break_hook); register_kernel_break_hook(&kprobes_break_ss_hook); + register_kernel_break_hook(&kretprobes_break_hook); return 0; } diff --git a/arch/arm64/kernel/probes/kprobes_trampoline.S b/arch/arm64/kernel/probes/kprobes_trampoline.S index 9a6499bed58b0..a362f3dbb3d11 100644 --- a/arch/arm64/kernel/probes/kprobes_trampoline.S +++ b/arch/arm64/kernel/probes/kprobes_trampoline.S @@ -4,83 +4,17 @@ */ #include <linux/linkage.h> -#include <asm/asm-offsets.h> +#include <asm/asm-bug.h> #include <asm/assembler.h> .text - .macro save_all_base_regs - stp x0, x1, [sp, #S_X0] - stp x2, x3, [sp, #S_X2] - stp x4, x5, [sp, #S_X4] - stp x6, x7, [sp, #S_X6] - stp x8, x9, [sp, #S_X8] - stp x10, x11, [sp, #S_X10] - stp x12, x13, [sp, #S_X12] - stp x14, x15, [sp, #S_X14] - stp x16, x17, [sp, #S_X16] - stp x18, x19, [sp, #S_X18] - stp x20, x21, [sp, #S_X20] - stp x22, x23, [sp, #S_X22] - stp x24, x25, [sp, #S_X24] - stp x26, x27, [sp, #S_X26] - stp x28, x29, [sp, #S_X28] - add x0, sp, #PT_REGS_SIZE - stp lr, x0, [sp, #S_LR] - /* - * Construct a useful saved PSTATE - */ - mrs x0, nzcv - mrs x1, daif - orr x0, x0, x1 - mrs x1, CurrentEL - orr x0, x0, x1 - mrs x1, SPSel - orr x0, x0, x1 - stp xzr, x0, [sp, #S_PC] - .endm - - .macro restore_all_base_regs - ldr x0, [sp, #S_PSTATE] - and x0, x0, #(PSR_N_BIT | PSR_Z_BIT | PSR_C_BIT | PSR_V_BIT) - msr nzcv, x0 - ldp x0, x1, [sp, #S_X0] - ldp x2, x3, [sp, #S_X2] - ldp x4, x5, [sp, #S_X4] - ldp x6, x7, [sp, #S_X6] - ldp x8, x9, [sp, #S_X8] - ldp x10, x11, [sp, #S_X10] - ldp x12, x13, [sp, #S_X12] - ldp x14, x15, [sp, #S_X14] - ldp x16, x17, [sp, #S_X16] - ldp x18, x19, [sp, #S_X18] - ldp x20, x21, [sp, #S_X20] - ldp x22, x23, [sp, #S_X22] - ldp x24, x25, [sp, #S_X24] - ldp x26, x27, [sp, #S_X26] - ldp x28, x29, [sp, #S_X28] - .endm - SYM_CODE_START(__kretprobe_trampoline) - sub sp, sp, #PT_REGS_SIZE - - save_all_base_regs - - /* Setup a frame pointer. */ - add x29, sp, #S_FP - - mov x0, sp - bl trampoline_probe_handler /* - * Replace trampoline address in lr with actual orig_ret_addr return - * address. + * Trigger a breakpoint exception. The PC will be adjusted by + * kretprobe_breakpoint_handler(), and no subsequent instructions will + * be executed from the trampoline. */ - mov lr, x0 - - /* The frame pointer (x29) is restored with other registers. */ - restore_all_base_regs - - add sp, sp, #PT_REGS_SIZE - ret - + brk #KRETPROBES_BRK_IMM + ASM_BUG() SYM_CODE_END(__kretprobe_trampoline)
On arm64, kprobes always take an exception and so create a struct pt_regs through the usual exception entry logic. Similarly kretprobes taskes and exception for function entry, but for function returns it uses a trampoline which attempts to create a struct pt_regs without taking an exception. This is problematic for a few reasons, including: 1) The kretprobes trampoline neither saves nor restores all of the portions of PSTATE. Before invoking the handler it saves a number of portions of PSTATE, and after returning from the handler it restores NZCV before returning to the original return address provided by the handler. 2) The kretprobe trampoline constructs the PSTATE value piecemeal from special purpose registers as it cannot read all of PSTATE atomically without taking an exception. This is somewhat fragile, and it's not possible to reliably recover PSTATE information which only exists on some physical CPUs (e.g. when SSBS support is mismatched). Today the kretprobes trampoline does not record: - BTYPE - SSBS - ALLINT - SS - PAN - UAO - DIT - TCO ... and this will only get worse with future architecture extensions which add more PSTATE bits. 3) The kretprobes trampoline doesn't store portions of struct pt_regs (e.g. the PMR value when using pseudo-NMIs). Due to this, helpers which operate on a struct pt_regs, such as interrupts_enabled(), may not work correctly. 4) The function entry and function exit handlers run in different contexts. The entry handler will always be run in a debug exception context (which is currently treated as an NMI), but the return will be treated as whatever context the instrumented function was executed in. The differences between these contexts are liable to cause problems (e.g. as the two can be differently interruptible or preemptible, adversely affecting synchronization between the handlers). 5) As the kretprobes trampoline runs in the same context as the code being probed, it is subject to the same single-stepping context, which may not be desirable if this is being driven by the kprobes handlers. Overall, this is fragile, painful to maintain, and gets in the way of supporting other things (e.g. RELIABLE_STACKTRACE, FEAT_NMI). This patch addresses these issues by replacing the kretprobes trampoline with a `BRK` instruction, and using an exception boundary to acquire and restore the regs, in the same way as the regular kprobes trampoline. Ive tested this atop v6.8-rc3: | KTAP version 1 | 1..1 | KTAP version 1 | # Subtest: kprobes_test | # module: test_kprobes | 1..7 | ok 1 test_kprobe | ok 2 test_kprobes | ok 3 test_kprobe_missed | ok 4 test_kretprobe | ok 5 test_kretprobes | ok 6 test_stacktrace_on_kretprobe | ok 7 test_stacktrace_on_nested_kretprobe | # kprobes_test: pass:7 fail:0 skip:0 total:7 | # Totals: pass:7 fail:0 skip:0 total:7 | ok 1 kprobes_test Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Florent Revest <revest@chromium.org> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Steven Rostedt <rostedt@goodmis.org> --- arch/arm64/include/asm/brk-imm.h | 2 + arch/arm64/kernel/probes/kprobes.c | 21 +++-- arch/arm64/kernel/probes/kprobes_trampoline.S | 78 ++----------------- 3 files changed, 24 insertions(+), 77 deletions(-)