[PATCH] arm64: kretprobes: acquire the regs via a BRK exception

[Masami Hiramatsu (Google)]

Hi Mark,

On Thu,  8 Feb 2024 14:59:16 +0000
Mark Rutland <mark.rutland at 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 at kernel.org>

Thank you!

> 
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> Cc: Will Deacon <will at kernel.org>
> Cc: Florent Revest <revest at chromium.org>
> Cc: Masami Hiramatsu <mhiramat at kernel.org>
> Cc: Steven Rostedt <rostedt at 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
> 


-- 
Masami Hiramatsu (Google) <mhiramat at kernel.org>