* [PATCH v2 0/7] arm64: uprobes - ARM32 instruction probing [not found] <CGME20180926121213eucas1p1e85f71d1187eb6b50c320377e5ea907f@eucas1p1.samsung.com> @ 2018-09-26 12:11 ` Maciej Slodczyk [not found] ` <CGME20180926121214eucas1p2b262936ddd96980b7b4369f16b52c6ce@eucas1p2.samsung.com> ` (6 more replies) 0 siblings, 7 replies; 18+ messages in thread From: Maciej Slodczyk @ 2018-09-26 12:11 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk, m.slodczyk2 The uprobe feature on ARM64 kernel does not support ARM A32 instruction probing, making 32 bit apps running on 64 bit kernel unprobeable. This patchset utilizes ARM32 uprobe code in ARM64 tree with following modifications: - moves ARM32 uprobes code form arch/arm to lib/uprobes/arm to be reused by ARM64 code - makes adjustments in api so that it suits the one used by ARM64 kernel, e.g. struct pt_regs, register numbers - takes into account differences between 32 and 64 bit ARM architectures. A64 instruction handling is left unchanged. Detect what kind of instruction is being probed and depending on the result: - if an A64 instruction handle it the old way, using existing A64 instructions probing code, - if an A32 instruction decode it and handle using the new code, moved from 32 bit arm kernel tree. Currently following test cases have been carried out with positive results: - simulation-able instructions, like: STM, LDM, BLX, BL - xol-able instructions, like: STR, LDR, ADD, CMN, CMP - branching - uretprobes. This patchset does not implement T32 (Thumb) instructions probing (which is also unsupported on "native" ARM32 arch). v2: - Rebased on 4.19-rc4 and tested against it - Fixed build errors reported by kbuild test robot Maciej Slodczyk (7): arm64: move arm uprobes code to be reused by arm64 arm64: uprobes - fix checkpatch issues arm64: introduce get_swbp_insn() instead of static assignment arm64: change arm64 probes handler prototype arm64: make arm uprobes code reusable by arm64 arm64: change arm_probe_decode_insn() function name arm64: uprobes - ARM32 instruction probing arch/arm/include/asm/probes.h | 8 ++ arch/arm/include/asm/ptrace.h | 32 +++++ arch/arm/include/asm/uprobes.h | 2 +- arch/arm/probes/Makefile | 8 +- arch/arm/probes/kprobes/actions-arm.c | 2 +- arch/arm/probes/kprobes/checkers-arm.c | 4 +- arch/arm/probes/kprobes/checkers-common.c | 4 +- arch/arm/probes/kprobes/checkers-thumb.c | 2 +- arch/arm/probes/kprobes/checkers.h | 2 +- arch/arm/probes/kprobes/core.c | 2 +- arch/arm/probes/kprobes/core.h | 2 +- arch/arm/probes/kprobes/test-core.c | 2 +- arch/arm/probes/uprobes/Makefile | 2 +- arch/arm/probes/uprobes/core.c | 10 +- arch/arm64/include/asm/probes.h | 24 +++- arch/arm64/include/asm/ptrace.h | 21 +++ arch/arm64/include/asm/uprobes.h | 21 ++- arch/arm64/kernel/debug-monitors.c | 8 ++ arch/arm64/kernel/probes/Makefile | 2 + arch/arm64/kernel/probes/decode-insn.c | 30 ++-- arch/arm64/kernel/probes/decode-insn.h | 15 +- arch/arm64/kernel/probes/kprobes.c | 4 +- arch/arm64/kernel/probes/simulate-insn.c | 32 +++-- arch/arm64/kernel/probes/simulate-insn.h | 24 ++-- arch/arm64/kernel/probes/uprobes.c | 122 ++++++++++++++-- include/linux/uprobes.h | 1 + kernel/events/uprobes.c | 12 +- lib/Makefile | 2 + lib/probes/Makefile | 2 + lib/probes/arm/Makefile | 11 ++ .../uprobes => lib/probes/arm}/actions-arm.c | 153 ++++++++++++++++++--- {arch/arm/probes => lib/probes/arm}/decode-arm.c | 58 +++++--- {arch/arm/probes => lib/probes/arm}/decode-arm.h | 4 +- {arch/arm/probes => lib/probes/arm}/decode.c | 18 ++- {arch/arm/probes => lib/probes/arm}/decode.h | 48 ++++--- 35 files changed, 552 insertions(+), 142 deletions(-) create mode 100644 lib/probes/Makefile create mode 100644 lib/probes/arm/Makefile rename {arch/arm/probes/uprobes => lib/probes/arm}/actions-arm.c (61%) rename {arch/arm/probes => lib/probes/arm}/decode-arm.c (95%) rename {arch/arm/probes => lib/probes/arm}/decode-arm.h (94%) rename {arch/arm/probes => lib/probes/arm}/decode.c (98%) rename {arch/arm/probes => lib/probes/arm}/decode.h (94%) -- 2.7.4 ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <CGME20180926121214eucas1p2b262936ddd96980b7b4369f16b52c6ce@eucas1p2.samsung.com>]
* [PATCH v2 1/7] arm64: move arm uprobes code to be reused by arm64 [not found] ` <CGME20180926121214eucas1p2b262936ddd96980b7b4369f16b52c6ce@eucas1p2.samsung.com> @ 2018-09-26 12:11 ` Maciej Slodczyk 2018-09-29 9:37 ` Suzuki K Poulose 0 siblings, 1 reply; 18+ messages in thread From: Maciej Slodczyk @ 2018-09-26 12:11 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk, m.slodczyk2 Move ARM32 uprobes code from arch/arm/probes/ to a more common location - lib/probes/arm/. This code will be used by ARM64 code when uprobing 32-bit applications. Signed-off-by: Maciej Slodczyk <m.slodczyk2@partner.samsung.com> --- arch/arm/probes/Makefile | 8 ++++---- arch/arm/probes/kprobes/actions-arm.c | 2 +- arch/arm/probes/kprobes/checkers-arm.c | 4 ++-- arch/arm/probes/kprobes/checkers-common.c | 4 ++-- arch/arm/probes/kprobes/checkers-thumb.c | 2 +- arch/arm/probes/kprobes/checkers.h | 2 +- arch/arm/probes/kprobes/core.c | 2 +- arch/arm/probes/kprobes/core.h | 2 +- arch/arm/probes/kprobes/test-core.c | 2 +- arch/arm/probes/uprobes/Makefile | 2 +- arch/arm/probes/uprobes/core.c | 4 ++-- lib/Makefile | 2 ++ lib/probes/Makefile | 1 + lib/probes/arm/Makefile | 6 ++++++ {arch/arm/probes/uprobes => lib/probes/arm}/actions-arm.c | 4 ++-- {arch/arm/probes => lib/probes/arm}/decode-arm.c | 0 {arch/arm/probes => lib/probes/arm}/decode-arm.h | 0 {arch/arm/probes => lib/probes/arm}/decode.c | 0 {arch/arm/probes => lib/probes/arm}/decode.h | 0 19 files changed, 28 insertions(+), 19 deletions(-) create mode 100644 lib/probes/Makefile create mode 100644 lib/probes/arm/Makefile rename {arch/arm/probes/uprobes => lib/probes/arm}/actions-arm.c (99%) rename {arch/arm/probes => lib/probes/arm}/decode-arm.c (100%) rename {arch/arm/probes => lib/probes/arm}/decode-arm.h (100%) rename {arch/arm/probes => lib/probes/arm}/decode.c (100%) rename {arch/arm/probes => lib/probes/arm}/decode.h (100%) diff --git a/arch/arm/probes/Makefile b/arch/arm/probes/Makefile index 8b0ea5a..9e22de6 100644 --- a/arch/arm/probes/Makefile +++ b/arch/arm/probes/Makefile @@ -1,8 +1,8 @@ # SPDX-License-Identifier: GPL-2.0 -obj-$(CONFIG_UPROBES) += decode.o decode-arm.o uprobes/ -obj-$(CONFIG_KPROBES) += decode.o kprobes/ +subdir-ccflags-y := -I$(srctree)/lib/probes/arm/ + +obj-$(CONFIG_UPROBES) += uprobes/ +obj-$(CONFIG_KPROBES) += kprobes/ ifdef CONFIG_THUMB2_KERNEL obj-$(CONFIG_KPROBES) += decode-thumb.o -else -obj-$(CONFIG_KPROBES) += decode-arm.o endif diff --git a/arch/arm/probes/kprobes/actions-arm.c b/arch/arm/probes/kprobes/actions-arm.c index b9056d6..fb1179e 100644 --- a/arch/arm/probes/kprobes/actions-arm.c +++ b/arch/arm/probes/kprobes/actions-arm.c @@ -62,7 +62,7 @@ #include <linux/kprobes.h> #include <linux/ptrace.h> -#include "../decode-arm.h" +#include "decode-arm.h" #include "core.h" #include "checkers.h" diff --git a/arch/arm/probes/kprobes/checkers-arm.c b/arch/arm/probes/kprobes/checkers-arm.c index 7b98173..29520a2 100644 --- a/arch/arm/probes/kprobes/checkers-arm.c +++ b/arch/arm/probes/kprobes/checkers-arm.c @@ -14,8 +14,8 @@ */ #include <linux/kernel.h> -#include "../decode.h" -#include "../decode-arm.h" +#include "decode.h" +#include "decode-arm.h" #include "checkers.h" static enum probes_insn __kprobes arm_check_stack(probes_opcode_t insn, diff --git a/arch/arm/probes/kprobes/checkers-common.c b/arch/arm/probes/kprobes/checkers-common.c index 971119c..afc3a38 100644 --- a/arch/arm/probes/kprobes/checkers-common.c +++ b/arch/arm/probes/kprobes/checkers-common.c @@ -14,8 +14,8 @@ */ #include <linux/kernel.h> -#include "../decode.h" -#include "../decode-arm.h" +#include "decode.h" +#include "decode-arm.h" #include "checkers.h" enum probes_insn checker_stack_use_none(probes_opcode_t insn, diff --git a/arch/arm/probes/kprobes/checkers-thumb.c b/arch/arm/probes/kprobes/checkers-thumb.c index d608e3b..847f1ee 100644 --- a/arch/arm/probes/kprobes/checkers-thumb.c +++ b/arch/arm/probes/kprobes/checkers-thumb.c @@ -14,7 +14,7 @@ */ #include <linux/kernel.h> -#include "../decode.h" +#include "decode.h" #include "../decode-thumb.h" #include "checkers.h" diff --git a/arch/arm/probes/kprobes/checkers.h b/arch/arm/probes/kprobes/checkers.h index cf6c9e7..9bbab0d 100644 --- a/arch/arm/probes/kprobes/checkers.h +++ b/arch/arm/probes/kprobes/checkers.h @@ -17,7 +17,7 @@ #include <linux/kernel.h> #include <linux/types.h> -#include "../decode.h" +#include "decode.h" extern probes_check_t checker_stack_use_none; extern probes_check_t checker_stack_use_unknown; diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c index f8bd523..de5e854 100644 --- a/arch/arm/probes/kprobes/core.c +++ b/arch/arm/probes/kprobes/core.c @@ -34,7 +34,7 @@ #include <asm/patch.h> #include <asm/sections.h> -#include "../decode-arm.h" +#include "decode-arm.h" #include "../decode-thumb.h" #include "core.h" diff --git a/arch/arm/probes/kprobes/core.h b/arch/arm/probes/kprobes/core.h index ec5d1f2..4da60f0 100644 --- a/arch/arm/probes/kprobes/core.h +++ b/arch/arm/probes/kprobes/core.h @@ -20,7 +20,7 @@ #define _ARM_KERNEL_KPROBES_H #include <asm/kprobes.h> -#include "../decode.h" +#include "decode.h" /* * These undefined instructions must be unique and diff --git a/arch/arm/probes/kprobes/test-core.c b/arch/arm/probes/kprobes/test-core.c index cc237fa..c19df9f 100644 --- a/arch/arm/probes/kprobes/test-core.c +++ b/arch/arm/probes/kprobes/test-core.c @@ -212,7 +212,7 @@ #include "core.h" #include "test-core.h" -#include "../decode-arm.h" +#include "decode-arm.h" #include "../decode-thumb.h" diff --git a/arch/arm/probes/uprobes/Makefile b/arch/arm/probes/uprobes/Makefile index e1dc3d0..0d05ba9 100644 --- a/arch/arm/probes/uprobes/Makefile +++ b/arch/arm/probes/uprobes/Makefile @@ -1 +1 @@ -obj-$(CONFIG_UPROBES) += core.o actions-arm.o +obj-$(CONFIG_UPROBES) += core.o diff --git a/arch/arm/probes/uprobes/core.c b/arch/arm/probes/uprobes/core.c index bf99226..90d0954 100644 --- a/arch/arm/probes/uprobes/core.c +++ b/arch/arm/probes/uprobes/core.c @@ -17,8 +17,8 @@ #include <asm/opcodes.h> #include <asm/traps.h> -#include "../decode.h" -#include "../decode-arm.h" +#include "decode.h" +#include "decode-arm.h" #include "core.h" #define UPROBE_TRAP_NR UINT_MAX diff --git a/lib/Makefile b/lib/Makefile index ca3f7eb..cf2c5d6 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -129,6 +129,8 @@ obj-$(CONFIG_ZSTD_COMPRESS) += zstd/ obj-$(CONFIG_ZSTD_DECOMPRESS) += zstd/ obj-$(CONFIG_XZ_DEC) += xz/ obj-$(CONFIG_RAID6_PQ) += raid6/ +obj-$(CONFIG_UPROBES) += probes/ +obj-$(CONFIG_KPROBES) += probes/ lib-$(CONFIG_DECOMPRESS_GZIP) += decompress_inflate.o lib-$(CONFIG_DECOMPRESS_BZIP2) += decompress_bunzip2.o diff --git a/lib/probes/Makefile b/lib/probes/Makefile new file mode 100644 index 0000000..534a2b7 --- /dev/null +++ b/lib/probes/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_ARM) += arm/ diff --git a/lib/probes/arm/Makefile b/lib/probes/arm/Makefile new file mode 100644 index 0000000..1c97b8f --- /dev/null +++ b/lib/probes/arm/Makefile @@ -0,0 +1,6 @@ +ccflags-y += -I$(srctree)/arch/arm/probes/uprobes/ +obj-$(CONFIG_UPROBES) += decode.o decode-arm.o actions-arm.o +obj-$(CONFIG_KPROBES) += decode.o +ifndef CONFIG_THUMB2_KERNEL +obj-$(CONFIG_KPROBES) += decode-arm.o +endif diff --git a/arch/arm/probes/uprobes/actions-arm.c b/lib/probes/arm/actions-arm.c similarity index 99% rename from arch/arm/probes/uprobes/actions-arm.c rename to lib/probes/arm/actions-arm.c index 76eb449..a25795a 100644 --- a/arch/arm/probes/uprobes/actions-arm.c +++ b/lib/probes/arm/actions-arm.c @@ -13,8 +13,8 @@ #include <linux/uprobes.h> #include <linux/module.h> -#include "../decode.h" -#include "../decode-arm.h" +#include "decode.h" +#include "decode-arm.h" #include "core.h" static int uprobes_substitute_pc(unsigned long *pinsn, u32 oregs) diff --git a/arch/arm/probes/decode-arm.c b/lib/probes/arm/decode-arm.c similarity index 100% rename from arch/arm/probes/decode-arm.c rename to lib/probes/arm/decode-arm.c diff --git a/arch/arm/probes/decode-arm.h b/lib/probes/arm/decode-arm.h similarity index 100% rename from arch/arm/probes/decode-arm.h rename to lib/probes/arm/decode-arm.h diff --git a/arch/arm/probes/decode.c b/lib/probes/arm/decode.c similarity index 100% rename from arch/arm/probes/decode.c rename to lib/probes/arm/decode.c diff --git a/arch/arm/probes/decode.h b/lib/probes/arm/decode.h similarity index 100% rename from arch/arm/probes/decode.h rename to lib/probes/arm/decode.h -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/7] arm64: move arm uprobes code to be reused by arm64 2018-09-26 12:11 ` [PATCH v2 1/7] arm64: move arm uprobes code to be reused by arm64 Maciej Slodczyk @ 2018-09-29 9:37 ` Suzuki K Poulose 2018-10-01 13:12 ` Maciej Slodczyk 0 siblings, 1 reply; 18+ messages in thread From: Suzuki K Poulose @ 2018-09-29 9:37 UTC (permalink / raw) To: Maciej Slodczyk, linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk On 09/26/2018 01:11 PM, Maciej Slodczyk wrote: > Move ARM32 uprobes code from arch/arm/probes/ to a more common location - > lib/probes/arm/. This code will be used by ARM64 code when uprobing 32-bit > applications. The patch looks OK to me, with the following nit: The subject tag looks incorrect. Since this patch moves the ARM code around and has nothing to do with arm64 (yet). So, the subject could be : ARM: move uprobes code to be reused by arm64 which could allow the concerned arm people to check this patch. Suzuki ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/7] arm64: move arm uprobes code to be reused by arm64 2018-09-29 9:37 ` Suzuki K Poulose @ 2018-10-01 13:12 ` Maciej Slodczyk 0 siblings, 0 replies; 18+ messages in thread From: Maciej Slodczyk @ 2018-10-01 13:12 UTC (permalink / raw) To: Suzuki K Poulose, linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk On 29.09.2018 11:37, Suzuki K Poulose wrote: > following nit: > > The subject tag looks incorrect. Since this patch moves the ARM > code around and has nothing to do with arm64 (yet). So, the subject > could be : > > ARM: move uprobes code to be reused by arm64 > > which could allow the concerned arm people to check this patch. Hi, Thank you for the review. I'll adjust subject tags in v3. Thank you, Maciej ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <CGME20180926121214eucas1p1660542d20425551038da8d3feaf7e1b7@eucas1p1.samsung.com>]
* [PATCH v2 2/7] arm64: uprobes - fix checkpatch issues [not found] ` <CGME20180926121214eucas1p1660542d20425551038da8d3feaf7e1b7@eucas1p1.samsung.com> @ 2018-09-26 12:12 ` Maciej Slodczyk 2018-09-29 9:39 ` Suzuki K Poulose 0 siblings, 1 reply; 18+ messages in thread From: Maciej Slodczyk @ 2018-09-26 12:12 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk, m.slodczyk2 Fix checkpatch issues in moved arm uprobes code. Signed-off-by: Maciej Slodczyk <m.slodczyk2@partner.samsung.com> --- lib/probes/arm/actions-arm.c | 4 ++-- lib/probes/arm/decode-arm.c | 1 + lib/probes/arm/decode-arm.h | 4 ++-- lib/probes/arm/decode.c | 6 ++++++ lib/probes/arm/decode.h | 7 +++++-- 5 files changed, 16 insertions(+), 6 deletions(-) diff --git a/lib/probes/arm/actions-arm.c b/lib/probes/arm/actions-arm.c index a25795a..db6017d 100644 --- a/lib/probes/arm/actions-arm.c +++ b/lib/probes/arm/actions-arm.c @@ -165,10 +165,10 @@ uprobe_decode_ldmstm(probes_opcode_t insn, { struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, asi); - unsigned reglist = insn & 0xffff; + unsigned int reglist = insn & 0xffff; int rn = (insn >> 16) & 0xf; int lbit = insn & (1 << 20); - unsigned used = reglist | (1 << rn); + unsigned int used = reglist | (1 << rn); if (rn == 15) return INSN_REJECTED; diff --git a/lib/probes/arm/decode-arm.c b/lib/probes/arm/decode-arm.c index f72c33a..3aa2e58 100644 --- a/lib/probes/arm/decode-arm.c +++ b/lib/probes/arm/decode-arm.c @@ -101,6 +101,7 @@ void __kprobes simulate_mrs(probes_opcode_t insn, { int rd = (insn >> 12) & 0xf; unsigned long mask = 0xf8ff03df; /* Mask out execution state */ + regs->uregs[rd] = regs->ARM_cpsr & mask; } diff --git a/lib/probes/arm/decode-arm.h b/lib/probes/arm/decode-arm.h index b3b80f6..ed34f03 100644 --- a/lib/probes/arm/decode-arm.h +++ b/lib/probes/arm/decode-arm.h @@ -66,8 +66,8 @@ void __kprobes simulate_mov_ipsp(probes_opcode_t opcode, extern const union decode_item probes_decode_arm_table[]; -enum probes_insn arm_probes_decode_insn(probes_opcode_t, - struct arch_probes_insn *, bool emulate, +enum probes_insn arm_probes_decode_insn(probes_opcode_t opcode, + struct arch_probes_insn *api, bool emulate, const union decode_action *actions, const struct decode_checker *checkers[]); diff --git a/lib/probes/arm/decode.c b/lib/probes/arm/decode.c index 880ebe0..6840054 100644 --- a/lib/probes/arm/decode.c +++ b/lib/probes/arm/decode.c @@ -55,6 +55,7 @@ bool load_write_pc_interworks; void __init test_load_write_pc_interworking(void) { int arch = cpu_architecture(); + BUG_ON(arch == CPU_ARCH_UNKNOWN); load_write_pc_interworks = arch >= CPU_ARCH_ARMv5T; } @@ -69,6 +70,7 @@ bool alu_write_pc_interworks; void __init test_alu_write_pc_interworking(void) { int arch = cpu_architecture(); + BUG_ON(arch == CPU_ARCH_UNKNOWN); alu_write_pc_interworks = arch >= CPU_ARCH_ARMv7; } @@ -151,6 +153,7 @@ static unsigned long __kprobes __check_lt(unsigned long cpsr) static unsigned long __kprobes __check_gt(unsigned long cpsr) { unsigned long temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */ + temp |= (cpsr << 1); /* PSR_N_BIT |= PSR_Z_BIT */ return (~temp) & PSR_N_BIT; } @@ -158,6 +161,7 @@ static unsigned long __kprobes __check_gt(unsigned long cpsr) static unsigned long __kprobes __check_le(unsigned long cpsr) { unsigned long temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */ + temp |= (cpsr << 1); /* PSR_N_BIT |= PSR_Z_BIT */ return temp & PSR_N_BIT; } @@ -228,6 +232,7 @@ set_emulated_insn(probes_opcode_t insn, struct arch_probes_insn *asi, #ifdef CONFIG_THUMB2_KERNEL if (thumb) { u16 *ip = (u16 *)asi->insn; + if (is_wide_instruction(insn)) *ip++ = __opcode_to_mem_thumb16(insn >> 16); *ip++ = __opcode_to_mem_thumb16(insn); @@ -465,6 +470,7 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi, case DECODE_TYPE_TABLE: { struct decode_table *d = (struct decode_table *)h; + next = (struct decode_header *)d->table.table; break; } diff --git a/lib/probes/arm/decode.h b/lib/probes/arm/decode.h index 548d622..43b02fd 100644 --- a/lib/probes/arm/decode.h +++ b/lib/probes/arm/decode.h @@ -59,6 +59,7 @@ static inline unsigned long it_advance(unsigned long cpsr) /* We need to shift left ITSTATE<4:0> */ const unsigned long mask = 0x06001c00; /* Mask ITSTATE<4:0> */ unsigned long it = cpsr & mask; + it <<= 1; it |= it >> (27 - 10); /* Carry ITSTATE<2> to correct place */ it &= mask; @@ -71,6 +72,7 @@ static inline unsigned long it_advance(unsigned long cpsr) static inline void __kprobes bx_write_pc(long pcv, struct pt_regs *regs) { long cpsr = regs->ARM_cpsr; + if (pcv & 0x1) { cpsr |= PSR_T_BIT; pcv &= ~0x1; @@ -253,7 +255,8 @@ static inline void __kprobes alu_write_pc(long pcv, struct pt_regs *regs) * instruction will be modified to "AND R0, R2, R3, ASL R1" and then placed into * the kprobes instruction slot. This can then be called later by the handler * function emulate_rd12rn16rm0rs8_rwflags (a pointer to which is retrieved from - * the indicated slot in the action array), in order to simulate the instruction. + * the indicated slot in the action array), in order to simulate the + * instruction. */ enum decode_type { @@ -282,7 +285,7 @@ enum decode_reg_type { REG_TYPE_NOPCWB, /* No PC if load/store write-back flag also set */ /* The following types are used when the encoding for PC indicates - * another instruction form. This distiction only matters for test + * another instruction form. This distinction only matters for test * case coverage checks. */ REG_TYPE_NOPCX, /* Register must not be PC */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/7] arm64: uprobes - fix checkpatch issues 2018-09-26 12:12 ` [PATCH v2 2/7] arm64: uprobes - fix checkpatch issues Maciej Slodczyk @ 2018-09-29 9:39 ` Suzuki K Poulose 0 siblings, 0 replies; 18+ messages in thread From: Suzuki K Poulose @ 2018-09-29 9:39 UTC (permalink / raw) To: Maciej Slodczyk, linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk On 09/26/2018 01:12 PM, Maciej Slodczyk wrote: > Fix checkpatch issues in moved arm uprobes code. > Again the patch doesn't have anything to do with arm64, so wrong tag. Otherwise looks fine. Suzuki ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <CGME20180926121215eucas1p10437d5bd9db81bedbcc363d24d196ded@eucas1p1.samsung.com>]
* [PATCH v2 3/7] arm64: introduce get_swbp_insn() instead of static assignment [not found] ` <CGME20180926121215eucas1p10437d5bd9db81bedbcc363d24d196ded@eucas1p1.samsung.com> @ 2018-09-26 12:12 ` Maciej Slodczyk 0 siblings, 0 replies; 18+ messages in thread From: Maciej Slodczyk @ 2018-09-26 12:12 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk, m.slodczyk2 In uprobes generic code, an arch specific software breakpoint instruction is statically assigned with a #define statement. It does not allow to examine the context and set the proper arch on runtime, which is the case of uprobing either a 32 or 64 bit app on a 64-bit kernel. Introduce get_swbp_insn() with weak attribute that will allow to be redefined in ARM64 uprobes code. Signed-off-by: Maciej Slodczyk <m.slodczyk2@partner.samsung.com> --- include/linux/uprobes.h | 1 + kernel/events/uprobes.c | 12 +++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index bb9d208..8c2fdd9 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -117,6 +117,7 @@ struct uprobes_state { extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr); extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr); +extern uprobe_opcode_t get_swbp_insn(void); extern bool is_swbp_insn(uprobe_opcode_t *insn); extern bool is_trap_insn(uprobe_opcode_t *insn); extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs); diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 3207a4d..a2c3e62 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -216,6 +216,16 @@ static int __replace_page(struct vm_area_struct *vma, unsigned long addr, } /** + * get_swbp_insn - return breakpoint instruction. + * Default implementation of get_swbp_insn + * Returns architecture-dependent breakpoint instruction opcode + */ +uprobe_opcode_t __weak get_swbp_insn(void) +{ + return UPROBE_SWBP_INSN; +} + +/** * is_swbp_insn - check if instruction is breakpoint instruction. * @insn: instruction to be checked. * Default implementation of is_swbp_insn @@ -1181,7 +1191,7 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area) static struct xol_area *__create_xol_area(unsigned long vaddr) { struct mm_struct *mm = current->mm; - uprobe_opcode_t insn = UPROBE_SWBP_INSN; + uprobe_opcode_t insn = get_swbp_insn(); struct xol_area *area; area = kmalloc(sizeof(*area), GFP_KERNEL); -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <CGME20180926121216eucas1p28c13ab1a21ac5ef5058206b92954604f@eucas1p2.samsung.com>]
* [PATCH v2 4/7] arm64: change arm64 probes handler prototype [not found] ` <CGME20180926121216eucas1p28c13ab1a21ac5ef5058206b92954604f@eucas1p2.samsung.com> @ 2018-09-26 12:12 ` Maciej Slodczyk 0 siblings, 0 replies; 18+ messages in thread From: Maciej Slodczyk @ 2018-09-26 12:12 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk, m.slodczyk2 A probes_handler_t() handler function prototype differ between ARM32 and ARM64 arch subtrees. Make ARM64 prototype the same as ARM32 prototype and adjust the ARM64 code to work with the new prototype. Signed-off-by: Maciej Slodczyk <m.slodczyk2@partner.samsung.com> --- arch/arm64/include/asm/probes.h | 6 +++++- arch/arm64/kernel/probes/kprobes.c | 2 +- arch/arm64/kernel/probes/simulate-insn.c | 32 ++++++++++++++++++++++++-------- arch/arm64/kernel/probes/simulate-insn.h | 24 ++++++++++++++++-------- arch/arm64/kernel/probes/uprobes.c | 3 +-- 5 files changed, 47 insertions(+), 20 deletions(-) diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h index 6a5b289..1747e9a 100644 --- a/arch/arm64/include/asm/probes.h +++ b/arch/arm64/include/asm/probes.h @@ -16,7 +16,11 @@ #define _ARM_PROBES_H typedef u32 probe_opcode_t; -typedef void (probes_handler_t) (u32 opcode, long addr, struct pt_regs *); +struct arch_probe_insn; + +typedef void (probes_handler_t) (u32 opcode, + struct arch_probe_insn *api, + struct pt_regs *); /* architecture specific copy of original instruction */ struct arch_probe_insn { diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index e78c3ef..3988967 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -69,7 +69,7 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); if (p->ainsn.api.handler) - p->ainsn.api.handler((u32)p->opcode, (long)p->addr, regs); + p->ainsn.api.handler((u32)p->opcode, &p->ainsn.api, regs); /* single step simulated, now go for post processing */ post_kprobe_handler(kcb, regs); diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c index be05868..22dc7a7 100644 --- a/arch/arm64/kernel/probes/simulate-insn.c +++ b/arch/arm64/kernel/probes/simulate-insn.c @@ -92,9 +92,11 @@ static bool __kprobes check_tbnz(u32 opcode, struct pt_regs *regs) * instruction simulation functions */ void __kprobes -simulate_adr_adrp(u32 opcode, long addr, struct pt_regs *regs) +simulate_adr_adrp(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs) { long imm, xn, val; + long addr = instruction_pointer(regs); xn = opcode & 0x1f; imm = ((opcode >> 3) & 0x1ffffc) | ((opcode >> 29) & 0x3); @@ -110,9 +112,11 @@ simulate_adr_adrp(u32 opcode, long addr, struct pt_regs *regs) } void __kprobes -simulate_b_bl(u32 opcode, long addr, struct pt_regs *regs) +simulate_b_bl(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs) { int disp = bbl_displacement(opcode); + long addr = instruction_pointer(regs); /* Link register is x30 */ if (opcode & (1 << 31)) @@ -122,9 +126,11 @@ simulate_b_bl(u32 opcode, long addr, struct pt_regs *regs) } void __kprobes -simulate_b_cond(u32 opcode, long addr, struct pt_regs *regs) +simulate_b_cond(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs) { int disp = 4; + long addr = instruction_pointer(regs); if (aarch32_opcode_cond_checks[opcode & 0xf](regs->pstate & 0xffffffff)) disp = bcond_displacement(opcode); @@ -133,9 +139,11 @@ simulate_b_cond(u32 opcode, long addr, struct pt_regs *regs) } void __kprobes -simulate_br_blr_ret(u32 opcode, long addr, struct pt_regs *regs) +simulate_br_blr_ret(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs) { int xn = (opcode >> 5) & 0x1f; + long addr = instruction_pointer(regs); /* update pc first in case we're doing a "blr lr" */ instruction_pointer_set(regs, get_x_reg(regs, xn)); @@ -146,9 +154,11 @@ simulate_br_blr_ret(u32 opcode, long addr, struct pt_regs *regs) } void __kprobes -simulate_cbz_cbnz(u32 opcode, long addr, struct pt_regs *regs) +simulate_cbz_cbnz(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs) { int disp = 4; + long addr = instruction_pointer(regs); if (opcode & (1 << 24)) { if (check_cbnz(opcode, regs)) @@ -161,9 +171,11 @@ simulate_cbz_cbnz(u32 opcode, long addr, struct pt_regs *regs) } void __kprobes -simulate_tbz_tbnz(u32 opcode, long addr, struct pt_regs *regs) +simulate_tbz_tbnz(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs) { int disp = 4; + long addr = instruction_pointer(regs); if (opcode & (1 << 24)) { if (check_tbnz(opcode, regs)) @@ -176,11 +188,13 @@ simulate_tbz_tbnz(u32 opcode, long addr, struct pt_regs *regs) } void __kprobes -simulate_ldr_literal(u32 opcode, long addr, struct pt_regs *regs) +simulate_ldr_literal(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs) { u64 *load_addr; int xn = opcode & 0x1f; int disp; + long addr = instruction_pointer(regs); disp = ldr_displacement(opcode); load_addr = (u64 *) (addr + disp); @@ -194,11 +208,13 @@ simulate_ldr_literal(u32 opcode, long addr, struct pt_regs *regs) } void __kprobes -simulate_ldrsw_literal(u32 opcode, long addr, struct pt_regs *regs) +simulate_ldrsw_literal(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs) { s32 *load_addr; int xn = opcode & 0x1f; int disp; + long addr = instruction_pointer(regs); disp = ldr_displacement(opcode); load_addr = (s32 *) (addr + disp); diff --git a/arch/arm64/kernel/probes/simulate-insn.h b/arch/arm64/kernel/probes/simulate-insn.h index 050bde6..31b3840 100644 --- a/arch/arm64/kernel/probes/simulate-insn.h +++ b/arch/arm64/kernel/probes/simulate-insn.h @@ -16,13 +16,21 @@ #ifndef _ARM_KERNEL_KPROBES_SIMULATE_INSN_H #define _ARM_KERNEL_KPROBES_SIMULATE_INSN_H -void simulate_adr_adrp(u32 opcode, long addr, struct pt_regs *regs); -void simulate_b_bl(u32 opcode, long addr, struct pt_regs *regs); -void simulate_b_cond(u32 opcode, long addr, struct pt_regs *regs); -void simulate_br_blr_ret(u32 opcode, long addr, struct pt_regs *regs); -void simulate_cbz_cbnz(u32 opcode, long addr, struct pt_regs *regs); -void simulate_tbz_tbnz(u32 opcode, long addr, struct pt_regs *regs); -void simulate_ldr_literal(u32 opcode, long addr, struct pt_regs *regs); -void simulate_ldrsw_literal(u32 opcode, long addr, struct pt_regs *regs); +void simulate_adr_adrp(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs); +void simulate_b_bl(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs); +void simulate_b_cond(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs); +void simulate_br_blr_ret(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs); +void simulate_cbz_cbnz(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs); +void simulate_tbz_tbnz(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs); +void simulate_ldr_literal(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs); +void simulate_ldrsw_literal(u32 opcode, struct arch_probe_insn *api, + struct pt_regs *regs); #endif /* _ARM_KERNEL_KPROBES_SIMULATE_INSN_H */ diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c index 636ca01..a83642c 100644 --- a/arch/arm64/kernel/probes/uprobes.c +++ b/arch/arm64/kernel/probes/uprobes.c @@ -112,10 +112,9 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) return false; insn = *(probe_opcode_t *)(&auprobe->insn[0]); - addr = instruction_pointer(regs); if (auprobe->api.handler) - auprobe->api.handler(insn, addr, regs); + auprobe->api.handler(insn, &auprobe->api, regs); return true; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <CGME20180926121216eucas1p2b896ce19f49214d497721db9d6e59bfb@eucas1p2.samsung.com>]
* [PATCH v2 5/7] arm64: make arm uprobes code reusable by arm64 [not found] ` <CGME20180926121216eucas1p2b896ce19f49214d497721db9d6e59bfb@eucas1p2.samsung.com> @ 2018-09-26 12:12 ` Maciej Slodczyk 2018-09-27 15:52 ` Julien Thierry 0 siblings, 1 reply; 18+ messages in thread From: Maciej Slodczyk @ 2018-09-26 12:12 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk, m.slodczyk2 There are many segments of ARM32 uprobes code that is very specific to 32-bit ARM arch and many differences between the two architectures that could be made portable (e.g. register numbers). Exclude the ARM32 specific code from ARM64 compilation process and make adjustments in ARM32 uprobes code to be accessible by ARM64 code. Signed-off-by: Maciej Slodczyk <m.slodczyk2@partner.samsung.com> --- arch/arm/include/asm/probes.h | 8 ++ arch/arm/include/asm/ptrace.h | 32 ++++++++ arch/arm/include/asm/uprobes.h | 2 +- arch/arm/probes/uprobes/core.c | 6 +- arch/arm64/include/asm/probes.h | 24 ++++-- arch/arm64/include/asm/ptrace.h | 21 +++++ arch/arm64/include/asm/uprobes.h | 21 ++++- arch/arm64/kernel/probes/Makefile | 2 + arch/arm64/kernel/probes/decode-insn.c | 28 +++---- arch/arm64/kernel/probes/decode-insn.h | 15 ++-- arch/arm64/kernel/probes/kprobes.c | 4 +- arch/arm64/kernel/probes/simulate-insn.c | 16 ++-- arch/arm64/kernel/probes/simulate-insn.h | 16 ++-- arch/arm64/kernel/probes/uprobes.c | 12 +-- lib/probes/Makefile | 1 + lib/probes/arm/Makefile | 5 ++ lib/probes/arm/actions-arm.c | 130 ++++++++++++++++++++++++++++--- lib/probes/arm/decode-arm.c | 47 ++++++----- lib/probes/arm/decode.c | 12 ++- lib/probes/arm/decode.h | 41 ++++++---- 20 files changed, 335 insertions(+), 108 deletions(-) diff --git a/arch/arm/include/asm/probes.h b/arch/arm/include/asm/probes.h index 991c912..b24938f 100644 --- a/arch/arm/include/asm/probes.h +++ b/arch/arm/include/asm/probes.h @@ -21,6 +21,13 @@ #ifndef __ASSEMBLY__ +enum probes_insn { + INSN_REJECTED, + INSN_GOOD_NO_SLOT, + INSN_GOOD, +}; + + typedef u32 probes_opcode_t; struct arch_probes_insn; @@ -45,6 +52,7 @@ struct arch_probes_insn { bool kprobe_direct_exec; }; +extern probes_check_cc * const probes_condition_checks[]; #endif /* __ASSEMBLY__ */ /* diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h index c7cdbb4..99f19f2 100644 --- a/arch/arm/include/asm/ptrace.h +++ b/arch/arm/include/asm/ptrace.h @@ -93,6 +93,8 @@ static inline long regs_return_value(struct pt_regs *regs) } #define instruction_pointer(regs) (regs)->ARM_pc +#define link_register(regs) ((regs)->ARM_lr) +#define state_register(regs) ((regs)->ARM_cpsr) #ifdef CONFIG_THUMB2_KERNEL #define frame_pointer(regs) (regs)->ARM_r7 @@ -106,6 +108,35 @@ static inline void instruction_pointer_set(struct pt_regs *regs, instruction_pointer(regs) = val; } +static inline void link_register_set(struct pt_regs *regs, + unsigned long val) +{ + link_register(regs) = val; +} + +static inline void state_register_set(struct pt_regs *regs, + unsigned long val) +{ + state_register(regs) = val; +} + +/* + * Read a register given an architectural register index r. + */ +static inline unsigned long pt_regs_read_reg(const struct pt_regs *regs, int r) +{ + return regs->uregs[r]; +} + +/* + * Write a register given an architectural register index r. + */ +static inline void pt_regs_write_reg(struct pt_regs *regs, int r, + unsigned long val) +{ + regs->uregs[r] = val; +} + #ifdef CONFIG_SMP extern unsigned long profile_pc(struct pt_regs *regs); #else @@ -167,5 +198,6 @@ static inline unsigned long user_stack_pointer(struct pt_regs *regs) ((current_stack_pointer | (THREAD_SIZE - 1)) - 7) - 1; \ }) +#define ARM_COMPAT_LR_OFFSET 0 #endif /* __ASSEMBLY__ */ #endif diff --git a/arch/arm/include/asm/uprobes.h b/arch/arm/include/asm/uprobes.h index 9472c20..536af7f 100644 --- a/arch/arm/include/asm/uprobes.h +++ b/arch/arm/include/asm/uprobes.h @@ -39,7 +39,7 @@ struct arch_uprobe { void (*posthandler)(struct arch_uprobe *auprobe, struct arch_uprobe_task *autask, struct pt_regs *regs); - struct arch_probes_insn asi; + struct arch_probes_insn api; }; #endif diff --git a/arch/arm/probes/uprobes/core.c b/arch/arm/probes/uprobes/core.c index 90d0954..8abec17 100644 --- a/arch/arm/probes/uprobes/core.c +++ b/arch/arm/probes/uprobes/core.c @@ -38,7 +38,7 @@ int set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, bool arch_uprobe_ignore(struct arch_uprobe *auprobe, struct pt_regs *regs) { - if (!auprobe->asi.insn_check_cc(regs->ARM_cpsr)) { + if (!auprobe->api.insn_check_cc(regs->ARM_cpsr)) { regs->ARM_pc += 4; return true; } @@ -55,7 +55,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) opcode = __mem_to_opcode_arm(*(unsigned int *) auprobe->insn); - auprobe->asi.insn_singlestep(opcode, &auprobe->asi, regs); + auprobe->api.insn_singlestep(opcode, &auprobe->api, regs); return true; } @@ -87,7 +87,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, auprobe->ixol[0] = __opcode_to_mem_arm(insn); auprobe->ixol[1] = __opcode_to_mem_arm(UPROBE_SS_ARM_INSN); - ret = arm_probes_decode_insn(insn, &auprobe->asi, false, + ret = arm_probes_decode_insn(insn, &auprobe->api, false, uprobes_probes_actions, NULL); switch (ret) { case INSN_REJECTED: diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h index 1747e9a..78c0788 100644 --- a/arch/arm64/include/asm/probes.h +++ b/arch/arm64/include/asm/probes.h @@ -15,25 +15,33 @@ #ifndef _ARM_PROBES_H #define _ARM_PROBES_H -typedef u32 probe_opcode_t; -struct arch_probe_insn; +enum probes_insn { + INSN_REJECTED, + INSN_GOOD_NO_SLOT, + INSN_GOOD, +}; + +typedef u32 probes_opcode_t; +struct arch_probes_insn; -typedef void (probes_handler_t) (u32 opcode, - struct arch_probe_insn *api, +typedef void (probes_insn_handler_t) (u32 opcode, + struct arch_probes_insn *api, struct pt_regs *); +typedef unsigned long (probes_check_cc)(unsigned long); + /* architecture specific copy of original instruction */ -struct arch_probe_insn { - probe_opcode_t *insn; +struct arch_probes_insn { + probes_opcode_t *insn; pstate_check_t *pstate_cc; - probes_handler_t *handler; + probes_insn_handler_t *insn_handler; /* restore address after step xol */ unsigned long restore; }; #ifdef CONFIG_KPROBES typedef u32 kprobe_opcode_t; struct arch_specific_insn { - struct arch_probe_insn api; + struct arch_probes_insn api; }; #endif diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index 177b851..a884fd6 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -301,8 +301,29 @@ static inline void procedure_link_pointer_set(struct pt_regs *regs, procedure_link_pointer(regs) = val; } + +#define link_register(regs) ((regs)->compat_lr) + +static inline void link_register_set(struct pt_regs *regs, + unsigned long val) +{ + link_register(regs) = val; +} + +#define state_register(regs) ((regs)->pstate) + +static inline void state_register_set(struct pt_regs *regs, + unsigned long val) +{ + state_register(regs) = val; +} + + #undef profile_pc extern unsigned long profile_pc(struct pt_regs *regs); + +#define ARM_COMPAT_LR_OFFSET 4 + #endif /* __ASSEMBLY__ */ #endif diff --git a/arch/arm64/include/asm/uprobes.h b/arch/arm64/include/asm/uprobes.h index 8d00407..b984634 100644 --- a/arch/arm64/include/asm/uprobes.h +++ b/arch/arm64/include/asm/uprobes.h @@ -22,6 +22,12 @@ typedef u32 uprobe_opcode_t; struct arch_uprobe_task { + u64 backup; +}; + +enum uprobe_arch { + UPROBE_AARCH64, + UPROBE_AARCH32 }; struct arch_uprobe { @@ -29,8 +35,21 @@ struct arch_uprobe { u8 insn[MAX_UINSN_BYTES]; u8 ixol[MAX_UINSN_BYTES]; }; - struct arch_probe_insn api; + + probes_opcode_t orig_insn; + probes_opcode_t bp_insn; + + struct arch_probes_insn api; bool simulate; + u64 pcreg; + enum uprobe_arch arch; + + void (*prehandler)(struct arch_uprobe *auprobe, + struct arch_uprobe_task *autask, + struct pt_regs *regs); + void (*posthandler)(struct arch_uprobe *auprobe, + struct arch_uprobe_task *autask, + struct pt_regs *regs); }; #endif diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile index 8e4be92..c5d57e3 100644 --- a/arch/arm64/kernel/probes/Makefile +++ b/arch/arm64/kernel/probes/Makefile @@ -1,4 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 +ccflags-y += -I$(srctree)/lib/probes/arm/ +ccflags-y += -I$(srctree)/arch/arm64/kernel/probes/ obj-$(CONFIG_KPROBES) += kprobes.o decode-insn.o \ kprobes_trampoline.o \ simulate-insn.o diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c index 6bf6657..a0597a2 100644 --- a/arch/arm64/kernel/probes/decode-insn.c +++ b/arch/arm64/kernel/probes/decode-insn.c @@ -77,8 +77,8 @@ static bool __kprobes aarch64_insn_is_steppable(u32 insn) * INSN_GOOD If instruction is supported and uses instruction slot, * INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot. */ -enum probe_insn __kprobes -arm_probe_decode_insn(probe_opcode_t insn, struct arch_probe_insn *api) +enum probes_insn __kprobes +arm_probe_decode_insn(probes_opcode_t insn, struct arch_probes_insn *api) { /* * Instructions reading or modifying the PC won't work from the XOL @@ -88,26 +88,26 @@ arm_probe_decode_insn(probe_opcode_t insn, struct arch_probe_insn *api) return INSN_GOOD; if (aarch64_insn_is_bcond(insn)) { - api->handler = simulate_b_cond; + api->insn_handler = simulate_b_cond; } else if (aarch64_insn_is_cbz(insn) || aarch64_insn_is_cbnz(insn)) { - api->handler = simulate_cbz_cbnz; + api->insn_handler = simulate_cbz_cbnz; } else if (aarch64_insn_is_tbz(insn) || aarch64_insn_is_tbnz(insn)) { - api->handler = simulate_tbz_tbnz; + api->insn_handler = simulate_tbz_tbnz; } else if (aarch64_insn_is_adr_adrp(insn)) { - api->handler = simulate_adr_adrp; + api->insn_handler = simulate_adr_adrp; } else if (aarch64_insn_is_b(insn) || aarch64_insn_is_bl(insn)) { - api->handler = simulate_b_bl; + api->insn_handler = simulate_b_bl; } else if (aarch64_insn_is_br(insn) || aarch64_insn_is_blr(insn) || aarch64_insn_is_ret(insn)) { - api->handler = simulate_br_blr_ret; + api->insn_handler = simulate_br_blr_ret; } else if (aarch64_insn_is_ldr_lit(insn)) { - api->handler = simulate_ldr_literal; + api->insn_handler = simulate_ldr_literal; } else if (aarch64_insn_is_ldrsw_lit(insn)) { - api->handler = simulate_ldrsw_literal; + api->insn_handler = simulate_ldrsw_literal; } else { /* * Instruction cannot be stepped out-of-line and we don't @@ -138,12 +138,12 @@ is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end) return false; } -enum probe_insn __kprobes +enum probes_insn __kprobes arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi) { - enum probe_insn decoded; - probe_opcode_t insn = le32_to_cpu(*addr); - probe_opcode_t *scan_end = NULL; + enum probes_insn decoded; + probes_opcode_t insn = le32_to_cpu(*addr); + probes_opcode_t *scan_end = NULL; unsigned long size = 0, offset = 0; /* diff --git a/arch/arm64/kernel/probes/decode-insn.h b/arch/arm64/kernel/probes/decode-insn.h index 192ab00..93e021f 100644 --- a/arch/arm64/kernel/probes/decode-insn.h +++ b/arch/arm64/kernel/probes/decode-insn.h @@ -16,7 +16,9 @@ #ifndef _ARM_KERNEL_KPROBES_ARM64_H #define _ARM_KERNEL_KPROBES_ARM64_H +#include <asm/probes.h> #include <asm/kprobes.h> +#include "decode.h" /* * ARM strongly recommends a limit of 128 bytes between LoadExcl and @@ -25,17 +27,12 @@ */ #define MAX_ATOMIC_CONTEXT_SIZE (128 / sizeof(kprobe_opcode_t)) -enum probe_insn { - INSN_REJECTED, - INSN_GOOD_NO_SLOT, - INSN_GOOD, -}; - #ifdef CONFIG_KPROBES -enum probe_insn __kprobes +enum probes_insn __kprobes arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi); #endif -enum probe_insn __kprobes -arm_probe_decode_insn(probe_opcode_t insn, struct arch_probe_insn *asi); +enum probes_insn __kprobes +arm_probe_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi); +extern const union decode_action uprobes_probes_actions[]; #endif /* _ARM_KERNEL_KPROBES_ARM64_H */ diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index 3988967..6b392e1 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -68,8 +68,8 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) { struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); - if (p->ainsn.api.handler) - p->ainsn.api.handler((u32)p->opcode, &p->ainsn.api, regs); + if (p->ainsn.api.insn_handler) + p->ainsn.api.insn_handler((u32)p->opcode, &p->ainsn.api, regs); /* single step simulated, now go for post processing */ post_kprobe_handler(kcb, regs); diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c index 22dc7a7..9898c41 100644 --- a/arch/arm64/kernel/probes/simulate-insn.c +++ b/arch/arm64/kernel/probes/simulate-insn.c @@ -92,7 +92,7 @@ static bool __kprobes check_tbnz(u32 opcode, struct pt_regs *regs) * instruction simulation functions */ void __kprobes -simulate_adr_adrp(u32 opcode, struct arch_probe_insn *api, +simulate_adr_adrp(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs) { long imm, xn, val; @@ -112,7 +112,7 @@ simulate_adr_adrp(u32 opcode, struct arch_probe_insn *api, } void __kprobes -simulate_b_bl(u32 opcode, struct arch_probe_insn *api, +simulate_b_bl(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs) { int disp = bbl_displacement(opcode); @@ -126,7 +126,7 @@ simulate_b_bl(u32 opcode, struct arch_probe_insn *api, } void __kprobes -simulate_b_cond(u32 opcode, struct arch_probe_insn *api, +simulate_b_cond(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs) { int disp = 4; @@ -139,7 +139,7 @@ simulate_b_cond(u32 opcode, struct arch_probe_insn *api, } void __kprobes -simulate_br_blr_ret(u32 opcode, struct arch_probe_insn *api, +simulate_br_blr_ret(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs) { int xn = (opcode >> 5) & 0x1f; @@ -154,7 +154,7 @@ simulate_br_blr_ret(u32 opcode, struct arch_probe_insn *api, } void __kprobes -simulate_cbz_cbnz(u32 opcode, struct arch_probe_insn *api, +simulate_cbz_cbnz(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs) { int disp = 4; @@ -171,7 +171,7 @@ simulate_cbz_cbnz(u32 opcode, struct arch_probe_insn *api, } void __kprobes -simulate_tbz_tbnz(u32 opcode, struct arch_probe_insn *api, +simulate_tbz_tbnz(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs) { int disp = 4; @@ -188,7 +188,7 @@ simulate_tbz_tbnz(u32 opcode, struct arch_probe_insn *api, } void __kprobes -simulate_ldr_literal(u32 opcode, struct arch_probe_insn *api, +simulate_ldr_literal(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs) { u64 *load_addr; @@ -208,7 +208,7 @@ simulate_ldr_literal(u32 opcode, struct arch_probe_insn *api, } void __kprobes -simulate_ldrsw_literal(u32 opcode, struct arch_probe_insn *api, +simulate_ldrsw_literal(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs) { s32 *load_addr; diff --git a/arch/arm64/kernel/probes/simulate-insn.h b/arch/arm64/kernel/probes/simulate-insn.h index 31b3840..822c3c4 100644 --- a/arch/arm64/kernel/probes/simulate-insn.h +++ b/arch/arm64/kernel/probes/simulate-insn.h @@ -16,21 +16,21 @@ #ifndef _ARM_KERNEL_KPROBES_SIMULATE_INSN_H #define _ARM_KERNEL_KPROBES_SIMULATE_INSN_H -void simulate_adr_adrp(u32 opcode, struct arch_probe_insn *api, +void simulate_adr_adrp(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs); -void simulate_b_bl(u32 opcode, struct arch_probe_insn *api, +void simulate_b_bl(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs); -void simulate_b_cond(u32 opcode, struct arch_probe_insn *api, +void simulate_b_cond(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs); -void simulate_br_blr_ret(u32 opcode, struct arch_probe_insn *api, +void simulate_br_blr_ret(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs); -void simulate_cbz_cbnz(u32 opcode, struct arch_probe_insn *api, +void simulate_cbz_cbnz(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs); -void simulate_tbz_tbnz(u32 opcode, struct arch_probe_insn *api, +void simulate_tbz_tbnz(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs); -void simulate_ldr_literal(u32 opcode, struct arch_probe_insn *api, +void simulate_ldr_literal(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs); -void simulate_ldrsw_literal(u32 opcode, struct arch_probe_insn *api, +void simulate_ldrsw_literal(u32 opcode, struct arch_probes_insn *api, struct pt_regs *regs); #endif /* _ARM_KERNEL_KPROBES_SIMULATE_INSN_H */ diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c index a83642c..f99b50b 100644 --- a/arch/arm64/kernel/probes/uprobes.c +++ b/arch/arm64/kernel/probes/uprobes.c @@ -37,7 +37,7 @@ unsigned long uprobe_get_swbp_addr(struct pt_regs *regs) int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long addr) { - probe_opcode_t insn; + probes_opcode_t insn; /* TODO: Currently we do not support AARCH32 instruction probing */ if (mm->context.flags & MMCF_AARCH32) @@ -45,7 +45,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, else if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) return -EINVAL; - insn = *(probe_opcode_t *)(&auprobe->insn[0]); + insn = *(probes_opcode_t *)(&auprobe->insn[0]); switch (arm_probe_decode_insn(insn, &auprobe->api)) { case INSN_REJECTED: @@ -105,16 +105,16 @@ bool arch_uprobe_xol_was_trapped(struct task_struct *t) bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) { - probe_opcode_t insn; + probes_opcode_t insn; unsigned long addr; if (!auprobe->simulate) return false; - insn = *(probe_opcode_t *)(&auprobe->insn[0]); + insn = *(probes_opcode_t *)(&auprobe->insn[0]); - if (auprobe->api.handler) - auprobe->api.handler(insn, &auprobe->api, regs); + if (auprobe->api.insn_handler) + auprobe->api.insn_handler(insn, &auprobe->api, regs); return true; } diff --git a/lib/probes/Makefile b/lib/probes/Makefile index 534a2b7..682e2df 100644 --- a/lib/probes/Makefile +++ b/lib/probes/Makefile @@ -1 +1,2 @@ obj-$(CONFIG_ARM) += arm/ +obj-$(CONFIG_ARM64) += arm/ diff --git a/lib/probes/arm/Makefile b/lib/probes/arm/Makefile index 1c97b8f..239deac 100644 --- a/lib/probes/arm/Makefile +++ b/lib/probes/arm/Makefile @@ -1,4 +1,9 @@ +ccflags-y += -I$(srctree)/lib/uprobes/arm/ +ifdef CONFIG_ARM64 +ccflags-y += -I$(srctree)/arch/arm64/kernel/probes/ +else ccflags-y += -I$(srctree)/arch/arm/probes/uprobes/ +endif obj-$(CONFIG_UPROBES) += decode.o decode-arm.o actions-arm.o obj-$(CONFIG_KPROBES) += decode.o ifndef CONFIG_THUMB2_KERNEL diff --git a/lib/probes/arm/actions-arm.c b/lib/probes/arm/actions-arm.c index db6017d..2393573 100644 --- a/lib/probes/arm/actions-arm.c +++ b/lib/probes/arm/actions-arm.c @@ -15,7 +15,10 @@ #include "decode.h" #include "decode-arm.h" -#include "core.h" + +#ifdef CONFIG_ARM64 +#include <../../../arm/include/asm/opcodes.h> +#endif /* CONFIG_ARM64 */ static int uprobes_substitute_pc(unsigned long *pinsn, u32 oregs) { @@ -72,8 +75,8 @@ static void uprobe_set_pc(struct arch_uprobe *auprobe, { u32 pcreg = auprobe->pcreg; - autask->backup = regs->uregs[pcreg]; - regs->uregs[pcreg] = regs->ARM_pc + 8; + autask->backup = pt_regs_read_reg(regs, pcreg); + pt_regs_write_reg(regs, pcreg, instruction_pointer(regs) + 8); } static void uprobe_unset_pc(struct arch_uprobe *auprobe, @@ -81,7 +84,7 @@ static void uprobe_unset_pc(struct arch_uprobe *auprobe, struct pt_regs *regs) { /* PC will be taken care of by common code */ - regs->uregs[auprobe->pcreg] = autask->backup; + pt_regs_write_reg(regs, auprobe->pcreg, autask->backup); } static void uprobe_aluwrite_pc(struct arch_uprobe *auprobe, @@ -90,8 +93,8 @@ static void uprobe_aluwrite_pc(struct arch_uprobe *auprobe, { u32 pcreg = auprobe->pcreg; - alu_write_pc(regs->uregs[pcreg], regs); - regs->uregs[pcreg] = autask->backup; + alu_write_pc(pt_regs_read_reg(regs, pcreg), regs); + pt_regs_write_reg(regs, pcreg, autask->backup); } static void uprobe_write_pc(struct arch_uprobe *auprobe, @@ -100,8 +103,8 @@ static void uprobe_write_pc(struct arch_uprobe *auprobe, { u32 pcreg = auprobe->pcreg; - load_write_pc(regs->uregs[pcreg], regs); - regs->uregs[pcreg] = autask->backup; + load_write_pc(pt_regs_read_reg(regs, pcreg), regs); + pt_regs_write_reg(regs, pcreg, autask->backup); } enum probes_insn @@ -109,12 +112,13 @@ decode_pc_ro(probes_opcode_t insn, struct arch_probes_insn *asi, const struct decode_header *d) { struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, - asi); + api); + struct decode_emulate *decode = (struct decode_emulate *) d; u32 regs = decode->header.type_regs.bits >> DECODE_TYPE_BITS; int reg; - reg = uprobes_substitute_pc(&auprobe->ixol[0], regs); + reg = uprobes_substitute_pc((unsigned long *)&auprobe->ixol[0], regs); if (reg == 15) return INSN_GOOD; @@ -133,7 +137,8 @@ decode_wb_pc(probes_opcode_t insn, struct arch_probes_insn *asi, const struct decode_header *d, bool alu) { struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, - asi); + api); + enum probes_insn ret = decode_pc_ro(insn, asi, d); if (((insn >> 12) & 0xf) == 15) @@ -158,13 +163,110 @@ decode_ldr(probes_opcode_t insn, struct arch_probes_insn *asi, return decode_wb_pc(insn, asi, d, false); } +/* + * based on arm kprobes implementation + */ +static void __kprobes simulate_ldm1stm1(probes_opcode_t insn, + struct arch_probes_insn *asi, + struct pt_regs *regs) +{ + int rn = (insn >> 16) & 0xf; + int lbit = insn & (1 << 20); + int wbit = insn & (1 << 21); + int ubit = insn & (1 << 23); + int pbit = insn & (1 << 24); + int reg; + u32 *addr = (u32 *)pt_regs_read_reg(regs, rn); + unsigned int reg_bit_vector; + unsigned int reg_count; + + reg_count = 0; + reg_bit_vector = insn & 0xffff; + + while (reg_bit_vector) { + reg_bit_vector &= (reg_bit_vector - 1); + ++reg_count; + } + if (!ubit) + addr -= reg_count; + addr += (!pbit == !ubit); + + reg_bit_vector = insn & 0xffff; + + while (reg_bit_vector) { + reg = __ffs(reg_bit_vector); + reg_bit_vector &= (reg_bit_vector - 1); + if (lbit) { /* LDM */ + if (reg == 15) + instruction_pointer_set(regs, (*addr++) - 4); + else + pt_regs_write_reg(regs, reg, *addr++); + } else { /* STM */ + if (reg == 15) + *addr++ = instruction_pointer(regs); + else + *addr++ = pt_regs_read_reg(regs, reg); + } + } + + /* write back new value of Rn */ + if (wbit) { + if (!ubit) + addr -= reg_count; + addr -= (!pbit == !ubit); + if (rn == 15) + instruction_pointer_set(regs, (long)addr); + else + pt_regs_write_reg(regs, rn, (long)addr); + } + + instruction_pointer_set(regs, instruction_pointer(regs) + 4); +} + +static void __kprobes simulate_stm1_pc(probes_opcode_t insn, + struct arch_probes_insn *asi, + struct pt_regs *regs) +{ + unsigned long addr = instruction_pointer(regs) - 4; + + instruction_pointer_set(regs, (long)addr + str_pc_offset); + simulate_ldm1stm1(insn, asi, regs); + instruction_pointer_set(regs, (long)addr + 4); +} + +static void __kprobes simulate_ldm1_pc(probes_opcode_t insn, + struct arch_probes_insn *asi, + struct pt_regs *regs) +{ + simulate_ldm1stm1(insn, asi, regs); + load_write_pc(instruction_pointer(regs), regs); +} + +enum probes_insn +uprobe_decode_ldmstm_aarch64(probes_opcode_t insn, + struct arch_probes_insn *asi, + const struct decode_header *d) +{ + probes_insn_handler_t *handler = 0; + unsigned int reglist = insn & 0xffff; + int is_ldm = insn & 0x100000; + + /* PC on a reglist? */ + if (reglist & 0x8000) + handler = is_ldm ? simulate_ldm1_pc : simulate_stm1_pc; + else + handler = simulate_ldm1stm1; + asi->insn_handler = handler; + return INSN_GOOD_NO_SLOT; +} + enum probes_insn uprobe_decode_ldmstm(probes_opcode_t insn, struct arch_probes_insn *asi, const struct decode_header *d) { struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, - asi); + api); unsigned int reglist = insn & 0xffff; int rn = (insn >> 16) & 0xf; int lbit = insn & (1 << 20); @@ -228,5 +330,9 @@ const union decode_action uprobes_probes_actions[] = { [PROBES_MUL_ADD] = {.handler = probes_simulate_nop}, [PROBES_BITFIELD] = {.handler = probes_simulate_nop}, [PROBES_BRANCH] = {.handler = simulate_bbl}, +#ifdef CONFIG_ARM64 + [PROBES_LDMSTM] = {.decoder = uprobe_decode_ldmstm_aarch64} +#else [PROBES_LDMSTM] = {.decoder = uprobe_decode_ldmstm} +#endif }; diff --git a/lib/probes/arm/decode-arm.c b/lib/probes/arm/decode-arm.c index 3aa2e58..36eb939 100644 --- a/lib/probes/arm/decode-arm.c +++ b/lib/probes/arm/decode-arm.c @@ -61,39 +61,45 @@ void __kprobes simulate_bbl(probes_opcode_t insn, struct arch_probes_insn *asi, struct pt_regs *regs) { - long iaddr = (long) regs->ARM_pc - 4; - int disp = branch_displacement(insn); + long iaddr = (long) instruction_pointer(regs) + ARM_COMPAT_LR_OFFSET; + int disp = branch_displacement(insn); if (insn & (1 << 24)) - regs->ARM_lr = iaddr + 4; + link_register_set(regs, iaddr); - regs->ARM_pc = iaddr + 8 + disp; + instruction_pointer_set(regs, iaddr + 4 + disp); } void __kprobes simulate_blx1(probes_opcode_t insn, struct arch_probes_insn *asi, struct pt_regs *regs) { - long iaddr = (long) regs->ARM_pc - 4; + long iaddr = (long) instruction_pointer(regs) + ARM_COMPAT_LR_OFFSET; int disp = branch_displacement(insn); + long cpsr; - regs->ARM_lr = iaddr + 4; - regs->ARM_pc = iaddr + 8 + disp + ((insn >> 23) & 0x2); - regs->ARM_cpsr |= PSR_T_BIT; + link_register_set(regs, iaddr); + instruction_pointer_set(regs, iaddr + 4 + disp + ((insn >> 23) & 0x2)); + cpsr = state_register(regs) | PSR_T_BIT; + state_register_set(regs, cpsr); } void __kprobes simulate_blx2bx(probes_opcode_t insn, struct arch_probes_insn *asi, struct pt_regs *regs) { int rm = insn & 0xf; - long rmv = regs->uregs[rm]; + long rmv = pt_regs_read_reg(regs, rm); + long cpsr; if (insn & (1 << 5)) - regs->ARM_lr = (long) regs->ARM_pc; - - regs->ARM_pc = rmv & ~0x1; - regs->ARM_cpsr &= ~PSR_T_BIT; - if (rmv & 0x1) - regs->ARM_cpsr |= PSR_T_BIT; + link_register_set(regs, (long) instruction_pointer(regs)); + + instruction_pointer_set(regs, rmv & ~0x1); + cpsr = state_register(regs) & ~PSR_T_BIT; + state_register_set(regs, cpsr); + if (rmv & 0x1) { + cpsr = state_register(regs) | PSR_T_BIT; + state_register_set(regs, cpsr); + } } void __kprobes simulate_mrs(probes_opcode_t insn, @@ -102,13 +108,13 @@ void __kprobes simulate_mrs(probes_opcode_t insn, int rd = (insn >> 12) & 0xf; unsigned long mask = 0xf8ff03df; /* Mask out execution state */ - regs->uregs[rd] = regs->ARM_cpsr & mask; + pt_regs_write_reg(regs, rd, state_register(regs) & mask); } void __kprobes simulate_mov_ipsp(probes_opcode_t insn, struct arch_probes_insn *asi, struct pt_regs *regs) { - regs->uregs[12] = regs->uregs[13]; + pt_regs_write_reg(regs, 12, pt_regs_read_reg(regs, 13)); } /* @@ -246,13 +252,14 @@ static const union decode_item arm_cccc_0000_____1001_table[] = { static const union decode_item arm_cccc_0001_____1001_table[] = { /* Synchronization primitives */ - +#ifndef CONFIG_ARM64 #if __LINUX_ARM_ARCH__ < 6 /* Deprecated on ARMv6 and may be UNDEFINED on v7 */ /* SMP/SWPB cccc 0001 0x00 xxxx xxxx xxxx 1001 xxxx */ DECODE_EMULATEX (0x0fb000f0, 0x01000090, PROBES_SWP, REGS(NOPC, NOPC, 0, 0, NOPC)), #endif +#endif /* CONFIG_ARM64 */ /* LDREX/STREX{,D,B,H} cccc 0001 1xxx xxxx xxxx xxxx 1001 xxxx */ /* And unallocated instructions... */ DECODE_END @@ -709,7 +716,7 @@ EXPORT_SYMBOL_GPL(probes_decode_arm_table); static void __kprobes arm_singlestep(probes_opcode_t insn, struct arch_probes_insn *asi, struct pt_regs *regs) { - regs->ARM_pc += 4; + instruction_pointer_set(regs, instruction_pointer(regs) + 4); asi->insn_handler(insn, asi, regs); } @@ -730,8 +737,10 @@ arm_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi, bool emulate, const union decode_action *actions, const struct decode_checker *checkers[]) { +#ifndef CONFIG_ARM64 asi->insn_singlestep = arm_singlestep; asi->insn_check_cc = probes_condition_checks[insn>>28]; +#endif return probes_decode_insn(insn, asi, probes_decode_arm_table, false, emulate, actions, checkers); } diff --git a/lib/probes/arm/decode.c b/lib/probes/arm/decode.c index 6840054..7a31042 100644 --- a/lib/probes/arm/decode.c +++ b/lib/probes/arm/decode.c @@ -13,13 +13,18 @@ #include <linux/kernel.h> #include <linux/types.h> +#ifdef CONFIG_ARM64 +#include <asm/insn.h> +#include <../../../arm/include/asm/opcodes.h> +#else /* CONFIG_ARM64 */ #include <asm/system_info.h> #include <asm/ptrace.h> +#endif /* CONFIG_ARM64 */ #include <linux/bug.h> - #include "decode.h" + #ifndef find_str_pc_offset /* @@ -189,7 +194,9 @@ void __kprobes probes_emulate_none(probes_opcode_t opcode, struct arch_probes_insn *asi, struct pt_regs *regs) { +#ifndef CONFIG_ARM64 asi->insn_fn(); +#endif /* CONFIG_ARM64 */ } /* @@ -430,6 +437,7 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi, */ probes_opcode_t origin_insn = insn; +#ifndef CONFIG_ARM64 /* * stack_space is initialized to 0 here. Checker functions * should update is value if they find this is a stack store @@ -446,7 +454,7 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi, * registers are used', to prevent any potential optimization. */ asi->register_usage_flags = ~0UL; - +#endif /* CONFIG_ARM64 */ if (emulate) insn = prepare_emulated_insn(insn, asi, thumb); diff --git a/lib/probes/arm/decode.h b/lib/probes/arm/decode.h index 43b02fd..b1fd9ab 100644 --- a/lib/probes/arm/decode.h +++ b/lib/probes/arm/decode.h @@ -23,10 +23,20 @@ #include <linux/stddef.h> #include <asm/probes.h> #include <asm/kprobes.h> +#ifdef CONFIG_ARM64 +#include <asm/ptrace.h> +#include <asm/insn.h> -void __init arm_probes_decode_init(void); +#define PSR_T_BIT PSR_AA32_T_BIT + +#define str_pc_offset 8 +#define load_write_pc_interworks true +#define alu_write_pc_interworks true +#define find_str_pc_offset() +#define test_load_write_pc_interworking() +#define test_alu_write_pc_interworking() -extern probes_check_cc * const probes_condition_checks[16]; +#else /* CONFIG_ARM64 */ #if __LINUX_ARM_ARCH__ >= 7 @@ -40,8 +50,7 @@ extern probes_check_cc * const probes_condition_checks[16]; extern int str_pc_offset; void __init find_str_pc_offset(void); -#endif - +#endif /* __LINUX_ARM_ARCH__ */ /* * Update ITSTATE after normal execution of an IT block instruction. @@ -69,9 +78,13 @@ static inline unsigned long it_advance(unsigned long cpsr) return cpsr; } +#endif /* CONFIG_ARM64 */ + +void __init arm_probes_decode_init(void); + static inline void __kprobes bx_write_pc(long pcv, struct pt_regs *regs) { - long cpsr = regs->ARM_cpsr; + long cpsr = state_register(regs); if (pcv & 0x1) { cpsr |= PSR_T_BIT; @@ -80,11 +93,11 @@ static inline void __kprobes bx_write_pc(long pcv, struct pt_regs *regs) cpsr &= ~PSR_T_BIT; pcv &= ~0x2; /* Avoid UNPREDICTABLE address allignment */ } - regs->ARM_cpsr = cpsr; - regs->ARM_pc = pcv; + state_register_set(regs, cpsr); + instruction_pointer_set(regs, pcv); } - +#ifndef CONFIG_ARM64 #if __LINUX_ARM_ARCH__ >= 6 /* Kernels built for >= ARMv6 should never run on <= ARMv5 hardware, so... */ @@ -98,16 +111,18 @@ extern bool load_write_pc_interworks; void __init test_load_write_pc_interworking(void); #endif +#endif /* CONFIG_ARM64 */ static inline void __kprobes load_write_pc(long pcv, struct pt_regs *regs) { if (load_write_pc_interworks) bx_write_pc(pcv, regs); else - regs->ARM_pc = pcv; + instruction_pointer_set(regs, pcv); } +#ifndef CONFIG_ARM64 #if __LINUX_ARM_ARCH__ >= 7 #define alu_write_pc_interworks true @@ -126,13 +141,14 @@ extern bool alu_write_pc_interworks; void __init test_alu_write_pc_interworking(void); #endif /* __LINUX_ARM_ARCH__ == 6 */ +#endif /* CONFIG_ARM64 */ static inline void __kprobes alu_write_pc(long pcv, struct pt_regs *regs) { if (alu_write_pc_interworks) bx_write_pc(pcv, regs); else - regs->ARM_pc = pcv; + instruction_pointer_set(regs, pcv); } @@ -395,11 +411,6 @@ struct decode_or { #define DECODE_OR(_mask, _value) \ DECODE_HEADER(DECODE_TYPE_OR, _mask, _value, 0) -enum probes_insn { - INSN_REJECTED, - INSN_GOOD, - INSN_GOOD_NO_SLOT -}; struct decode_reject { struct decode_header header; -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/7] arm64: make arm uprobes code reusable by arm64 2018-09-26 12:12 ` [PATCH v2 5/7] arm64: make arm uprobes code reusable by arm64 Maciej Slodczyk @ 2018-09-27 15:52 ` Julien Thierry 2018-10-01 13:28 ` Maciej Slodczyk 0 siblings, 1 reply; 18+ messages in thread From: Julien Thierry @ 2018-09-27 15:52 UTC (permalink / raw) To: Maciej Slodczyk, linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk Hi Maciej, I think that it would be good to move the renaming changes out of this patch. On 26/09/18 13:12, Maciej Slodczyk wrote: > There are many segments of ARM32 uprobes code that is very specific to > 32-bit ARM arch and many differences between the two architectures that > could be made portable (e.g. register numbers). Exclude the ARM32 specific > code from ARM64 compilation process and make adjustments in ARM32 uprobes > code to be accessible by ARM64 code. > > Signed-off-by: Maciej Slodczyk <m.slodczyk2@partner.samsung.com> > --- > arch/arm/include/asm/probes.h | 8 ++ > arch/arm/include/asm/ptrace.h | 32 ++++++++ > arch/arm/include/asm/uprobes.h | 2 +- > arch/arm/probes/uprobes/core.c | 6 +- > arch/arm64/include/asm/probes.h | 24 ++++-- > arch/arm64/include/asm/ptrace.h | 21 +++++ > arch/arm64/include/asm/uprobes.h | 21 ++++- > arch/arm64/kernel/probes/Makefile | 2 + > arch/arm64/kernel/probes/decode-insn.c | 28 +++---- > arch/arm64/kernel/probes/decode-insn.h | 15 ++-- > arch/arm64/kernel/probes/kprobes.c | 4 +- > arch/arm64/kernel/probes/simulate-insn.c | 16 ++-- > arch/arm64/kernel/probes/simulate-insn.h | 16 ++-- > arch/arm64/kernel/probes/uprobes.c | 12 +-- > lib/probes/Makefile | 1 + > lib/probes/arm/Makefile | 5 ++ > lib/probes/arm/actions-arm.c | 130 ++++++++++++++++++++++++++++--- > lib/probes/arm/decode-arm.c | 47 ++++++----- > lib/probes/arm/decode.c | 12 ++- > lib/probes/arm/decode.h | 41 ++++++---- > 20 files changed, 335 insertions(+), 108 deletions(-) > > diff --git a/arch/arm/include/asm/probes.h b/arch/arm/include/asm/probes.h > index 991c912..b24938f 100644 > --- a/arch/arm/include/asm/probes.h > +++ b/arch/arm/include/asm/probes.h > @@ -21,6 +21,13 @@ > > #ifndef __ASSEMBLY__ > > +enum probes_insn { > + INSN_REJECTED, > + INSN_GOOD_NO_SLOT, > + INSN_GOOD, > +}; > + > + > typedef u32 probes_opcode_t; > > struct arch_probes_insn; > @@ -45,6 +52,7 @@ struct arch_probes_insn { > bool kprobe_direct_exec; > }; > > +extern probes_check_cc * const probes_condition_checks[]; > #endif /* __ASSEMBLY__ */ > > /* > diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h > index c7cdbb4..99f19f2 100644 > --- a/arch/arm/include/asm/ptrace.h > +++ b/arch/arm/include/asm/ptrace.h > @@ -93,6 +93,8 @@ static inline long regs_return_value(struct pt_regs *regs) > } > > #define instruction_pointer(regs) (regs)->ARM_pc > +#define link_register(regs) ((regs)->ARM_lr) ^ here you have a space > +#define state_register(regs) ((regs)->ARM_cpsr) ^ here you have a tab, you probably want a space here > > #ifdef CONFIG_THUMB2_KERNEL > #define frame_pointer(regs) (regs)->ARM_r7 > @@ -106,6 +108,35 @@ static inline void instruction_pointer_set(struct pt_regs *regs, > instruction_pointer(regs) = val; > } > > +static inline void link_register_set(struct pt_regs *regs, > + unsigned long val) > +{ > + link_register(regs) = val; > +} > + > +static inline void state_register_set(struct pt_regs *regs, > + unsigned long val) > +{ > + state_register(regs) = val; > +} > + > +/* > + * Read a register given an architectural register index r. > + */ > +static inline unsigned long pt_regs_read_reg(const struct pt_regs *regs, int r) > +{ > + return regs->uregs[r]; > +} > + > +/* > + * Write a register given an architectural register index r. > + */ > +static inline void pt_regs_write_reg(struct pt_regs *regs, int r, > + unsigned long val) > +{ > + regs->uregs[r] = val; > +} > + > #ifdef CONFIG_SMP > extern unsigned long profile_pc(struct pt_regs *regs); > #else > @@ -167,5 +198,6 @@ static inline unsigned long user_stack_pointer(struct pt_regs *regs) > ((current_stack_pointer | (THREAD_SIZE - 1)) - 7) - 1; \ > }) > > +#define ARM_COMPAT_LR_OFFSET 0 Not sure this should be defined here. What's the meaning of compat for arch/arm ? > #endif /* __ASSEMBLY__ */ > #endif > diff --git a/arch/arm/include/asm/uprobes.h b/arch/arm/include/asm/uprobes.h > index 9472c20..536af7f 100644 > --- a/arch/arm/include/asm/uprobes.h > +++ b/arch/arm/include/asm/uprobes.h > @@ -39,7 +39,7 @@ struct arch_uprobe { > void (*posthandler)(struct arch_uprobe *auprobe, > struct arch_uprobe_task *autask, > struct pt_regs *regs); > - struct arch_probes_insn asi; > + struct arch_probes_insn api; It would be easier to follow thing by making this change in its own patch. (Probably before you move arm32 code to lib/probes) > }; > > #endif > diff --git a/arch/arm/probes/uprobes/core.c b/arch/arm/probes/uprobes/core.c > index 90d0954..8abec17 100644 > --- a/arch/arm/probes/uprobes/core.c > +++ b/arch/arm/probes/uprobes/core.c > @@ -38,7 +38,7 @@ int set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, > > bool arch_uprobe_ignore(struct arch_uprobe *auprobe, struct pt_regs *regs) > { > - if (!auprobe->asi.insn_check_cc(regs->ARM_cpsr)) { > + if (!auprobe->api.insn_check_cc(regs->ARM_cpsr)) { > regs->ARM_pc += 4; > return true; > } > @@ -55,7 +55,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) > > opcode = __mem_to_opcode_arm(*(unsigned int *) auprobe->insn); > > - auprobe->asi.insn_singlestep(opcode, &auprobe->asi, regs); > + auprobe->api.insn_singlestep(opcode, &auprobe->api, regs); > > return true; > } > @@ -87,7 +87,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > auprobe->ixol[0] = __opcode_to_mem_arm(insn); > auprobe->ixol[1] = __opcode_to_mem_arm(UPROBE_SS_ARM_INSN); > > - ret = arm_probes_decode_insn(insn, &auprobe->asi, false, > + ret = arm_probes_decode_insn(insn, &auprobe->api, false, > uprobes_probes_actions, NULL); > switch (ret) { > case INSN_REJECTED: > diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h > index 1747e9a..78c0788 100644 > --- a/arch/arm64/include/asm/probes.h > +++ b/arch/arm64/include/asm/probes.h > @@ -15,25 +15,33 @@ > #ifndef _ARM_PROBES_H > #define _ARM_PROBES_H > > -typedef u32 probe_opcode_t; > -struct arch_probe_insn; > +enum probes_insn { > + INSN_REJECTED, > + INSN_GOOD_NO_SLOT, > + INSN_GOOD, > +}; Why have two definitions of this enum rather than a common one in lib/probes? > + > +typedef u32 probes_opcode_t; > +struct arch_probes_insn; > > -typedef void (probes_handler_t) (u32 opcode, > - struct arch_probe_insn *api, > +typedef void (probes_insn_handler_t) (u32 opcode, > + struct arch_probes_insn *api, In the previous patch you were already aligning this handler the ARM32's equivalent. Why not fix the name (for the handler and struct arch_probes_insn) in the previous patch? > struct pt_regs *); > > +typedef unsigned long (probes_check_cc)(unsigned long); > + > /* architecture specific copy of original instruction */ > -struct arch_probe_insn { > - probe_opcode_t *insn; > +struct arch_probes_insn { > + probes_opcode_t *insn; > pstate_check_t *pstate_cc; > - probes_handler_t *handler; > + probes_insn_handler_t *insn_handler; > /* restore address after step xol */ > unsigned long restore; > }; > #ifdef CONFIG_KPROBES > typedef u32 kprobe_opcode_t; > struct arch_specific_insn { > - struct arch_probe_insn api; > + struct arch_probes_insn api; > }; > #endif > > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h > index 177b851..a884fd6 100644 > --- a/arch/arm64/include/asm/ptrace.h > +++ b/arch/arm64/include/asm/ptrace.h > @@ -301,8 +301,29 @@ static inline void procedure_link_pointer_set(struct pt_regs *regs, > procedure_link_pointer(regs) = val; > } > > + > +#define link_register(regs) ((regs)->compat_lr) > + > +static inline void link_register_set(struct pt_regs *regs, > + unsigned long val) > +{ > + link_register(regs) = val; > +} pstate.h isn't really related to compat mode and whichever compat definition it contains the relations are made explicit through their names. I don't think a macro "link_register" defined in arch/arm64 and visible to any file including ptrace.h (which is a lot) should return "compat_lr" instead of the actual link register. I'd say have the link_register macro check whether "regs" refers to a compat mode context or not and provide the adequate link register. Otherwise maybe you can get away with naming the macro "arm_link_register" and the macro "arm_link_register_set". But I would prefer the previous approach. > + > +#define state_register(regs) ((regs)->pstate) > + > +static inline void state_register_set(struct pt_regs *regs, > + unsigned long val) > +{ > + state_register(regs) = val; > +} > + > + > #undef profile_pc > extern unsigned long profile_pc(struct pt_regs *regs); > > + > +#define ARM_COMPAT_LR_OFFSET 4 > + > #endif /* __ASSEMBLY__ */ > #endif > diff --git a/arch/arm64/include/asm/uprobes.h b/arch/arm64/include/asm/uprobes.h > index 8d00407..b984634 100644 > --- a/arch/arm64/include/asm/uprobes.h > +++ b/arch/arm64/include/asm/uprobes.h > @@ -22,6 +22,12 @@ > typedef u32 uprobe_opcode_t; > > struct arch_uprobe_task { > + u64 backup; > +}; > + > +enum uprobe_arch { > + UPROBE_AARCH64, > + UPROBE_AARCH32 > }; > > struct arch_uprobe { > @@ -29,8 +35,21 @@ struct arch_uprobe { > u8 insn[MAX_UINSN_BYTES]; > u8 ixol[MAX_UINSN_BYTES]; > }; > - struct arch_probe_insn api; > + > + probes_opcode_t orig_insn; > + probes_opcode_t bp_insn; > + > + struct arch_probes_insn api; > bool simulate; > + u64 pcreg; > + enum uprobe_arch arch; > + > + void (*prehandler)(struct arch_uprobe *auprobe, > + struct arch_uprobe_task *autask, > + struct pt_regs *regs); > + void (*posthandler)(struct arch_uprobe *auprobe, > + struct arch_uprobe_task *autask, > + struct pt_regs *regs); > }; > > #endif > diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile > index 8e4be92..c5d57e3 100644 > --- a/arch/arm64/kernel/probes/Makefile > +++ b/arch/arm64/kernel/probes/Makefile > @@ -1,4 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0 > +ccflags-y += -I$(srctree)/lib/probes/arm/ > +ccflags-y += -I$(srctree)/arch/arm64/kernel/probes/ > obj-$(CONFIG_KPROBES) += kprobes.o decode-insn.o \ > kprobes_trampoline.o \ > simulate-insn.o > diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c > index 6bf6657..a0597a2 100644 > --- a/arch/arm64/kernel/probes/decode-insn.c > +++ b/arch/arm64/kernel/probes/decode-insn.c > @@ -77,8 +77,8 @@ static bool __kprobes aarch64_insn_is_steppable(u32 insn) > * INSN_GOOD If instruction is supported and uses instruction slot, > * INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot. > */ > -enum probe_insn __kprobes > -arm_probe_decode_insn(probe_opcode_t insn, struct arch_probe_insn *api) > +enum probes_insn __kprobes > +arm_probe_decode_insn(probes_opcode_t insn, struct arch_probes_insn *api) > { > /* > * Instructions reading or modifying the PC won't work from the XOL > @@ -88,26 +88,26 @@ arm_probe_decode_insn(probe_opcode_t insn, struct arch_probe_insn *api) > return INSN_GOOD; > > if (aarch64_insn_is_bcond(insn)) { > - api->handler = simulate_b_cond; > + api->insn_handler = simulate_b_cond; > } else if (aarch64_insn_is_cbz(insn) || > aarch64_insn_is_cbnz(insn)) { > - api->handler = simulate_cbz_cbnz; > + api->insn_handler = simulate_cbz_cbnz; > } else if (aarch64_insn_is_tbz(insn) || > aarch64_insn_is_tbnz(insn)) { > - api->handler = simulate_tbz_tbnz; > + api->insn_handler = simulate_tbz_tbnz; > } else if (aarch64_insn_is_adr_adrp(insn)) { > - api->handler = simulate_adr_adrp; > + api->insn_handler = simulate_adr_adrp; > } else if (aarch64_insn_is_b(insn) || > aarch64_insn_is_bl(insn)) { > - api->handler = simulate_b_bl; > + api->insn_handler = simulate_b_bl; > } else if (aarch64_insn_is_br(insn) || > aarch64_insn_is_blr(insn) || > aarch64_insn_is_ret(insn)) { > - api->handler = simulate_br_blr_ret; > + api->insn_handler = simulate_br_blr_ret; > } else if (aarch64_insn_is_ldr_lit(insn)) { > - api->handler = simulate_ldr_literal; > + api->insn_handler = simulate_ldr_literal; > } else if (aarch64_insn_is_ldrsw_lit(insn)) { > - api->handler = simulate_ldrsw_literal; > + api->insn_handler = simulate_ldrsw_literal; > } else { > /* > * Instruction cannot be stepped out-of-line and we don't > @@ -138,12 +138,12 @@ is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end) > return false; > } > > -enum probe_insn __kprobes > +enum probes_insn __kprobes > arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi) > { > - enum probe_insn decoded; > - probe_opcode_t insn = le32_to_cpu(*addr); > - probe_opcode_t *scan_end = NULL; > + enum probes_insn decoded; > + probes_opcode_t insn = le32_to_cpu(*addr); > + probes_opcode_t *scan_end = NULL; > unsigned long size = 0, offset = 0; > > /* > diff --git a/arch/arm64/kernel/probes/decode-insn.h b/arch/arm64/kernel/probes/decode-insn.h > index 192ab00..93e021f 100644 > --- a/arch/arm64/kernel/probes/decode-insn.h > +++ b/arch/arm64/kernel/probes/decode-insn.h > @@ -16,7 +16,9 @@ > #ifndef _ARM_KERNEL_KPROBES_ARM64_H > #define _ARM_KERNEL_KPROBES_ARM64_H > > +#include <asm/probes.h> > #include <asm/kprobes.h> > +#include "decode.h" > > /* > * ARM strongly recommends a limit of 128 bytes between LoadExcl and > @@ -25,17 +27,12 @@ > */ > #define MAX_ATOMIC_CONTEXT_SIZE (128 / sizeof(kprobe_opcode_t)) > > -enum probe_insn { > - INSN_REJECTED, > - INSN_GOOD_NO_SLOT, > - INSN_GOOD, > -}; > - > #ifdef CONFIG_KPROBES > -enum probe_insn __kprobes > +enum probes_insn __kprobes > arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi); > #endif > -enum probe_insn __kprobes > -arm_probe_decode_insn(probe_opcode_t insn, struct arch_probe_insn *asi); > +enum probes_insn __kprobes > +arm_probe_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi); > > +extern const union decode_action uprobes_probes_actions[]; > #endif /* _ARM_KERNEL_KPROBES_ARM64_H */ > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c > index 3988967..6b392e1 100644 > --- a/arch/arm64/kernel/probes/kprobes.c > +++ b/arch/arm64/kernel/probes/kprobes.c > @@ -68,8 +68,8 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) > { > struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); > > - if (p->ainsn.api.handler) > - p->ainsn.api.handler((u32)p->opcode, &p->ainsn.api, regs); > + if (p->ainsn.api.insn_handler) > + p->ainsn.api.insn_handler((u32)p->opcode, &p->ainsn.api, regs); > > /* single step simulated, now go for post processing */ > post_kprobe_handler(kcb, regs); > diff --git a/arch/arm64/kernel/probes/simulate-insn.c b/arch/arm64/kernel/probes/simulate-insn.c > index 22dc7a7..9898c41 100644 > --- a/arch/arm64/kernel/probes/simulate-insn.c > +++ b/arch/arm64/kernel/probes/simulate-insn.c > @@ -92,7 +92,7 @@ static bool __kprobes check_tbnz(u32 opcode, struct pt_regs *regs) > * instruction simulation functions > */ > void __kprobes > -simulate_adr_adrp(u32 opcode, struct arch_probe_insn *api, > +simulate_adr_adrp(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs) > { > long imm, xn, val; > @@ -112,7 +112,7 @@ simulate_adr_adrp(u32 opcode, struct arch_probe_insn *api, > } > > void __kprobes > -simulate_b_bl(u32 opcode, struct arch_probe_insn *api, > +simulate_b_bl(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs) > { > int disp = bbl_displacement(opcode); > @@ -126,7 +126,7 @@ simulate_b_bl(u32 opcode, struct arch_probe_insn *api, > } > > void __kprobes > -simulate_b_cond(u32 opcode, struct arch_probe_insn *api, > +simulate_b_cond(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs) > { > int disp = 4; > @@ -139,7 +139,7 @@ simulate_b_cond(u32 opcode, struct arch_probe_insn *api, > } > > void __kprobes > -simulate_br_blr_ret(u32 opcode, struct arch_probe_insn *api, > +simulate_br_blr_ret(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs) > { > int xn = (opcode >> 5) & 0x1f; > @@ -154,7 +154,7 @@ simulate_br_blr_ret(u32 opcode, struct arch_probe_insn *api, > } > > void __kprobes > -simulate_cbz_cbnz(u32 opcode, struct arch_probe_insn *api, > +simulate_cbz_cbnz(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs) > { > int disp = 4; > @@ -171,7 +171,7 @@ simulate_cbz_cbnz(u32 opcode, struct arch_probe_insn *api, > } > > void __kprobes > -simulate_tbz_tbnz(u32 opcode, struct arch_probe_insn *api, > +simulate_tbz_tbnz(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs) > { > int disp = 4; > @@ -188,7 +188,7 @@ simulate_tbz_tbnz(u32 opcode, struct arch_probe_insn *api, > } > > void __kprobes > -simulate_ldr_literal(u32 opcode, struct arch_probe_insn *api, > +simulate_ldr_literal(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs) > { > u64 *load_addr; > @@ -208,7 +208,7 @@ simulate_ldr_literal(u32 opcode, struct arch_probe_insn *api, > } > > void __kprobes > -simulate_ldrsw_literal(u32 opcode, struct arch_probe_insn *api, > +simulate_ldrsw_literal(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs) > { > s32 *load_addr; > diff --git a/arch/arm64/kernel/probes/simulate-insn.h b/arch/arm64/kernel/probes/simulate-insn.h > index 31b3840..822c3c4 100644 > --- a/arch/arm64/kernel/probes/simulate-insn.h > +++ b/arch/arm64/kernel/probes/simulate-insn.h > @@ -16,21 +16,21 @@ > #ifndef _ARM_KERNEL_KPROBES_SIMULATE_INSN_H > #define _ARM_KERNEL_KPROBES_SIMULATE_INSN_H > > -void simulate_adr_adrp(u32 opcode, struct arch_probe_insn *api, > +void simulate_adr_adrp(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs); > -void simulate_b_bl(u32 opcode, struct arch_probe_insn *api, > +void simulate_b_bl(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs); > -void simulate_b_cond(u32 opcode, struct arch_probe_insn *api, > +void simulate_b_cond(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs); > -void simulate_br_blr_ret(u32 opcode, struct arch_probe_insn *api, > +void simulate_br_blr_ret(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs); > -void simulate_cbz_cbnz(u32 opcode, struct arch_probe_insn *api, > +void simulate_cbz_cbnz(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs); > -void simulate_tbz_tbnz(u32 opcode, struct arch_probe_insn *api, > +void simulate_tbz_tbnz(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs); > -void simulate_ldr_literal(u32 opcode, struct arch_probe_insn *api, > +void simulate_ldr_literal(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs); > -void simulate_ldrsw_literal(u32 opcode, struct arch_probe_insn *api, > +void simulate_ldrsw_literal(u32 opcode, struct arch_probes_insn *api, > struct pt_regs *regs); > > #endif /* _ARM_KERNEL_KPROBES_SIMULATE_INSN_H */ > diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c > index a83642c..f99b50b 100644 > --- a/arch/arm64/kernel/probes/uprobes.c > +++ b/arch/arm64/kernel/probes/uprobes.c > @@ -37,7 +37,7 @@ unsigned long uprobe_get_swbp_addr(struct pt_regs *regs) > int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > unsigned long addr) > { > - probe_opcode_t insn; > + probes_opcode_t insn; > > /* TODO: Currently we do not support AARCH32 instruction probing */ > if (mm->context.flags & MMCF_AARCH32) > @@ -45,7 +45,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > else if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) > return -EINVAL; > > - insn = *(probe_opcode_t *)(&auprobe->insn[0]); > + insn = *(probes_opcode_t *)(&auprobe->insn[0]); > > switch (arm_probe_decode_insn(insn, &auprobe->api)) { > case INSN_REJECTED: > @@ -105,16 +105,16 @@ bool arch_uprobe_xol_was_trapped(struct task_struct *t) > > bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) > { > - probe_opcode_t insn; > + probes_opcode_t insn; > unsigned long addr; > > if (!auprobe->simulate) > return false; > > - insn = *(probe_opcode_t *)(&auprobe->insn[0]); > + insn = *(probes_opcode_t *)(&auprobe->insn[0]); > > - if (auprobe->api.handler) > - auprobe->api.handler(insn, &auprobe->api, regs); > + if (auprobe->api.insn_handler) > + auprobe->api.insn_handler(insn, &auprobe->api, regs); > > return true; > } > diff --git a/lib/probes/Makefile b/lib/probes/Makefile > index 534a2b7..682e2df 100644 > --- a/lib/probes/Makefile > +++ b/lib/probes/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_ARM) += arm/ > +obj-$(CONFIG_ARM64) += arm/ > diff --git a/lib/probes/arm/Makefile b/lib/probes/arm/Makefile > index 1c97b8f..239deac 100644 > --- a/lib/probes/arm/Makefile > +++ b/lib/probes/arm/Makefile > @@ -1,4 +1,9 @@ > +ccflags-y += -I$(srctree)/lib/uprobes/arm/ > +ifdef CONFIG_ARM64 > +ccflags-y += -I$(srctree)/arch/arm64/kernel/probes/ > +else > ccflags-y += -I$(srctree)/arch/arm/probes/uprobes/ > +endif > obj-$(CONFIG_UPROBES) += decode.o decode-arm.o actions-arm.o > obj-$(CONFIG_KPROBES) += decode.o > ifndef CONFIG_THUMB2_KERNEL > diff --git a/lib/probes/arm/actions-arm.c b/lib/probes/arm/actions-arm.c > index db6017d..2393573 100644 > --- a/lib/probes/arm/actions-arm.c > +++ b/lib/probes/arm/actions-arm.c > @@ -15,7 +15,10 @@ > > #include "decode.h" > #include "decode-arm.h" > -#include "core.h" > + > +#ifdef CONFIG_ARM64 > +#include <../../../arm/include/asm/opcodes.h> Hmmm not sure this is something that is accepted. > +#endif /* CONFIG_ARM64 */ > > static int uprobes_substitute_pc(unsigned long *pinsn, u32 oregs) > { > @@ -72,8 +75,8 @@ static void uprobe_set_pc(struct arch_uprobe *auprobe, > { > u32 pcreg = auprobe->pcreg; > > - autask->backup = regs->uregs[pcreg]; > - regs->uregs[pcreg] = regs->ARM_pc + 8; > + autask->backup = pt_regs_read_reg(regs, pcreg); > + pt_regs_write_reg(regs, pcreg, instruction_pointer(regs) + 8); > } > > static void uprobe_unset_pc(struct arch_uprobe *auprobe, > @@ -81,7 +84,7 @@ static void uprobe_unset_pc(struct arch_uprobe *auprobe, > struct pt_regs *regs) > { > /* PC will be taken care of by common code */ > - regs->uregs[auprobe->pcreg] = autask->backup; > + pt_regs_write_reg(regs, auprobe->pcreg, autask->backup); > } > > static void uprobe_aluwrite_pc(struct arch_uprobe *auprobe, > @@ -90,8 +93,8 @@ static void uprobe_aluwrite_pc(struct arch_uprobe *auprobe, > { > u32 pcreg = auprobe->pcreg; > > - alu_write_pc(regs->uregs[pcreg], regs); > - regs->uregs[pcreg] = autask->backup; > + alu_write_pc(pt_regs_read_reg(regs, pcreg), regs); > + pt_regs_write_reg(regs, pcreg, autask->backup); > } > > static void uprobe_write_pc(struct arch_uprobe *auprobe, > @@ -100,8 +103,8 @@ static void uprobe_write_pc(struct arch_uprobe *auprobe, > { > u32 pcreg = auprobe->pcreg; > > - load_write_pc(regs->uregs[pcreg], regs); > - regs->uregs[pcreg] = autask->backup; > + load_write_pc(pt_regs_read_reg(regs, pcreg), regs); > + pt_regs_write_reg(regs, pcreg, autask->backup); > } > > enum probes_insn > @@ -109,12 +112,13 @@ decode_pc_ro(probes_opcode_t insn, struct arch_probes_insn *asi, > const struct decode_header *d) > { > struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, > - asi); > + api); > + > struct decode_emulate *decode = (struct decode_emulate *) d; > u32 regs = decode->header.type_regs.bits >> DECODE_TYPE_BITS; > int reg; > > - reg = uprobes_substitute_pc(&auprobe->ixol[0], regs); > + reg = uprobes_substitute_pc((unsigned long *)&auprobe->ixol[0], regs); > if (reg == 15) > return INSN_GOOD; > > @@ -133,7 +137,8 @@ decode_wb_pc(probes_opcode_t insn, struct arch_probes_insn *asi, > const struct decode_header *d, bool alu) > { > struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, > - asi); > + api); > + > enum probes_insn ret = decode_pc_ro(insn, asi, d); > > if (((insn >> 12) & 0xf) == 15) > @@ -158,13 +163,110 @@ decode_ldr(probes_opcode_t insn, struct arch_probes_insn *asi, > return decode_wb_pc(insn, asi, d, false); > } > > +/* > + * based on arm kprobes implementation > + */ > +static void __kprobes simulate_ldm1stm1(probes_opcode_t insn, > + struct arch_probes_insn *asi, The whole asi/api mix become a bit confusing IMO. Should we have api when the argument is of type "arch_probes_insn" and asi when the type is "arch_specific_insn"? Should we have more coherent definitions of those structures between arm and arm64 if we are going to share functions between them? > + struct pt_regs *regs) > +{ > + int rn = (insn >> 16) & 0xf; > + int lbit = insn & (1 << 20); > + int wbit = insn & (1 << 21); > + int ubit = insn & (1 << 23); > + int pbit = insn & (1 << 24); > + int reg; > + u32 *addr = (u32 *)pt_regs_read_reg(regs, rn); > + unsigned int reg_bit_vector; > + unsigned int reg_count; > + > + reg_count = 0; > + reg_bit_vector = insn & 0xffff; > + > + while (reg_bit_vector) { > + reg_bit_vector &= (reg_bit_vector - 1); > + ++reg_count; > + } > + if (!ubit) > + addr -= reg_count; > + addr += (!pbit == !ubit); > + > + reg_bit_vector = insn & 0xffff; > + > + while (reg_bit_vector) { > + reg = __ffs(reg_bit_vector); > + reg_bit_vector &= (reg_bit_vector - 1); > + if (lbit) { /* LDM */ > + if (reg == 15) > + instruction_pointer_set(regs, (*addr++) - 4); > + else > + pt_regs_write_reg(regs, reg, *addr++); > + } else { /* STM */ > + if (reg == 15) > + *addr++ = instruction_pointer(regs); > + else > + *addr++ = pt_regs_read_reg(regs, reg); > + } > + } > + > + /* write back new value of Rn */ > + if (wbit) { > + if (!ubit) > + addr -= reg_count; > + addr -= (!pbit == !ubit); > + if (rn == 15) > + instruction_pointer_set(regs, (long)addr); > + else > + pt_regs_write_reg(regs, rn, (long)addr); > + } > + > + instruction_pointer_set(regs, instruction_pointer(regs) + 4); > +} > + > +static void __kprobes simulate_stm1_pc(probes_opcode_t insn, > + struct arch_probes_insn *asi, > + struct pt_regs *regs) > +{ > + unsigned long addr = instruction_pointer(regs) - 4; > + > + instruction_pointer_set(regs, (long)addr + str_pc_offset); > + simulate_ldm1stm1(insn, asi, regs); > + instruction_pointer_set(regs, (long)addr + 4); > +} > + > +static void __kprobes simulate_ldm1_pc(probes_opcode_t insn, > + struct arch_probes_insn *asi, > + struct pt_regs *regs) > +{ > + simulate_ldm1stm1(insn, asi, regs); > + load_write_pc(instruction_pointer(regs), regs); > +} > + #ifdef CONFIG_ARM64 > +enum probes_insn > +uprobe_decode_ldmstm_aarch64(probes_opcode_t insn, > + struct arch_probes_insn *asi, > + const struct decode_header *d) Should be static. > +{ > + probes_insn_handler_t *handler = 0; > + unsigned int reglist = insn & 0xffff; > + int is_ldm = insn & 0x100000; > + > + /* PC on a reglist? */ > + if (reglist & 0x8000) > + handler = is_ldm ? simulate_ldm1_pc : simulate_stm1_pc; > + else > + handler = simulate_ldm1stm1; > + asi->insn_handler = handler; > + return INSN_GOOD_NO_SLOT; > +} > + #endif #endofreview Cheers, Julien > enum probes_insn > uprobe_decode_ldmstm(probes_opcode_t insn, > struct arch_probes_insn *asi, > const struct decode_header *d) > { > struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, > - asi); > + api); > unsigned int reglist = insn & 0xffff; > int rn = (insn >> 16) & 0xf; > int lbit = insn & (1 << 20); > @@ -228,5 +330,9 @@ const union decode_action uprobes_probes_actions[] = { > [PROBES_MUL_ADD] = {.handler = probes_simulate_nop}, > [PROBES_BITFIELD] = {.handler = probes_simulate_nop}, > [PROBES_BRANCH] = {.handler = simulate_bbl}, > +#ifdef CONFIG_ARM64 > + [PROBES_LDMSTM] = {.decoder = uprobe_decode_ldmstm_aarch64} > +#else > [PROBES_LDMSTM] = {.decoder = uprobe_decode_ldmstm} > +#endif > }; > diff --git a/lib/probes/arm/decode-arm.c b/lib/probes/arm/decode-arm.c > index 3aa2e58..36eb939 100644 > --- a/lib/probes/arm/decode-arm.c > +++ b/lib/probes/arm/decode-arm.c > @@ -61,39 +61,45 @@ > void __kprobes simulate_bbl(probes_opcode_t insn, > struct arch_probes_insn *asi, struct pt_regs *regs) > { > - long iaddr = (long) regs->ARM_pc - 4; > - int disp = branch_displacement(insn); > + long iaddr = (long) instruction_pointer(regs) + ARM_COMPAT_LR_OFFSET; > + int disp = branch_displacement(insn); > > if (insn & (1 << 24)) > - regs->ARM_lr = iaddr + 4; > + link_register_set(regs, iaddr); > > - regs->ARM_pc = iaddr + 8 + disp; > + instruction_pointer_set(regs, iaddr + 4 + disp); > } > > void __kprobes simulate_blx1(probes_opcode_t insn, > struct arch_probes_insn *asi, struct pt_regs *regs) > { > - long iaddr = (long) regs->ARM_pc - 4; > + long iaddr = (long) instruction_pointer(regs) + ARM_COMPAT_LR_OFFSET; > int disp = branch_displacement(insn); > + long cpsr; > > - regs->ARM_lr = iaddr + 4; > - regs->ARM_pc = iaddr + 8 + disp + ((insn >> 23) & 0x2); > - regs->ARM_cpsr |= PSR_T_BIT; > + link_register_set(regs, iaddr); > + instruction_pointer_set(regs, iaddr + 4 + disp + ((insn >> 23) & 0x2)); > + cpsr = state_register(regs) | PSR_T_BIT; > + state_register_set(regs, cpsr); > } > > void __kprobes simulate_blx2bx(probes_opcode_t insn, > struct arch_probes_insn *asi, struct pt_regs *regs) > { > int rm = insn & 0xf; > - long rmv = regs->uregs[rm]; > + long rmv = pt_regs_read_reg(regs, rm); > + long cpsr; > > if (insn & (1 << 5)) > - regs->ARM_lr = (long) regs->ARM_pc; > - > - regs->ARM_pc = rmv & ~0x1; > - regs->ARM_cpsr &= ~PSR_T_BIT; > - if (rmv & 0x1) > - regs->ARM_cpsr |= PSR_T_BIT; > + link_register_set(regs, (long) instruction_pointer(regs)); > + > + instruction_pointer_set(regs, rmv & ~0x1); > + cpsr = state_register(regs) & ~PSR_T_BIT; > + state_register_set(regs, cpsr); > + if (rmv & 0x1) { > + cpsr = state_register(regs) | PSR_T_BIT; > + state_register_set(regs, cpsr); > + } > } > > void __kprobes simulate_mrs(probes_opcode_t insn, > @@ -102,13 +108,13 @@ void __kprobes simulate_mrs(probes_opcode_t insn, > int rd = (insn >> 12) & 0xf; > unsigned long mask = 0xf8ff03df; /* Mask out execution state */ > > - regs->uregs[rd] = regs->ARM_cpsr & mask; > + pt_regs_write_reg(regs, rd, state_register(regs) & mask); > } > > void __kprobes simulate_mov_ipsp(probes_opcode_t insn, > struct arch_probes_insn *asi, struct pt_regs *regs) > { > - regs->uregs[12] = regs->uregs[13]; > + pt_regs_write_reg(regs, 12, pt_regs_read_reg(regs, 13)); > } > > /* > @@ -246,13 +252,14 @@ static const union decode_item arm_cccc_0000_____1001_table[] = { > > static const union decode_item arm_cccc_0001_____1001_table[] = { > /* Synchronization primitives */ > - > +#ifndef CONFIG_ARM64 > #if __LINUX_ARM_ARCH__ < 6 > /* Deprecated on ARMv6 and may be UNDEFINED on v7 */ > /* SMP/SWPB cccc 0001 0x00 xxxx xxxx xxxx 1001 xxxx */ > DECODE_EMULATEX (0x0fb000f0, 0x01000090, PROBES_SWP, > REGS(NOPC, NOPC, 0, 0, NOPC)), > #endif > +#endif /* CONFIG_ARM64 */ > /* LDREX/STREX{,D,B,H} cccc 0001 1xxx xxxx xxxx xxxx 1001 xxxx */ > /* And unallocated instructions... */ > DECODE_END > @@ -709,7 +716,7 @@ EXPORT_SYMBOL_GPL(probes_decode_arm_table); > static void __kprobes arm_singlestep(probes_opcode_t insn, > struct arch_probes_insn *asi, struct pt_regs *regs) > { > - regs->ARM_pc += 4; > + instruction_pointer_set(regs, instruction_pointer(regs) + 4); > asi->insn_handler(insn, asi, regs); > } > > @@ -730,8 +737,10 @@ arm_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi, > bool emulate, const union decode_action *actions, > const struct decode_checker *checkers[]) > { > +#ifndef CONFIG_ARM64 > asi->insn_singlestep = arm_singlestep; > asi->insn_check_cc = probes_condition_checks[insn>>28]; > +#endif > return probes_decode_insn(insn, asi, probes_decode_arm_table, false, > emulate, actions, checkers); > } > diff --git a/lib/probes/arm/decode.c b/lib/probes/arm/decode.c > index 6840054..7a31042 100644 > --- a/lib/probes/arm/decode.c > +++ b/lib/probes/arm/decode.c > @@ -13,13 +13,18 @@ > > #include <linux/kernel.h> > #include <linux/types.h> > +#ifdef CONFIG_ARM64 > +#include <asm/insn.h> > +#include <../../../arm/include/asm/opcodes.h> > +#else /* CONFIG_ARM64 */ > #include <asm/system_info.h> > #include <asm/ptrace.h> > +#endif /* CONFIG_ARM64 */ > #include <linux/bug.h> > - > #include "decode.h" > > > + > #ifndef find_str_pc_offset > > /* > @@ -189,7 +194,9 @@ void __kprobes probes_emulate_none(probes_opcode_t opcode, > struct arch_probes_insn *asi, > struct pt_regs *regs) > { > +#ifndef CONFIG_ARM64 > asi->insn_fn(); > +#endif /* CONFIG_ARM64 */ > } > > /* > @@ -430,6 +437,7 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi, > */ > probes_opcode_t origin_insn = insn; > > +#ifndef CONFIG_ARM64 > /* > * stack_space is initialized to 0 here. Checker functions > * should update is value if they find this is a stack store > @@ -446,7 +454,7 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi, > * registers are used', to prevent any potential optimization. > */ > asi->register_usage_flags = ~0UL; > - > +#endif /* CONFIG_ARM64 */ > if (emulate) > insn = prepare_emulated_insn(insn, asi, thumb); > > diff --git a/lib/probes/arm/decode.h b/lib/probes/arm/decode.h > index 43b02fd..b1fd9ab 100644 > --- a/lib/probes/arm/decode.h > +++ b/lib/probes/arm/decode.h > @@ -23,10 +23,20 @@ > #include <linux/stddef.h> > #include <asm/probes.h> > #include <asm/kprobes.h> > +#ifdef CONFIG_ARM64 > +#include <asm/ptrace.h> > +#include <asm/insn.h> > > -void __init arm_probes_decode_init(void); > +#define PSR_T_BIT PSR_AA32_T_BIT > + > +#define str_pc_offset 8 > +#define load_write_pc_interworks true > +#define alu_write_pc_interworks true > +#define find_str_pc_offset() > +#define test_load_write_pc_interworking() > +#define test_alu_write_pc_interworking() > > -extern probes_check_cc * const probes_condition_checks[16]; > +#else /* CONFIG_ARM64 */ > > #if __LINUX_ARM_ARCH__ >= 7 > > @@ -40,8 +50,7 @@ extern probes_check_cc * const probes_condition_checks[16]; > extern int str_pc_offset; > void __init find_str_pc_offset(void); > > -#endif > - > +#endif /* __LINUX_ARM_ARCH__ */ > > /* > * Update ITSTATE after normal execution of an IT block instruction. > @@ -69,9 +78,13 @@ static inline unsigned long it_advance(unsigned long cpsr) > return cpsr; > } > > +#endif /* CONFIG_ARM64 */ > + > +void __init arm_probes_decode_init(void); > + > static inline void __kprobes bx_write_pc(long pcv, struct pt_regs *regs) > { > - long cpsr = regs->ARM_cpsr; > + long cpsr = state_register(regs); > > if (pcv & 0x1) { > cpsr |= PSR_T_BIT; > @@ -80,11 +93,11 @@ static inline void __kprobes bx_write_pc(long pcv, struct pt_regs *regs) > cpsr &= ~PSR_T_BIT; > pcv &= ~0x2; /* Avoid UNPREDICTABLE address allignment */ > } > - regs->ARM_cpsr = cpsr; > - regs->ARM_pc = pcv; > + state_register_set(regs, cpsr); > + instruction_pointer_set(regs, pcv); > } > > - > +#ifndef CONFIG_ARM64 > #if __LINUX_ARM_ARCH__ >= 6 > > /* Kernels built for >= ARMv6 should never run on <= ARMv5 hardware, so... */ > @@ -98,16 +111,18 @@ extern bool load_write_pc_interworks; > void __init test_load_write_pc_interworking(void); > > #endif > +#endif /* CONFIG_ARM64 */ > > static inline void __kprobes load_write_pc(long pcv, struct pt_regs *regs) > { > if (load_write_pc_interworks) > bx_write_pc(pcv, regs); > else > - regs->ARM_pc = pcv; > + instruction_pointer_set(regs, pcv); > } > > > +#ifndef CONFIG_ARM64 > #if __LINUX_ARM_ARCH__ >= 7 > > #define alu_write_pc_interworks true > @@ -126,13 +141,14 @@ extern bool alu_write_pc_interworks; > void __init test_alu_write_pc_interworking(void); > > #endif /* __LINUX_ARM_ARCH__ == 6 */ > +#endif /* CONFIG_ARM64 */ > > static inline void __kprobes alu_write_pc(long pcv, struct pt_regs *regs) > { > if (alu_write_pc_interworks) > bx_write_pc(pcv, regs); > else > - regs->ARM_pc = pcv; > + instruction_pointer_set(regs, pcv); > } > > > @@ -395,11 +411,6 @@ struct decode_or { > #define DECODE_OR(_mask, _value) \ > DECODE_HEADER(DECODE_TYPE_OR, _mask, _value, 0) > > -enum probes_insn { > - INSN_REJECTED, > - INSN_GOOD, > - INSN_GOOD_NO_SLOT > -}; > > struct decode_reject { > struct decode_header header; > -- Julien Thierry ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/7] arm64: make arm uprobes code reusable by arm64 2018-09-27 15:52 ` Julien Thierry @ 2018-10-01 13:28 ` Maciej Slodczyk 2018-10-02 8:08 ` Julien Thierry 0 siblings, 1 reply; 18+ messages in thread From: Maciej Slodczyk @ 2018-10-01 13:28 UTC (permalink / raw) To: Julien Thierry, linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk Hi, Thank you for the review. > I think that it would be good to move the renaming changes out of this > patch. > So, as I understand, you suggest separating renaming from moving and putting it in separate patches, right? >> }) >> +#define ARM_COMPAT_LR_OFFSET 0 > > Not sure this should be defined here. What's the meaning of compat for > arch/arm ? > Sure, I agree that the name is not very fortunate. I'll change it to something like ARM_UPROBES_BRANCH_LR_OFFSET. >> @@ -39,7 +39,7 @@ struct arch_uprobe { >> void (*posthandler)(struct arch_uprobe *auprobe, >> struct arch_uprobe_task *autask, >> struct pt_regs *regs); >> - struct arch_probes_insn asi; >> + struct arch_probes_insn api; > > It would be easier to follow thing by making this change in its own > patch. (Probably before you move arm32 code to lib/probes) > Yup. >> +enum probes_insn { >> + INSN_REJECTED, >> + INSN_GOOD_NO_SLOT, >> + INSN_GOOD, >> +}; > > Why have two definitions of this enum rather than a common one in > lib/probes? > Will fix in v3. >> -typedef void (probes_handler_t) (u32 opcode, >> - struct arch_probe_insn *api, >> +typedef void (probes_insn_handler_t) (u32 opcode, >> + struct arch_probes_insn *api, > > In the previous patch you were already aligning this handler the ARM32's > equivalent. Why not fix the name (for the handler and struct > arch_probes_insn) in the previous patch? > OK. >> + >> +#define link_register(regs) ((regs)->compat_lr) >> + >> +static inline void link_register_set(struct pt_regs *regs, >> + unsigned long val) >> +{ >> + link_register(regs) = val; >> +} > > pstate.h isn't really related to compat mode and whichever compat > definition it contains the relations are made explicit through their names. > > I don't think a macro "link_register" defined in arch/arm64 and visible > to any file including ptrace.h (which is a lot) should return > "compat_lr" instead of the actual link register. > > I'd say have the link_register macro check whether "regs" refers to a > compat mode context or not and provide the adequate link register. > > Otherwise maybe you can get away with naming the macro > "arm_link_register" and the macro "arm_link_register_set". But I would > prefer the previous approach. > OK. >> +#ifdef CONFIG_ARM64 >> +#include <../../../arm/include/asm/opcodes.h> > > Hmmm not sure this is something that is accepted. > OK, I'll fix it. >> +/* >> + * based on arm kprobes implementation >> + */ >> +static void __kprobes simulate_ldm1stm1(probes_opcode_t insn, >> + struct arch_probes_insn *asi, > > The whole asi/api mix become a bit confusing IMO. > Should we have api when the argument is of type "arch_probes_insn" and > asi when the type is "arch_specific_insn"? > Should we have more coherent definitions of those structures between arm > and arm64 if we are going to share functions between them? > OK, I'll try to figure something out that's less confusing. > > #ifdef CONFIG_ARM64 > >> +enum probes_insn >> +uprobe_decode_ldmstm_aarch64(probes_opcode_t insn, >> + struct arch_probes_insn *asi, >> + const struct decode_header *d) > > Should be static. > OK. Thanks again for the review. I'll rework the whole patchset to include your remarks. Thank you, Maciej ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/7] arm64: make arm uprobes code reusable by arm64 2018-10-01 13:28 ` Maciej Slodczyk @ 2018-10-02 8:08 ` Julien Thierry 0 siblings, 0 replies; 18+ messages in thread From: Julien Thierry @ 2018-10-02 8:08 UTC (permalink / raw) To: Maciej Slodczyk, linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk Hi, On 01/10/18 14:28, Maciej Slodczyk wrote: > Hi, > > Thank you for the review. > >> I think that it would be good to move the renaming changes out of this >> patch. >> > > So, as I understand, you suggest separating renaming from moving and > putting it in separate patches, right? > Yes, I just feel this patch has a lot of changes and things like renaming/adapting the callback handler and renaming the arm32 stucture field could be put in their own patches. Thanks, >>> }) >>> +#define ARM_COMPAT_LR_OFFSET 0 >> >> Not sure this should be defined here. What's the meaning of compat for >> arch/arm ? >> > > Sure, I agree that the name is not very fortunate. I'll change it to > something like ARM_UPROBES_BRANCH_LR_OFFSET. > >>> @@ -39,7 +39,7 @@ struct arch_uprobe { >>> void (*posthandler)(struct arch_uprobe *auprobe, >>> struct arch_uprobe_task *autask, >>> struct pt_regs *regs); >>> - struct arch_probes_insn asi; >>> + struct arch_probes_insn api; >> >> It would be easier to follow thing by making this change in its own >> patch. (Probably before you move arm32 code to lib/probes) >> > > Yup. > > >>> +enum probes_insn { >>> + INSN_REJECTED, >>> + INSN_GOOD_NO_SLOT, >>> + INSN_GOOD, >>> +}; >> >> Why have two definitions of this enum rather than a common one in >> lib/probes? >> > > Will fix in v3. > >>> -typedef void (probes_handler_t) (u32 opcode, >>> - struct arch_probe_insn *api, >>> +typedef void (probes_insn_handler_t) (u32 opcode, >>> + struct arch_probes_insn *api, >> >> In the previous patch you were already aligning this handler the ARM32's >> equivalent. Why not fix the name (for the handler and struct >> arch_probes_insn) in the previous patch? >> > > OK. > >>> + >>> +#define link_register(regs) ((regs)->compat_lr) >>> + >>> +static inline void link_register_set(struct pt_regs *regs, >>> + unsigned long val) >>> +{ >>> + link_register(regs) = val; >>> +} >> >> pstate.h isn't really related to compat mode and whichever compat >> definition it contains the relations are made explicit through their names. >> >> I don't think a macro "link_register" defined in arch/arm64 and visible >> to any file including ptrace.h (which is a lot) should return >> "compat_lr" instead of the actual link register. >> >> I'd say have the link_register macro check whether "regs" refers to a >> compat mode context or not and provide the adequate link register. >> >> Otherwise maybe you can get away with naming the macro >> "arm_link_register" and the macro "arm_link_register_set". But I would >> prefer the previous approach. >> > > OK. > >>> +#ifdef CONFIG_ARM64 >>> +#include <../../../arm/include/asm/opcodes.h> >> >> Hmmm not sure this is something that is accepted. >> > > OK, I'll fix it. > >>> +/* >>> + * based on arm kprobes implementation >>> + */ >>> +static void __kprobes simulate_ldm1stm1(probes_opcode_t insn, >>> + struct arch_probes_insn *asi, >> >> The whole asi/api mix become a bit confusing IMO. >> Should we have api when the argument is of type "arch_probes_insn" and >> asi when the type is "arch_specific_insn"? >> Should we have more coherent definitions of those structures between arm >> and arm64 if we are going to share functions between them? >> > > OK, I'll try to figure something out that's less confusing. > >> >> #ifdef CONFIG_ARM64 >> >>> +enum probes_insn >>> +uprobe_decode_ldmstm_aarch64(probes_opcode_t insn, >>> + struct arch_probes_insn *asi, >>> + const struct decode_header *d) >> >> Should be static. >> > > OK. > > Thanks again for the review. I'll rework the whole patchset to include > your remarks. > > > Thank you, > Maciej > -- Julien Thierry ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <CGME20180926121217eucas1p198d96ed637d1aa8a98c1b90466dde745@eucas1p1.samsung.com>]
* [PATCH v2 6/7] arm64: change arm_probe_decode_insn() function name [not found] ` <CGME20180926121217eucas1p198d96ed637d1aa8a98c1b90466dde745@eucas1p1.samsung.com> @ 2018-09-26 12:12 ` Maciej Slodczyk 0 siblings, 0 replies; 18+ messages in thread From: Maciej Slodczyk @ 2018-09-26 12:12 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk, m.slodczyk2 ARM probe decoding function has similar name in arm32 - arm_probes_decode_insn(), and arm64 - arm_probe_decode_insn(). Change arm64 probes decoding function name from arm_probe_decode_insn() to arm64_probes_decode_insn() to minimize the risk of confusion. Signed-off-by: Maciej Slodczyk <m.slodczyk2@partner.samsung.com> --- arch/arm64/kernel/probes/decode-insn.c | 4 ++-- arch/arm64/kernel/probes/decode-insn.h | 2 +- arch/arm64/kernel/probes/uprobes.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c index a0597a2..37be1a9 100644 --- a/arch/arm64/kernel/probes/decode-insn.c +++ b/arch/arm64/kernel/probes/decode-insn.c @@ -78,7 +78,7 @@ static bool __kprobes aarch64_insn_is_steppable(u32 insn) * INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot. */ enum probes_insn __kprobes -arm_probe_decode_insn(probes_opcode_t insn, struct arch_probes_insn *api) +arm64_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *api) { /* * Instructions reading or modifying the PC won't work from the XOL @@ -162,7 +162,7 @@ arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi) else scan_end = addr - MAX_ATOMIC_CONTEXT_SIZE; } - decoded = arm_probe_decode_insn(insn, &asi->api); + decoded = arm64_probes_decode_insn(insn, &asi->api); if (decoded != INSN_REJECTED && scan_end) if (is_probed_address_atomic(addr - 1, scan_end)) diff --git a/arch/arm64/kernel/probes/decode-insn.h b/arch/arm64/kernel/probes/decode-insn.h index 93e021f..e6c7b9e 100644 --- a/arch/arm64/kernel/probes/decode-insn.h +++ b/arch/arm64/kernel/probes/decode-insn.h @@ -32,7 +32,7 @@ enum probes_insn __kprobes arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi); #endif enum probes_insn __kprobes -arm_probe_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi); +arm64_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi); extern const union decode_action uprobes_probes_actions[]; #endif /* _ARM_KERNEL_KPROBES_ARM64_H */ diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c index f99b50b..e7b8912 100644 --- a/arch/arm64/kernel/probes/uprobes.c +++ b/arch/arm64/kernel/probes/uprobes.c @@ -47,7 +47,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, insn = *(probes_opcode_t *)(&auprobe->insn[0]); - switch (arm_probe_decode_insn(insn, &auprobe->api)) { + switch (arm64_probes_decode_insn(insn, &auprobe->api)) { case INSN_REJECTED: return -EINVAL; -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <CGME20180926121218eucas1p1b20a88cfec17c6403a35e4f23de96ade@eucas1p1.samsung.com>]
* [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing [not found] ` <CGME20180926121218eucas1p1b20a88cfec17c6403a35e4f23de96ade@eucas1p1.samsung.com> @ 2018-09-26 12:12 ` Maciej Slodczyk 2018-09-27 16:18 ` Julien Thierry 2018-09-27 17:01 ` Robin Murphy 0 siblings, 2 replies; 18+ messages in thread From: Maciej Slodczyk @ 2018-09-26 12:12 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk, m.slodczyk2 Detect what kind of instruction is being probed and depending on the result: - if an A64 instruction handle it the old way, using existing A64 instructions probing code, - if an A32 instruction decode it and handle using the new code, moved from 32 bit arm kernel tree. Signed-off-by: Maciej Slodczyk <m.slodczyk2@partner.samsung.com> --- arch/arm64/kernel/debug-monitors.c | 8 +++ arch/arm64/kernel/probes/uprobes.c | 111 ++++++++++++++++++++++++++++++++++--- lib/probes/arm/actions-arm.c | 35 ++++++++---- lib/probes/arm/decode-arm.c | 16 +++++- 4 files changed, 148 insertions(+), 22 deletions(-) diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 06ca574..ee10c0b 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -366,6 +366,14 @@ int aarch32_break_handler(struct pt_regs *regs) if (!bp) return -EFAULT; + /* + * Since bp != false, a sofware breakpoint instruction is being handled. + * If in user mode (compat_user_mode() few lines above), + * try to handle it by an uprobe handler, if registered. + */ + if (!brk_handler((unsigned long)pc, BRK64_ESR_UPROBES, regs)) + return 0; + send_user_sigtrap(TRAP_BRKPT); return 0; } diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c index e7b8912..872c8ec 100644 --- a/arch/arm64/kernel/probes/uprobes.c +++ b/arch/arm64/kernel/probes/uprobes.c @@ -11,9 +11,49 @@ #include <asm/cacheflush.h> #include "decode-insn.h" +#include "decode.h" +#include "decode-arm.h" +#include <../../../arm/include/asm/opcodes.h> #define UPROBE_INV_FAULT_CODE UINT_MAX +uprobe_opcode_t get_swbp_insn(void) +{ + if (is_compat_task()) + return AARCH32_BREAK_ARM; + else + return UPROBE_SWBP_INSN; +} + +bool is_swbp_insn(uprobe_opcode_t *insn) +{ + return ((__mem_to_opcode_arm(*insn) & 0x0fffffff) == + (AARCH32_BREAK_ARM & 0x0fffffff)) || + *insn == UPROBE_SWBP_INSN; +} + +int set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, + unsigned long vaddr) +{ + if (auprobe->arch == UPROBE_AARCH32) + return uprobe_write_opcode(auprobe, mm, vaddr, + __opcode_to_mem_arm(auprobe->bp_insn)); + else + return uprobe_write_opcode(auprobe, mm, vaddr, + UPROBE_SWBP_INSN); +} + +int set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, + unsigned long vaddr) +{ + if (auprobe->arch == UPROBE_AARCH32) + return uprobe_write_opcode(auprobe, mm, vaddr, + auprobe->orig_insn); + else + return uprobe_write_opcode(auprobe, mm, vaddr, + *(uprobe_opcode_t *)&auprobe->insn); +} + void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, void *src, unsigned long len) { @@ -38,16 +78,44 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long addr) { probes_opcode_t insn; + enum probes_insn retval; + unsigned int bpinsn; - /* TODO: Currently we do not support AARCH32 instruction probing */ - if (mm->context.flags & MMCF_AARCH32) - return -ENOTSUPP; - else if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) + insn = *(probes_opcode_t *)(&auprobe->insn[0]); + + if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) return -EINVAL; - insn = *(probes_opcode_t *)(&auprobe->insn[0]); + /* check if AARCH32 */ + if (is_compat_task()) { + + /* Thumb is not supported yet */ + if (addr & 0x3) + return -EINVAL; + + retval = arm_probes_decode_insn(insn, &auprobe->api, false, + uprobes_probes_actions, NULL); + auprobe->arch = UPROBE_AARCH32; + + /* + * original instruction could have been modified + * when preparing for xol on AArch32 + */ + auprobe->orig_insn = insn; + + bpinsn = AARCH32_BREAK_ARM & 0x0fffffff; + if (insn >= 0xe0000000) /* Unconditional instruction */ + bpinsn |= 0xe0000000; + else /* Copy condition from insn */ + bpinsn |= insn & 0xf0000000; + + auprobe->bp_insn = bpinsn; + } else { + retval = arm64_probes_decode_insn(insn, &auprobe->api); + auprobe->arch = UPROBE_AARCH64; + } - switch (arm64_probes_decode_insn(insn, &auprobe->api)) { + switch (retval) { case INSN_REJECTED: return -EINVAL; @@ -66,6 +134,9 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) { struct uprobe_task *utask = current->utask; + if (auprobe->prehandler) + auprobe->prehandler(auprobe, &utask->autask, regs); + /* Initialize with an invalid fault code to detect if ol insn trapped */ current->thread.fault_code = UPROBE_INV_FAULT_CODE; @@ -88,6 +159,9 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) user_disable_single_step(current); + if (auprobe->posthandler) + auprobe->posthandler(auprobe, &utask->autask, regs); + return 0; } bool arch_uprobe_xol_was_trapped(struct task_struct *t) @@ -103,10 +177,24 @@ bool arch_uprobe_xol_was_trapped(struct task_struct *t) return false; } +bool arch_uprobe_ignore(struct arch_uprobe *auprobe, struct pt_regs *regs) +{ + probes_opcode_t insn = *(probes_opcode_t *)(&auprobe->insn[0]); + pstate_check_t *check = (*aarch32_opcode_cond_checks[insn >> 28]); + + if (auprobe->arch == UPROBE_AARCH64) + return false; + + if (!check(regs->pstate & 0xffffffff)) { + instruction_pointer_set(regs, instruction_pointer(regs) + 4); + return true; + } + return false; +} + bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) { probes_opcode_t insn; - unsigned long addr; if (!auprobe->simulate) return false; @@ -154,9 +242,14 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, { unsigned long orig_ret_vaddr; - orig_ret_vaddr = procedure_link_pointer(regs); /* Replace the return addr with trampoline addr */ - procedure_link_pointer_set(regs, trampoline_vaddr); + if (is_compat_task()) { + orig_ret_vaddr = link_register(regs); + link_register_set(regs, trampoline_vaddr); + } else { + orig_ret_vaddr = procedure_link_pointer(regs); + procedure_link_pointer_set(regs, trampoline_vaddr); + } return orig_ret_vaddr; } diff --git a/lib/probes/arm/actions-arm.c b/lib/probes/arm/actions-arm.c index 2393573..cee1496 100644 --- a/lib/probes/arm/actions-arm.c +++ b/lib/probes/arm/actions-arm.c @@ -15,10 +15,7 @@ #include "decode.h" #include "decode-arm.h" - -#ifdef CONFIG_ARM64 #include <../../../arm/include/asm/opcodes.h> -#endif /* CONFIG_ARM64 */ static int uprobes_substitute_pc(unsigned long *pinsn, u32 oregs) { @@ -75,8 +72,13 @@ static void uprobe_set_pc(struct arch_uprobe *auprobe, { u32 pcreg = auprobe->pcreg; - autask->backup = pt_regs_read_reg(regs, pcreg); - pt_regs_write_reg(regs, pcreg, instruction_pointer(regs) + 8); + if (pcreg == 15) { + autask->backup = instruction_pointer(regs); + instruction_pointer_set(regs, instruction_pointer(regs) + 8); + } else { + autask->backup = pt_regs_read_reg(regs, pcreg); + pt_regs_write_reg(regs, pcreg, instruction_pointer(regs) + 8); + } } static void uprobe_unset_pc(struct arch_uprobe *auprobe, @@ -84,7 +86,10 @@ static void uprobe_unset_pc(struct arch_uprobe *auprobe, struct pt_regs *regs) { /* PC will be taken care of by common code */ - pt_regs_write_reg(regs, auprobe->pcreg, autask->backup); + if (auprobe->pcreg == 15) + instruction_pointer_set(regs, autask->backup); + else + pt_regs_write_reg(regs, auprobe->pcreg, autask->backup); } static void uprobe_aluwrite_pc(struct arch_uprobe *auprobe, @@ -93,8 +98,13 @@ static void uprobe_aluwrite_pc(struct arch_uprobe *auprobe, { u32 pcreg = auprobe->pcreg; - alu_write_pc(pt_regs_read_reg(regs, pcreg), regs); - pt_regs_write_reg(regs, pcreg, autask->backup); + if (pcreg == 15) { + alu_write_pc(instruction_pointer(regs), regs); + instruction_pointer_set(regs, autask->backup); + } else { + alu_write_pc(pt_regs_read_reg(regs, pcreg), regs); + pt_regs_write_reg(regs, pcreg, autask->backup); + } } static void uprobe_write_pc(struct arch_uprobe *auprobe, @@ -103,8 +113,13 @@ static void uprobe_write_pc(struct arch_uprobe *auprobe, { u32 pcreg = auprobe->pcreg; - load_write_pc(pt_regs_read_reg(regs, pcreg), regs); - pt_regs_write_reg(regs, pcreg, autask->backup); + if (pcreg == 15) { + load_write_pc(instruction_pointer(regs), regs); + instruction_pointer_set(regs, autask->backup); + } else { + load_write_pc(pt_regs_read_reg(regs, pcreg), regs); + pt_regs_write_reg(regs, pcreg, autask->backup); + } } enum probes_insn diff --git a/lib/probes/arm/decode-arm.c b/lib/probes/arm/decode-arm.c index 36eb939..2f2a810 100644 --- a/lib/probes/arm/decode-arm.c +++ b/lib/probes/arm/decode-arm.c @@ -87,11 +87,18 @@ void __kprobes simulate_blx2bx(probes_opcode_t insn, struct arch_probes_insn *asi, struct pt_regs *regs) { int rm = insn & 0xf; - long rmv = pt_regs_read_reg(regs, rm); + long rmv; long cpsr; + if (rm == 15) + rmv = (long) instruction_pointer(regs); + else + rmv = pt_regs_read_reg(regs, rm); + if (insn & (1 << 5)) - link_register_set(regs, (long) instruction_pointer(regs)); + link_register_set(regs, + (long) instruction_pointer(regs) + + ARM_COMPAT_LR_OFFSET); instruction_pointer_set(regs, rmv & ~0x1); cpsr = state_register(regs) & ~PSR_T_BIT; @@ -108,7 +115,10 @@ void __kprobes simulate_mrs(probes_opcode_t insn, int rd = (insn >> 12) & 0xf; unsigned long mask = 0xf8ff03df; /* Mask out execution state */ - pt_regs_write_reg(regs, rd, state_register(regs) & mask); + if (rd == 15) + instruction_pointer_set(regs, state_register(regs) & mask); + else + pt_regs_write_reg(regs, rd, state_register(regs) & mask); } void __kprobes simulate_mov_ipsp(probes_opcode_t insn, -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing 2018-09-26 12:12 ` [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing Maciej Slodczyk @ 2018-09-27 16:18 ` Julien Thierry 2018-09-27 17:01 ` Robin Murphy 1 sibling, 0 replies; 18+ messages in thread From: Julien Thierry @ 2018-09-27 16:18 UTC (permalink / raw) To: Maciej Slodczyk, linux-arm-kernel, linux-kernel Cc: linux, oleg, catalin.marinas, will.deacon, peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, b.zolnierkie, m.szyprowski, k.lewandowsk Hi Maciej, On 26/09/18 13:12, Maciej Slodczyk wrote: > Detect what kind of instruction is being probed and depending on the > result: > - if an A64 instruction handle it the old way, using existing A64 > instructions probing code, > - if an A32 instruction decode it and handle using the new code, moved > from 32 bit arm kernel tree. > > Signed-off-by: Maciej Slodczyk <m.slodczyk2@partner.samsung.com> > --- > arch/arm64/kernel/debug-monitors.c | 8 +++ > arch/arm64/kernel/probes/uprobes.c | 111 ++++++++++++++++++++++++++++++++++--- > lib/probes/arm/actions-arm.c | 35 ++++++++---- > lib/probes/arm/decode-arm.c | 16 +++++- > 4 files changed, 148 insertions(+), 22 deletions(-) > > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c > index 06ca574..ee10c0b 100644 > --- a/arch/arm64/kernel/debug-monitors.c > +++ b/arch/arm64/kernel/debug-monitors.c > @@ -366,6 +366,14 @@ int aarch32_break_handler(struct pt_regs *regs) > if (!bp) > return -EFAULT; > > + /* > + * Since bp != false, a sofware breakpoint instruction is being handled. > + * If in user mode (compat_user_mode() few lines above), > + * try to handle it by an uprobe handler, if registered. > + */ > + if (!brk_handler((unsigned long)pc, BRK64_ESR_UPROBES, regs)) > + return 0; > + > send_user_sigtrap(TRAP_BRKPT); > return 0; > } > diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c > index e7b8912..872c8ec 100644 > --- a/arch/arm64/kernel/probes/uprobes.c > +++ b/arch/arm64/kernel/probes/uprobes.c > @@ -11,9 +11,49 @@ > #include <asm/cacheflush.h> > > #include "decode-insn.h" > +#include "decode.h" > +#include "decode-arm.h" > +#include <../../../arm/include/asm/opcodes.h> I'm almost positive this is frowned upon. I don't see any headers from arch/arm every being included in arch/arm64. > > #define UPROBE_INV_FAULT_CODE UINT_MAX > > +uprobe_opcode_t get_swbp_insn(void) > +{ > + if (is_compat_task()) > + return AARCH32_BREAK_ARM; > + else > + return UPROBE_SWBP_INSN; > +} > + > +bool is_swbp_insn(uprobe_opcode_t *insn) > +{ > + return ((__mem_to_opcode_arm(*insn) & 0x0fffffff) == > + (AARCH32_BREAK_ARM & 0x0fffffff)) || > + *insn == UPROBE_SWBP_INSN; > +} > + > +int set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, > + unsigned long vaddr) > +{ > + if (auprobe->arch == UPROBE_AARCH32) > + return uprobe_write_opcode(auprobe, mm, vaddr, > + __opcode_to_mem_arm(auprobe->bp_insn)); > + else > + return uprobe_write_opcode(auprobe, mm, vaddr, > + UPROBE_SWBP_INSN); > +} > + > +int set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > + unsigned long vaddr) > +{ > + if (auprobe->arch == UPROBE_AARCH32) > + return uprobe_write_opcode(auprobe, mm, vaddr, > + auprobe->orig_insn); > + else > + return uprobe_write_opcode(auprobe, mm, vaddr, > + *(uprobe_opcode_t *)&auprobe->insn); > +} > + > void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, > void *src, unsigned long len) > { > @@ -38,16 +78,44 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > unsigned long addr) > { > probes_opcode_t insn; > + enum probes_insn retval; > + unsigned int bpinsn; > > - /* TODO: Currently we do not support AARCH32 instruction probing */ > - if (mm->context.flags & MMCF_AARCH32) > - return -ENOTSUPP; > - else if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) > + insn = *(probes_opcode_t *)(&auprobe->insn[0]); > + > + if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) > return -EINVAL; > > - insn = *(probes_opcode_t *)(&auprobe->insn[0]); > + /* check if AARCH32 */ > + if (is_compat_task()) { > + > + /* Thumb is not supported yet */ > + if (addr & 0x3) > + return -EINVAL; -ENOTSUPP? Cheers, Julien > + > + retval = arm_probes_decode_insn(insn, &auprobe->api, false, > + uprobes_probes_actions, NULL); > + auprobe->arch = UPROBE_AARCH32; > + > + /* > + * original instruction could have been modified > + * when preparing for xol on AArch32 > + */ > + auprobe->orig_insn = insn; > + > + bpinsn = AARCH32_BREAK_ARM & 0x0fffffff; > + if (insn >= 0xe0000000) /* Unconditional instruction */ > + bpinsn |= 0xe0000000; > + else /* Copy condition from insn */ > + bpinsn |= insn & 0xf0000000; > + > + auprobe->bp_insn = bpinsn; > + } else { > + retval = arm64_probes_decode_insn(insn, &auprobe->api); > + auprobe->arch = UPROBE_AARCH64; > + } > > - switch (arm64_probes_decode_insn(insn, &auprobe->api)) { > + switch (retval) { > case INSN_REJECTED: > return -EINVAL; > > @@ -66,6 +134,9 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > { > struct uprobe_task *utask = current->utask; > > + if (auprobe->prehandler) > + auprobe->prehandler(auprobe, &utask->autask, regs); > + > /* Initialize with an invalid fault code to detect if ol insn trapped */ > current->thread.fault_code = UPROBE_INV_FAULT_CODE; > > @@ -88,6 +159,9 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > > user_disable_single_step(current); > > + if (auprobe->posthandler) > + auprobe->posthandler(auprobe, &utask->autask, regs); > + > return 0; > } > bool arch_uprobe_xol_was_trapped(struct task_struct *t) > @@ -103,10 +177,24 @@ bool arch_uprobe_xol_was_trapped(struct task_struct *t) > return false; > } > > +bool arch_uprobe_ignore(struct arch_uprobe *auprobe, struct pt_regs *regs) > +{ > + probes_opcode_t insn = *(probes_opcode_t *)(&auprobe->insn[0]); > + pstate_check_t *check = (*aarch32_opcode_cond_checks[insn >> 28]); > + > + if (auprobe->arch == UPROBE_AARCH64) > + return false; > + > + if (!check(regs->pstate & 0xffffffff)) { > + instruction_pointer_set(regs, instruction_pointer(regs) + 4); > + return true; > + } > + return false; > +} > + > bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) > { > probes_opcode_t insn; > - unsigned long addr; > > if (!auprobe->simulate) > return false; > @@ -154,9 +242,14 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, > { > unsigned long orig_ret_vaddr; > > - orig_ret_vaddr = procedure_link_pointer(regs); > /* Replace the return addr with trampoline addr */ > - procedure_link_pointer_set(regs, trampoline_vaddr); > + if (is_compat_task()) { > + orig_ret_vaddr = link_register(regs); > + link_register_set(regs, trampoline_vaddr); > + } else { > + orig_ret_vaddr = procedure_link_pointer(regs); > + procedure_link_pointer_set(regs, trampoline_vaddr); > + } > > return orig_ret_vaddr; > } > diff --git a/lib/probes/arm/actions-arm.c b/lib/probes/arm/actions-arm.c > index 2393573..cee1496 100644 > --- a/lib/probes/arm/actions-arm.c > +++ b/lib/probes/arm/actions-arm.c > @@ -15,10 +15,7 @@ > > #include "decode.h" > #include "decode-arm.h" > - > -#ifdef CONFIG_ARM64 > #include <../../../arm/include/asm/opcodes.h> > -#endif /* CONFIG_ARM64 */ > > static int uprobes_substitute_pc(unsigned long *pinsn, u32 oregs) > { > @@ -75,8 +72,13 @@ static void uprobe_set_pc(struct arch_uprobe *auprobe, > { > u32 pcreg = auprobe->pcreg; > > - autask->backup = pt_regs_read_reg(regs, pcreg); > - pt_regs_write_reg(regs, pcreg, instruction_pointer(regs) + 8); > + if (pcreg == 15) { > + autask->backup = instruction_pointer(regs); > + instruction_pointer_set(regs, instruction_pointer(regs) + 8); > + } else { > + autask->backup = pt_regs_read_reg(regs, pcreg); > + pt_regs_write_reg(regs, pcreg, instruction_pointer(regs) + 8); > + } > } > > static void uprobe_unset_pc(struct arch_uprobe *auprobe, > @@ -84,7 +86,10 @@ static void uprobe_unset_pc(struct arch_uprobe *auprobe, > struct pt_regs *regs) > { > /* PC will be taken care of by common code */ > - pt_regs_write_reg(regs, auprobe->pcreg, autask->backup); > + if (auprobe->pcreg == 15) > + instruction_pointer_set(regs, autask->backup); > + else > + pt_regs_write_reg(regs, auprobe->pcreg, autask->backup); > } > > static void uprobe_aluwrite_pc(struct arch_uprobe *auprobe, > @@ -93,8 +98,13 @@ static void uprobe_aluwrite_pc(struct arch_uprobe *auprobe, > { > u32 pcreg = auprobe->pcreg; > > - alu_write_pc(pt_regs_read_reg(regs, pcreg), regs); > - pt_regs_write_reg(regs, pcreg, autask->backup); > + if (pcreg == 15) { > + alu_write_pc(instruction_pointer(regs), regs); > + instruction_pointer_set(regs, autask->backup); > + } else { > + alu_write_pc(pt_regs_read_reg(regs, pcreg), regs); > + pt_regs_write_reg(regs, pcreg, autask->backup); > + } > } > > static void uprobe_write_pc(struct arch_uprobe *auprobe, > @@ -103,8 +113,13 @@ static void uprobe_write_pc(struct arch_uprobe *auprobe, > { > u32 pcreg = auprobe->pcreg; > > - load_write_pc(pt_regs_read_reg(regs, pcreg), regs); > - pt_regs_write_reg(regs, pcreg, autask->backup); > + if (pcreg == 15) { > + load_write_pc(instruction_pointer(regs), regs); > + instruction_pointer_set(regs, autask->backup); > + } else { > + load_write_pc(pt_regs_read_reg(regs, pcreg), regs); > + pt_regs_write_reg(regs, pcreg, autask->backup); > + } > } > > enum probes_insn > diff --git a/lib/probes/arm/decode-arm.c b/lib/probes/arm/decode-arm.c > index 36eb939..2f2a810 100644 > --- a/lib/probes/arm/decode-arm.c > +++ b/lib/probes/arm/decode-arm.c > @@ -87,11 +87,18 @@ void __kprobes simulate_blx2bx(probes_opcode_t insn, > struct arch_probes_insn *asi, struct pt_regs *regs) > { > int rm = insn & 0xf; > - long rmv = pt_regs_read_reg(regs, rm); > + long rmv; > long cpsr; > > + if (rm == 15) > + rmv = (long) instruction_pointer(regs); > + else > + rmv = pt_regs_read_reg(regs, rm); > + > if (insn & (1 << 5)) > - link_register_set(regs, (long) instruction_pointer(regs)); > + link_register_set(regs, > + (long) instruction_pointer(regs) > + + ARM_COMPAT_LR_OFFSET); > > instruction_pointer_set(regs, rmv & ~0x1); > cpsr = state_register(regs) & ~PSR_T_BIT; > @@ -108,7 +115,10 @@ void __kprobes simulate_mrs(probes_opcode_t insn, > int rd = (insn >> 12) & 0xf; > unsigned long mask = 0xf8ff03df; /* Mask out execution state */ > > - pt_regs_write_reg(regs, rd, state_register(regs) & mask); > + if (rd == 15) > + instruction_pointer_set(regs, state_register(regs) & mask); > + else > + pt_regs_write_reg(regs, rd, state_register(regs) & mask); > } > > void __kprobes simulate_mov_ipsp(probes_opcode_t insn, > -- Julien Thierry ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing 2018-09-26 12:12 ` [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing Maciej Slodczyk 2018-09-27 16:18 ` Julien Thierry @ 2018-09-27 17:01 ` Robin Murphy 2018-10-01 13:40 ` Maciej Slodczyk 1 sibling, 1 reply; 18+ messages in thread From: Robin Murphy @ 2018-09-27 17:01 UTC (permalink / raw) To: Maciej Slodczyk, linux-arm-kernel, linux-kernel Cc: b.zolnierkie, peterz, catalin.marinas, will.deacon, linux, acme, oleg, alexander.shishkin, mingo, k.lewandowsk, namhyung, jolsa, m.szyprowski On 26/09/18 13:12, Maciej Slodczyk wrote: [...] > @@ -38,16 +78,44 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > unsigned long addr) > { > probes_opcode_t insn; > + enum probes_insn retval; > + unsigned int bpinsn; > > - /* TODO: Currently we do not support AARCH32 instruction probing */ > - if (mm->context.flags & MMCF_AARCH32) > - return -ENOTSUPP; > - else if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) > + insn = *(probes_opcode_t *)(&auprobe->insn[0]); > + > + if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) > return -EINVAL; > > - insn = *(probes_opcode_t *)(&auprobe->insn[0]); > + /* check if AARCH32 */ > + if (is_compat_task()) { > + > + /* Thumb is not supported yet */ > + if (addr & 0x3) I'm only skimming, so forgive me if I'm missing something which should be obvious, but this has a big red flag all over it. If "addr" is the actual instruction address (or even a branch target, for a non-interworking branch), plenty of Thumb instructions will just happen to lie at 4-byte-aligned addresses anyway. Furthermore, how would this check ever catch anything anyway given !IS_ALIGNED(addr, AARCH64_INSN_SIZE) above? Robin. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing 2018-09-27 17:01 ` Robin Murphy @ 2018-10-01 13:40 ` Maciej Slodczyk 2018-10-02 11:04 ` Robin Murphy 0 siblings, 1 reply; 18+ messages in thread From: Maciej Slodczyk @ 2018-10-01 13:40 UTC (permalink / raw) To: Robin Murphy, linux-arm-kernel, linux-kernel Cc: b.zolnierkie, peterz, catalin.marinas, will.deacon, linux, acme, oleg, alexander.shishkin, mingo, k.lewandowsk, namhyung, jolsa, m.szyprowski Hi Robin, Thank you for having a look at my patchset. On 27.09.2018 19:01, Robin Murphy wrote: > On 26/09/18 13:12, Maciej Slodczyk wrote: > [...] >> @@ -38,16 +78,44 @@ int arch_uprobe_analyze_insn(struct arch_uprobe >> *auprobe, struct mm_struct *mm, >> unsigned long addr) >> { >> probes_opcode_t insn; >> + enum probes_insn retval; >> + unsigned int bpinsn; >> - /* TODO: Currently we do not support AARCH32 instruction probing */ >> - if (mm->context.flags & MMCF_AARCH32) >> - return -ENOTSUPP; >> - else if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) >> + insn = *(probes_opcode_t *)(&auprobe->insn[0]); >> + >> + if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) >> return -EINVAL; >> - insn = *(probes_opcode_t *)(&auprobe->insn[0]); >> + /* check if AARCH32 */ >> + if (is_compat_task()) { >> + >> + /* Thumb is not supported yet */ >> + if (addr & 0x3) > > I'm only skimming, so forgive me if I'm missing something which should > be obvious, but this has a big red flag all over it. If "addr" is the > actual instruction address (or even a branch target, for a > non-interworking branch), plenty of Thumb instructions will just happen > to lie at 4-byte-aligned addresses anyway. > That's the same way Thumb instructions are filtered out in arch/arm uprobes and kprobes code. I believe that at this point all Thumb instruction have bit 0 set. Please correct me if I'm wrong. > Furthermore, how would this check ever catch anything anyway given > !IS_ALIGNED(addr, AARCH64_INSN_SIZE) above? You're right, there's no point in checking it here. I'll fix it in v3. Thank you, Maciej ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing 2018-10-01 13:40 ` Maciej Slodczyk @ 2018-10-02 11:04 ` Robin Murphy 0 siblings, 0 replies; 18+ messages in thread From: Robin Murphy @ 2018-10-02 11:04 UTC (permalink / raw) To: Maciej Slodczyk, linux-arm-kernel, linux-kernel Cc: b.zolnierkie, peterz, catalin.marinas, will.deacon, linux, acme, oleg, alexander.shishkin, mingo, k.lewandowsk, namhyung, jolsa, m.szyprowski On 2018-10-01 2:40 PM, Maciej Slodczyk wrote: > Hi Robin, > > Thank you for having a look at my patchset. > > On 27.09.2018 19:01, Robin Murphy wrote: >> On 26/09/18 13:12, Maciej Slodczyk wrote: >> [...] >>> @@ -38,16 +78,44 @@ int arch_uprobe_analyze_insn(struct arch_uprobe >>> *auprobe, struct mm_struct *mm, >>> unsigned long addr) >>> { >>> probes_opcode_t insn; >>> + enum probes_insn retval; >>> + unsigned int bpinsn; >>> - /* TODO: Currently we do not support AARCH32 instruction probing */ >>> - if (mm->context.flags & MMCF_AARCH32) >>> - return -ENOTSUPP; >>> - else if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) >>> + insn = *(probes_opcode_t *)(&auprobe->insn[0]); >>> + >>> + if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE)) >>> return -EINVAL; >>> - insn = *(probes_opcode_t *)(&auprobe->insn[0]); >>> + /* check if AARCH32 */ >>> + if (is_compat_task()) { >>> + >>> + /* Thumb is not supported yet */ >>> + if (addr & 0x3) >> >> I'm only skimming, so forgive me if I'm missing something which should >> be obvious, but this has a big red flag all over it. If "addr" is the >> actual instruction address (or even a branch target, for a >> non-interworking branch), plenty of Thumb instructions will just happen >> to lie at 4-byte-aligned addresses anyway. >> > That's the same way Thumb instructions are filtered out in arch/arm > uprobes and kprobes code. I believe that at this point all Thumb > instruction have bit 0 set. Please correct me if I'm wrong. No, only Thumb *symbols* have bit 0 set. AFAICS [ku]probes are dealing with arbitrary instruction *addresses* here, which will always be 2- or 4-byte aligned. Besides, even if that was the case, testing against 0x3 would be misleading if 0x1 is sufficient. The existing code in arch/arm/ looks just as wrong. >> Furthermore, how would this check ever catch anything anyway given >> !IS_ALIGNED(addr, AARCH64_INSN_SIZE) above? > > You're right, there's no point in checking it here. I'll fix it in v3. Although it still won't solve the problem of potentially dumping an A32 breakpoint in the middle of suitably-aligned T32 code. TBH I'm not sure it's even possible to solve that in the general case :( Robin. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-10-02 11:05 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20180926121213eucas1p1e85f71d1187eb6b50c320377e5ea907f@eucas1p1.samsung.com>
2018-09-26 12:11 ` [PATCH v2 0/7] arm64: uprobes - ARM32 instruction probing Maciej Slodczyk
[not found] ` <CGME20180926121214eucas1p2b262936ddd96980b7b4369f16b52c6ce@eucas1p2.samsung.com>
2018-09-26 12:11 ` [PATCH v2 1/7] arm64: move arm uprobes code to be reused by arm64 Maciej Slodczyk
2018-09-29 9:37 ` Suzuki K Poulose
2018-10-01 13:12 ` Maciej Slodczyk
[not found] ` <CGME20180926121214eucas1p1660542d20425551038da8d3feaf7e1b7@eucas1p1.samsung.com>
2018-09-26 12:12 ` [PATCH v2 2/7] arm64: uprobes - fix checkpatch issues Maciej Slodczyk
2018-09-29 9:39 ` Suzuki K Poulose
[not found] ` <CGME20180926121215eucas1p10437d5bd9db81bedbcc363d24d196ded@eucas1p1.samsung.com>
2018-09-26 12:12 ` [PATCH v2 3/7] arm64: introduce get_swbp_insn() instead of static assignment Maciej Slodczyk
[not found] ` <CGME20180926121216eucas1p28c13ab1a21ac5ef5058206b92954604f@eucas1p2.samsung.com>
2018-09-26 12:12 ` [PATCH v2 4/7] arm64: change arm64 probes handler prototype Maciej Slodczyk
[not found] ` <CGME20180926121216eucas1p2b896ce19f49214d497721db9d6e59bfb@eucas1p2.samsung.com>
2018-09-26 12:12 ` [PATCH v2 5/7] arm64: make arm uprobes code reusable by arm64 Maciej Slodczyk
2018-09-27 15:52 ` Julien Thierry
2018-10-01 13:28 ` Maciej Slodczyk
2018-10-02 8:08 ` Julien Thierry
[not found] ` <CGME20180926121217eucas1p198d96ed637d1aa8a98c1b90466dde745@eucas1p1.samsung.com>
2018-09-26 12:12 ` [PATCH v2 6/7] arm64: change arm_probe_decode_insn() function name Maciej Slodczyk
[not found] ` <CGME20180926121218eucas1p1b20a88cfec17c6403a35e4f23de96ade@eucas1p1.samsung.com>
2018-09-26 12:12 ` [PATCH v2 7/7] arm64: uprobes - ARM32 instruction probing Maciej Slodczyk
2018-09-27 16:18 ` Julien Thierry
2018-09-27 17:01 ` Robin Murphy
2018-10-01 13:40 ` Maciej Slodczyk
2018-10-02 11:04 ` Robin Murphy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).