| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 8 Jun 2016 10:07:14 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: David Long <dave.long@...aro.org>
Cc: Catalin Marinas <catalin.marinas@....com>,
Huang Shijie <shijie.huang@....com>,
James Morse <james.morse@....com>,
Marc Zyngier <marc.zyngier@....com>,
Pratyush Anand <panand@...hat.com>,
Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>,
Will Deacon <will.deacon@....com>,
William Cohen <wcohen@...hat.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Steve Capper <steve.capper@...aro.org>,
Li Bin <huawei.libin@...wei.com>,
Adam Buchbinder <adam.buchbinder@...il.com>,
Alex Bennée <alex.bennee@...aro.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Andrey Ryabinin <ryabinin.a.a@...il.com>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Christoffer Dall <christoffer.dall@...aro.org>,
Daniel Thompson <daniel.thompson@...aro.org>,
Dave P Martin <Dave.Martin@....com>,
Jens Wiklander <jens.wiklander@...aro.org>,
Jisheng Zhang <jszhang@...vell.com>,
John Blackwood <john.blackwood@...r.com>,
Mark Rutland <mark.rutland@....com>,
Petr Mladek <pmladek@...e.com>,
Robin Murphy <robin.murphy@....com>,
Suzuki K Poulose <suzuki.poulose@....com>,
Vladimir Murzin <Vladimir.Murzin@....com>,
Yang Shi <yang.shi@...aro.org>,
Zi Shen Lim <zlim.lnx@...il.com>,
yalin wang <yalin.wang2010@...il.com>,
Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v13 05/10] arm64: Kprobes with single stepping support
On Thu, 2 Jun 2016 23:26:19 -0400
David Long <dave.long@...aro.org> wrote:
> From: Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>
>
> Add support for basic kernel probes(kprobes) and jump probes
> (jprobes) for ARM64.
>
> Kprobes utilizes software breakpoint and single step debug
> exceptions supported on ARM v8.
>
> A software breakpoint is placed at the probe address to trap the
> kernel execution into the kprobe handler.
>
> ARM v8 supports enabling single stepping before the break exception
> return (ERET), with next PC in exception return address (ELR_EL1). The
> kprobe handler prepares an executable memory slot for out-of-line
> execution with a copy of the original instruction being probed, and
> enables single stepping. The PC is set to the out-of-line slot address
> before the ERET. With this scheme, the instruction is executed with the
> exact same register context except for the PC (and DAIF) registers.
>
> Debug mask (PSTATE.D) is enabled only when single stepping a recursive
> kprobe, e.g.: during kprobes reenter so that probed instruction can be
> single stepped within the kprobe handler -exception- context.
> The recursion depth of kprobe is always 2, i.e. upon probe re-entry,
> any further re-entry is prevented by not calling handlers and the case
> counted as a missed kprobe).
>
> Single stepping from the x-o-l slot has a drawback for PC-relative accesses
> like branching and symbolic literals access as the offset from the new PC
> (slot address) may not be ensured to fit in the immediate value of
> the opcode. Such instructions need simulation, so reject
> probing them.
>
> Instructions generating exceptions or cpu mode change are rejected
> for probing.
>
> Exclusive load/store instructions are rejected too. Additionally, the
> code is checked to see if it is inside an exclusive load/store sequence
> (code from Pratyush).
>
> System instructions are mostly enabled for stepping, except MSR/MRS
> accesses to "DAIF" flags in PSTATE, which are not safe for
> probing.
>
> Thanks to Steve Capper and Pratyush Anand for several suggested
> Changes.
Basically looks good to me.
I have some trivial comments.
>
> Signed-off-by: Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>
> Signed-off-by: David A. Long <dave.long@...aro.org>
> Signed-off-by: Pratyush Anand <panand@...hat.com>
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/debug-monitors.h | 5 +
> arch/arm64/include/asm/insn.h | 4 +-
> arch/arm64/include/asm/kprobes.h | 60 ++++
> arch/arm64/include/asm/probes.h | 44 +++
> arch/arm64/kernel/Makefile | 1 +
> arch/arm64/kernel/debug-monitors.c | 18 +-
> arch/arm64/kernel/kprobes-arm64.c | 144 +++++++++
> arch/arm64/kernel/kprobes-arm64.h | 35 +++
> arch/arm64/kernel/kprobes.c | 526 ++++++++++++++++++++++++++++++++
Not sure why kprobes.c and kprobes-arm64.c are splitted.
> arch/arm64/kernel/vmlinux.lds.S | 1 +
> arch/arm64/mm/fault.c | 27 +-
> 12 files changed, 861 insertions(+), 5 deletions(-)
> create mode 100644 arch/arm64/include/asm/kprobes.h
> create mode 100644 arch/arm64/include/asm/probes.h
> create mode 100644 arch/arm64/kernel/kprobes-arm64.c
> create mode 100644 arch/arm64/kernel/kprobes-arm64.h
> create mode 100644 arch/arm64/kernel/kprobes.c
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 0f7a624..5496b75 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -88,6 +88,7 @@ config ARM64
> select HAVE_REGS_AND_STACK_ACCESS_API
> select HAVE_RCU_TABLE_FREE
> select HAVE_SYSCALL_TRACEPOINTS
> + select HAVE_KPROBES
> select IOMMU_DMA if IOMMU_SUPPORT
> select IRQ_DOMAIN
> select IRQ_FORCED_THREADING
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 2fcb9b7..4b6b3f7 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -66,6 +66,11 @@
>
> #define CACHE_FLUSH_IS_SAFE 1
>
> +/* kprobes BRK opcodes with ESR encoding */
> +#define BRK64_ESR_MASK 0xFFFF
> +#define BRK64_ESR_KPROBES 0x0004
> +#define BRK64_OPCODE_KPROBES (AARCH64_BREAK_MON | (BRK64_ESR_KPROBES << 5))
> +
> /* AArch32 */
> #define DBG_ESR_EVT_BKPT 0x4
> #define DBG_ESR_EVT_VECC 0x5
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 98e4edd..be2d2b9 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -253,6 +253,8 @@ __AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
> __AARCH64_INSN_FUNCS(ldr_lit, 0xBF000000, 0x18000000)
> __AARCH64_INSN_FUNCS(ldrsw_lit, 0xFF000000, 0x98000000)
> __AARCH64_INSN_FUNCS(exclusive, 0x3F800000, 0x08000000)
> +__AARCH64_INSN_FUNCS(load_ex, 0x3F400000, 0x08400000)
> +__AARCH64_INSN_FUNCS(store_ex, 0x3F400000, 0x08000000)
> __AARCH64_INSN_FUNCS(stp_post, 0x7FC00000, 0x28800000)
> __AARCH64_INSN_FUNCS(ldp_post, 0x7FC00000, 0x28C00000)
> __AARCH64_INSN_FUNCS(stp_pre, 0x7FC00000, 0x29800000)
> @@ -402,7 +404,7 @@ bool aarch32_insn_is_wide(u32 insn);
> #define A32_RT_OFFSET 12
> #define A32_RT2_OFFSET 0
>
> -u32 aarch64_extract_system_register(u32 insn);
> +u32 aarch64_insn_extract_system_reg(u32 insn);
> u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
> u32 aarch32_insn_mcr_extract_opc2(u32 insn);
> u32 aarch32_insn_mcr_extract_crm(u32 insn);
> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
> new file mode 100644
> index 0000000..79c9511
> --- /dev/null
> +++ b/arch/arm64/include/asm/kprobes.h
> @@ -0,0 +1,60 @@
> +/*
> + * arch/arm64/include/asm/kprobes.h
> + *
> + * Copyright (C) 2013 Linaro Limited
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#ifndef _ARM_KPROBES_H
> +#define _ARM_KPROBES_H
> +
> +#include <linux/types.h>
> +#include <linux/ptrace.h>
> +#include <linux/percpu.h>
> +
> +#define __ARCH_WANT_KPROBES_INSN_SLOT
> +#define MAX_INSN_SIZE 1
> +#define MAX_STACK_SIZE 128
> +
> +#define flush_insn_slot(p) do { } while (0)
> +#define kretprobe_blacklist_size 0
> +
> +#include <asm/probes.h>
> +
> +struct prev_kprobe {
> + struct kprobe *kp;
> + unsigned int status;
> +};
> +
> +/* Single step context for kprobe */
> +struct kprobe_step_ctx {
> + unsigned long ss_pending;
> + unsigned long match_addr;
> +};
> +
> +/* per-cpu kprobe control block */
> +struct kprobe_ctlblk {
> + unsigned int kprobe_status;
> + unsigned long saved_irqflag;
> + struct prev_kprobe prev_kprobe;
> + struct kprobe_step_ctx ss_ctx;
> + struct pt_regs jprobe_saved_regs;
> + char jprobes_stack[MAX_STACK_SIZE];
> +};
> +
> +void arch_remove_kprobe(struct kprobe *);
> +int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
> +int kprobe_exceptions_notify(struct notifier_block *self,
> + unsigned long val, void *data);
> +int kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr);
> +int kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr);
> +
> +#endif /* _ARM_KPROBES_H */
> diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h
> new file mode 100644
> index 0000000..c5fcbe6
> --- /dev/null
> +++ b/arch/arm64/include/asm/probes.h
> @@ -0,0 +1,44 @@
> +/*
> + * arch/arm64/include/asm/probes.h
> + *
> + * Copyright (C) 2013 Linaro Limited
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +#ifndef _ARM_PROBES_H
> +#define _ARM_PROBES_H
> +
> +struct kprobe;
> +struct arch_specific_insn;
> +
> +typedef u32 kprobe_opcode_t;
> +typedef unsigned long (kprobes_pstate_check_t)(unsigned long);
> +typedef void (kprobes_handler_t) (u32 opcode, long addr, struct pt_regs *);
> +
> +enum pc_restore_type {
> + NO_RESTORE,
> + RESTORE_PC,
> +};
> +
> +struct kprobe_pc_restore {
> + enum pc_restore_type type;
> + unsigned long addr;
> +};
These seems overcoding. You just need "unsigned long restore_pc"
and if it is 0, skip it :)
> +
> +/* architecture specific copy of original instruction */
> +struct arch_specific_insn {
> + kprobe_opcode_t *insn;
> + kprobes_pstate_check_t *pstate_cc;
> + kprobes_handler_t *handler;
> + /* restore address after step xol */
> + struct kprobe_pc_restore restore;
> +};
> +
> +#endif
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 4653aca..d78ed62 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -37,6 +37,7 @@ arm64-obj-$(CONFIG_CPU_PM) += sleep.o suspend.o
> arm64-obj-$(CONFIG_CPU_IDLE) += cpuidle.o
> arm64-obj-$(CONFIG_JUMP_LABEL) += jump_label.o
> arm64-obj-$(CONFIG_KGDB) += kgdb.o
> +arm64-obj-$(CONFIG_KPROBES) += kprobes.o kprobes-arm64.o
> arm64-obj-$(CONFIG_EFI) += efi.o efi-entry.stub.o
> arm64-obj-$(CONFIG_PCI) += pci.o
> arm64-obj-$(CONFIG_ARMV8_DEPRECATED) += armv8_deprecated.o
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 65ee636..fcedced 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -24,6 +24,7 @@
> #include <linux/init.h>
> #include <linux/kprobes.h>
> #include <linux/ptrace.h>
> +#include <linux/kprobes.h>
> #include <linux/stat.h>
> #include <linux/uaccess.h>
>
> @@ -274,10 +275,14 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
> */
> user_rewind_single_step(current);
> } else {
> +#ifdef CONFIG_KPROBES
> + if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
> + return 0;
> +#endif
> if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
> return 0;
>
> - pr_warning("Unexpected kernel single-step exception at EL1\n");
> + pr_warn("Unexpected kernel single-step exception at EL1\n");
This change would better be splitted, anyway, it depends on the maintainer
of this file (Will and Catalin?)
> /*
> * Re-enable stepping since we know that we will be
> * returning to regs.
> @@ -332,8 +337,15 @@ static int brk_handler(unsigned long addr, unsigned int esr,
> {
> if (user_mode(regs)) {
> send_user_sigtrap(TRAP_BRKPT);
> - } else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) {
> - pr_warning("Unexpected kernel BRK exception at EL1\n");
> + }
> +#ifdef CONFIG_KPROBES
> + else if ((esr & BRK64_ESR_MASK) == BRK64_ESR_KPROBES) {
> + if (kprobe_breakpoint_handler(regs, esr) != DBG_HOOK_HANDLED)
> + return -EFAULT;
> + }
> +#endif
> + else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) {
> + pr_warn("Unexpected kernel BRK exception at EL1\n");
> return -EFAULT;
> }
>
> diff --git a/arch/arm64/kernel/kprobes-arm64.c b/arch/arm64/kernel/kprobes-arm64.c
> new file mode 100644
> index 0000000..0af5d94
> --- /dev/null
> +++ b/arch/arm64/kernel/kprobes-arm64.c
> @@ -0,0 +1,144 @@
> +/*
> + * arch/arm64/kernel/kprobes-arm64.c
> + *
> + * Copyright (C) 2013 Linaro Limited.
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/kprobes.h>
> +#include <linux/module.h>
> +#include <asm/kprobes.h>
> +#include <asm/insn.h>
> +#include <asm/sections.h>
> +
> +#include "kprobes-arm64.h"
> +
> +static bool __kprobes aarch64_insn_is_steppable(u32 insn)
> +{
> + /*
> + * Branch instructions will write a new value into the PC which is
> + * likely to be relative to the XOL address and therefore invalid.
> + * Deliberate generation of an exception during stepping is also not
> + * currently safe. Lastly, MSR instructions can do any number of nasty
> + * things we can't handle during single-stepping.
> + */
> + if (aarch64_get_insn_class(insn) == AARCH64_INSN_CLS_BR_SYS) {
> + if (aarch64_insn_is_branch(insn) ||
> + aarch64_insn_is_msr_imm(insn) ||
> + aarch64_insn_is_msr_reg(insn) ||
> + aarch64_insn_is_exception(insn) ||
> + aarch64_insn_is_eret(insn))
> + return false;
> +
> + /*
> + * The MRS instruction may not return a correct value when
> + * executing in the single-stepping environment. We do make one
> + * exception, for reading the DAIF bits.
> + */
> + if (aarch64_insn_is_mrs(insn))
> + return aarch64_insn_extract_system_reg(insn)
> + != AARCH64_INSN_SPCLREG_DAIF;
> +
> + /*
> + * The HINT instruction is is problematic when single-stepping,
> + * except for the NOP case.
> + */
> + if (aarch64_insn_is_hint(insn))
> + return aarch64_insn_is_nop(insn);
> +
> + return true;
> + }
> +
> + /*
> + * Instructions which load PC relative literals are not going to work
> + * when executed from an XOL slot. Instructions doing an exclusive
> + * load/store are not going to complete successfully when single-step
> + * exception handling happens in the middle of the sequence.
> + */
> + if (aarch64_insn_uses_literal(insn) ||
> + aarch64_insn_is_exclusive(insn))
> + return false;
> +
> + return true;
> +}
> +
> +/* Return:
> + * INSN_REJECTED If instruction is one not allowed to kprobe,
> + * INSN_GOOD If instruction is supported and uses instruction slot,
> + * INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot.
Is there any chance to return INSN_GOOD_NO_SLOT?
> + */
> +static enum kprobe_insn __kprobes
> +arm_probe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
> +{
> + /*
> + * Instructions reading or modifying the PC won't work from the XOL
> + * slot.
> + */
> + if (aarch64_insn_is_steppable(insn))
> + return INSN_GOOD;
> + else
> + return INSN_REJECTED;
> +}
> +
> +static bool __kprobes
> +is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end)
> +{
> + while (scan_start > scan_end) {
> + /*
> + * atomic region starts from exclusive load and ends with
> + * exclusive store.
> + */
> + if (aarch64_insn_is_store_ex(le32_to_cpu(*scan_start)))
> + return false;
> + else if (aarch64_insn_is_load_ex(le32_to_cpu(*scan_start)))
> + return true;
> + scan_start--;
> + }
> +
> + return false;
> +}
> +
> +enum kprobe_insn __kprobes
> +arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi)
> +{
> + enum kprobe_insn decoded;
> + kprobe_opcode_t insn = le32_to_cpu(*addr);
> + kprobe_opcode_t *scan_start = addr - 1;
> + kprobe_opcode_t *scan_end = addr - MAX_ATOMIC_CONTEXT_SIZE;
> +#if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
> + struct module *mod;
> +#endif
> +
> + if (addr >= (kprobe_opcode_t *)_text &&
> + scan_end < (kprobe_opcode_t *)_text)
> + scan_end = (kprobe_opcode_t *)_text;
> +#if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
> + else {
> + preempt_disable();
> + mod = __module_address((unsigned long)addr);
> + if (mod && within_module_init((unsigned long)addr, mod) &&
> + !within_module_init((unsigned long)scan_end, mod))
> + scan_end = (kprobe_opcode_t *)mod->init_layout.base;
> + else if (mod && within_module_core((unsigned long)addr, mod) &&
> + !within_module_core((unsigned long)scan_end, mod))
> + scan_end = (kprobe_opcode_t *)mod->core_layout.base;
What happen if mod == NULL? it should be return error, isn't it?
> + preempt_enable();
> + }
> +#endif
> + decoded = arm_probe_decode_insn(insn, asi);
> +
> + if (decoded == INSN_REJECTED ||
> + is_probed_address_atomic(scan_start, scan_end))
> + return INSN_REJECTED;
> +
> + return decoded;
> +}
Thank you,
--
Masami Hiramatsu <mhiramat@...nel.org>
Powered by blists - more mailing lists