* [PATCH v2 1/3] ARM: kprobes: introduces checker
2014-11-18 6:19 [PATCH v2 0/3] ARM: kprobes: introduces instruction checker Wang Nan
@ 2014-11-18 6:19 ` Wang Nan
2014-11-20 11:11 ` Jon Medhurst (Tixy)
` (2 more replies)
2014-11-18 6:19 ` [PATCH v2 2/3] ARM: kprobes: collects stack consumption for store instructions Wang Nan
2014-11-18 6:19 ` [PATCH v2 3/3] ARM: kprobes: disallow probing stack consuming instructions Wang Nan
2 siblings, 3 replies; 8+ messages in thread
From: Wang Nan @ 2014-11-18 6:19 UTC (permalink / raw)
To: linux-arm-kernel
This patch introdces 'checker' to decoding phase, and calls checkers
when instruction decoding. This allows further decoding for specific
instructions.
v1 -> v2:
- kprobe checker stubs are introduced in this patch.
Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
arch/arm/kernel/kprobes-arm.c | 2 ++
arch/arm/kernel/kprobes-thumb.c | 3 +++
arch/arm/kernel/kprobes.c | 6 ++++-
arch/arm/kernel/kprobes.h | 7 ++++--
arch/arm/kernel/probes-arm.c | 5 ++--
arch/arm/kernel/probes-arm.h | 3 ++-
arch/arm/kernel/probes-thumb.c | 10 ++++----
arch/arm/kernel/probes-thumb.h | 6 +++--
arch/arm/kernel/probes.c | 51 ++++++++++++++++++++++++++++++++++++++++-
arch/arm/kernel/probes.h | 11 ++++++++-
arch/arm/kernel/uprobes.c | 2 +-
11 files changed, 91 insertions(+), 15 deletions(-)
diff --git a/arch/arm/kernel/kprobes-arm.c b/arch/arm/kernel/kprobes-arm.c
index ac300c6..5a81275 100644
--- a/arch/arm/kernel/kprobes-arm.c
+++ b/arch/arm/kernel/kprobes-arm.c
@@ -341,3 +341,5 @@ const union decode_action kprobes_arm_actions[NUM_PROBES_ARM_ACTIONS] = {
[PROBES_BRANCH] = {.handler = simulate_bbl},
[PROBES_LDMSTM] = {.decoder = kprobe_decode_ldmstm}
};
+
+const struct decode_checker *kprobes_arm_checkers[] = {NULL};
diff --git a/arch/arm/kernel/kprobes-thumb.c b/arch/arm/kernel/kprobes-thumb.c
index 9495d7f..b8ba7d2 100644
--- a/arch/arm/kernel/kprobes-thumb.c
+++ b/arch/arm/kernel/kprobes-thumb.c
@@ -664,3 +664,6 @@ const union decode_action kprobes_t32_actions[NUM_PROBES_T32_ACTIONS] = {
[PROBES_T32_MUL_ADD_LONG] = {
.handler = t32_emulate_rdlo12rdhi8rn16rm0_noflags},
};
+
+const struct decode_checker *kprobes_t32_checkers[] = {NULL};
+const struct decode_checker *kprobes_t16_checkers[] = {NULL};
diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
index 6d64420..f114a3f 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/kernel/kprobes.c
@@ -61,6 +61,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
kprobe_decode_insn_t *decode_insn;
const union decode_action *actions;
int is;
+ const struct decode_checker **checkers;
if (in_exception_text(addr))
return -EINVAL;
@@ -74,9 +75,11 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
insn = __opcode_thumb32_compose(insn, inst2);
decode_insn = thumb32_probes_decode_insn;
actions = kprobes_t32_actions;
+ checkers = kprobes_t32_checkers;
} else {
decode_insn = thumb16_probes_decode_insn;
actions = kprobes_t16_actions;
+ checkers = kprobes_t16_checkers;
}
#else /* !CONFIG_THUMB2_KERNEL */
thumb = false;
@@ -85,12 +88,13 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
insn = __mem_to_opcode_arm(*p->addr);
decode_insn = arm_probes_decode_insn;
actions = kprobes_arm_actions;
+ checkers = kprobes_arm_checkers;
#endif
p->opcode = insn;
p->ainsn.insn = tmp_insn;
- switch ((*decode_insn)(insn, &p->ainsn, true, actions)) {
+ switch ((*decode_insn)(insn, &p->ainsn, true, actions, NULL)) {
case INSN_REJECTED: /* not supported */
return -EINVAL;
diff --git a/arch/arm/kernel/kprobes.h b/arch/arm/kernel/kprobes.h
index 9a2712e..05f3b40 100644
--- a/arch/arm/kernel/kprobes.h
+++ b/arch/arm/kernel/kprobes.h
@@ -36,16 +36,19 @@ kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_probes_insn *asi,
typedef enum probes_insn (kprobe_decode_insn_t)(probes_opcode_t,
struct arch_probes_insn *,
bool,
- const union decode_action *);
+ const union decode_action *,
+ const struct decode_checker *[*]);
#ifdef CONFIG_THUMB2_KERNEL
extern const union decode_action kprobes_t32_actions[];
extern const union decode_action kprobes_t16_actions[];
-
+extern const struct decode_checker *kprobes_t32_checkers[];
+extern const struct decode_checker *kprobes_t16_checkers[];
#else /* !CONFIG_THUMB2_KERNEL */
extern const union decode_action kprobes_arm_actions[];
+extern const struct decode_checker *kprobes_arm_checkers[];
#endif
diff --git a/arch/arm/kernel/probes-arm.c b/arch/arm/kernel/probes-arm.c
index 8eaef81..125feda 100644
--- a/arch/arm/kernel/probes-arm.c
+++ b/arch/arm/kernel/probes-arm.c
@@ -725,10 +725,11 @@ static void __kprobes arm_singlestep(probes_opcode_t insn,
*/
enum probes_insn __kprobes
arm_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
- bool emulate, const union decode_action *actions)
+ bool emulate, const union decode_action *actions,
+ const struct decode_checker *checkers[])
{
asi->insn_singlestep = arm_singlestep;
asi->insn_check_cc = probes_condition_checks[insn>>28];
return probes_decode_insn(insn, asi, probes_decode_arm_table, false,
- emulate, actions);
+ emulate, actions, checkers);
}
diff --git a/arch/arm/kernel/probes-arm.h b/arch/arm/kernel/probes-arm.h
index ace6572..8b11ec4 100644
--- a/arch/arm/kernel/probes-arm.h
+++ b/arch/arm/kernel/probes-arm.h
@@ -68,6 +68,7 @@ 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,
- const union decode_action *actions);
+ const union decode_action *actions,
+ const struct decode_checker *checkers[]);
#endif
diff --git a/arch/arm/kernel/probes-thumb.c b/arch/arm/kernel/probes-thumb.c
index 4131351..a5022b4 100644
--- a/arch/arm/kernel/probes-thumb.c
+++ b/arch/arm/kernel/probes-thumb.c
@@ -863,20 +863,22 @@ static void __kprobes thumb32_singlestep(probes_opcode_t opcode,
enum probes_insn __kprobes
thumb16_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
- bool emulate, const union decode_action *actions)
+ bool emulate, const union decode_action *actions,
+ const struct decode_checker *checkers[])
{
asi->insn_singlestep = thumb16_singlestep;
asi->insn_check_cc = thumb_check_cc;
return probes_decode_insn(insn, asi, probes_decode_thumb16_table, true,
- emulate, actions);
+ emulate, actions, checkers);
}
enum probes_insn __kprobes
thumb32_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
- bool emulate, const union decode_action *actions)
+ bool emulate, const union decode_action *actions,
+ const struct decode_checker *checkers[])
{
asi->insn_singlestep = thumb32_singlestep;
asi->insn_check_cc = thumb_check_cc;
return probes_decode_insn(insn, asi, probes_decode_thumb32_table, true,
- emulate, actions);
+ emulate, actions, checkers);
}
diff --git a/arch/arm/kernel/probes-thumb.h b/arch/arm/kernel/probes-thumb.h
index 7c6f6eb..ccfe3e4 100644
--- a/arch/arm/kernel/probes-thumb.h
+++ b/arch/arm/kernel/probes-thumb.h
@@ -89,9 +89,11 @@ extern const union decode_item probes_decode_thumb16_table[];
enum probes_insn __kprobes
thumb16_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
- bool emulate, const union decode_action *actions);
+ bool emulate, const union decode_action *actions,
+ const struct decode_checker *checkers[]);
enum probes_insn __kprobes
thumb32_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
- bool emulate, const union decode_action *actions);
+ bool emulate, const union decode_action *actions,
+ const struct decode_checker *checkers[]);
#endif
diff --git a/arch/arm/kernel/probes.c b/arch/arm/kernel/probes.c
index a8ab540..02598da 100644
--- a/arch/arm/kernel/probes.c
+++ b/arch/arm/kernel/probes.c
@@ -342,6 +342,31 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
[DECODE_TYPE_REJECT] = sizeof(struct decode_reject)
};
+static int run_checkers(const struct decode_checker *checkers[],
+ int action, probes_opcode_t insn,
+ struct arch_probes_insn *asi,
+ const struct decode_header *h)
+{
+ const struct decode_checker **p;
+
+ if (!checkers)
+ return INSN_GOOD;
+
+ p = checkers;
+ while (*p != NULL) {
+ int retval;
+ probes_check_t *checker_func = (*p)[action].checker;
+
+ retval = INSN_GOOD;
+ if (checker_func)
+ retval = checker_func(insn, asi, h);
+ if (retval == INSN_REJECTED)
+ return retval;
+ p++;
+ }
+ return INSN_GOOD;
+}
+
/*
* probes_decode_insn operates on data tables in order to decode an ARM
* architecture instruction onto which a kprobe has been placed.
@@ -388,11 +413,17 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
int __kprobes
probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
const union decode_item *table, bool thumb,
- bool emulate, const union decode_action *actions)
+ bool emulate, const union decode_action *actions,
+ const struct decode_checker *checkers[])
{
const struct decode_header *h = (struct decode_header *)table;
const struct decode_header *next;
bool matched = false;
+ /*
+ * @insn can be modified by decode_regs. Save its original
+ * value for checkers.
+ */
+ probes_opcode_t origin_insn = insn;
if (emulate)
insn = prepare_emulated_insn(insn, asi, thumb);
@@ -422,18 +453,36 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
}
case DECODE_TYPE_CUSTOM: {
+ int err;
struct decode_custom *d = (struct decode_custom *)h;
+ int action = d->decoder.action;
+
+ err = run_checkers(checkers, action, origin_insn, asi, h);
+ if (err == INSN_REJECTED)
+ return INSN_REJECTED;
return actions[d->decoder.action].decoder(insn, asi, h);
}
case DECODE_TYPE_SIMULATE: {
+ int err;
struct decode_simulate *d = (struct decode_simulate *)h;
+ int action = d->handler.action;
+
+ err = run_checkers(checkers, action, origin_insn, asi, h);
+ if (err == INSN_REJECTED)
+ return INSN_REJECTED;
asi->insn_handler = actions[d->handler.action].handler;
return INSN_GOOD_NO_SLOT;
}
case DECODE_TYPE_EMULATE: {
+ int err;
struct decode_emulate *d = (struct decode_emulate *)h;
+ int action = d->handler.action;
+
+ err = run_checkers(checkers, action, origin_insn, asi, h);
+ if (err == INSN_REJECTED)
+ return INSN_REJECTED;
if (!emulate)
return actions[d->handler.action].decoder(insn,
diff --git a/arch/arm/kernel/probes.h b/arch/arm/kernel/probes.h
index dba9f24..b4bf1f5 100644
--- a/arch/arm/kernel/probes.h
+++ b/arch/arm/kernel/probes.h
@@ -314,6 +314,14 @@ union decode_action {
probes_custom_decode_t *decoder;
};
+typedef enum probes_insn (probes_check_t)(probes_opcode_t,
+ struct arch_probes_insn *,
+ const struct decode_header *);
+
+struct decode_checker {
+ probes_check_t *checker;
+};
+
#define DECODE_END \
{.bits = DECODE_TYPE_END}
@@ -402,6 +410,7 @@ probes_insn_handler_t probes_emulate_none;
int __kprobes
probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
const union decode_item *table, bool thumb, bool emulate,
- const union decode_action *actions);
+ const union decode_action *actions,
+ const struct decode_checker **checkers);
#endif
diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
index 56adf9c..372585a 100644
--- a/arch/arm/kernel/uprobes.c
+++ b/arch/arm/kernel/uprobes.c
@@ -88,7 +88,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
auprobe->ixol[1] = __opcode_to_mem_arm(UPROBE_SS_ARM_INSN);
ret = arm_probes_decode_insn(insn, &auprobe->asi, false,
- uprobes_probes_actions);
+ uprobes_probes_actions, NULL);
switch (ret) {
case INSN_REJECTED:
return -EINVAL;
--
1.8.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 1/3] ARM: kprobes: introduces checker
2014-11-18 6:19 ` [PATCH v2 1/3] ARM: kprobes: introduces checker Wang Nan
@ 2014-11-20 11:11 ` Jon Medhurst (Tixy)
2014-11-20 11:57 ` Masami Hiramatsu
2014-11-20 12:14 ` Masami Hiramatsu
2 siblings, 0 replies; 8+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-11-20 11:11 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 2014-11-18 at 14:19 +0800, Wang Nan wrote:
> This patch introdces 'checker' to decoding phase, and calls checkers
> when instruction decoding. This allows further decoding for specific
> instructions.
>
> v1 -> v2:
> - kprobe checker stubs are introduced in this patch.
Showing changes between versions, like above, is a good thing to do as
it helps patch reviewers, but the changes don't want to appear in the
commit message when they are applied to git. So they should be put
bellow the first '---' where 'git am' will ignore them. This also
applies to some other patches in your series.
As for the actual code in this patch, it looks good to me, there's just
one nit-picking issue below, but when you fix that, and the commit
message to omit the comments on version difference, please also add
Reviewed-by: Jon Medhurst <tixy@linaro.org>
(I'll be commenting on the other patches shortly, but from my first look
I don't expect there to be any significant issues.)
>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
> arch/arm/kernel/kprobes-arm.c | 2 ++
> arch/arm/kernel/kprobes-thumb.c | 3 +++
> arch/arm/kernel/kprobes.c | 6 ++++-
> arch/arm/kernel/kprobes.h | 7 ++++--
> arch/arm/kernel/probes-arm.c | 5 ++--
> arch/arm/kernel/probes-arm.h | 3 ++-
> arch/arm/kernel/probes-thumb.c | 10 ++++----
> arch/arm/kernel/probes-thumb.h | 6 +++--
> arch/arm/kernel/probes.c | 51 ++++++++++++++++++++++++++++++++++++++++-
> arch/arm/kernel/probes.h | 11 ++++++++-
> arch/arm/kernel/uprobes.c | 2 +-
> 11 files changed, 91 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/kernel/kprobes-arm.c b/arch/arm/kernel/kprobes-arm.c
> index ac300c6..5a81275 100644
> --- a/arch/arm/kernel/kprobes-arm.c
> +++ b/arch/arm/kernel/kprobes-arm.c
> @@ -341,3 +341,5 @@ const union decode_action kprobes_arm_actions[NUM_PROBES_ARM_ACTIONS] = {
> [PROBES_BRANCH] = {.handler = simulate_bbl},
> [PROBES_LDMSTM] = {.decoder = kprobe_decode_ldmstm}
> };
> +
> +const struct decode_checker *kprobes_arm_checkers[] = {NULL};
> diff --git a/arch/arm/kernel/kprobes-thumb.c b/arch/arm/kernel/kprobes-thumb.c
> index 9495d7f..b8ba7d2 100644
> --- a/arch/arm/kernel/kprobes-thumb.c
> +++ b/arch/arm/kernel/kprobes-thumb.c
> @@ -664,3 +664,6 @@ const union decode_action kprobes_t32_actions[NUM_PROBES_T32_ACTIONS] = {
> [PROBES_T32_MUL_ADD_LONG] = {
> .handler = t32_emulate_rdlo12rdhi8rn16rm0_noflags},
> };
> +
> +const struct decode_checker *kprobes_t32_checkers[] = {NULL};
> +const struct decode_checker *kprobes_t16_checkers[] = {NULL};
> diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
> index 6d64420..f114a3f 100644
> --- a/arch/arm/kernel/kprobes.c
> +++ b/arch/arm/kernel/kprobes.c
> @@ -61,6 +61,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> kprobe_decode_insn_t *decode_insn;
> const union decode_action *actions;
> int is;
> + const struct decode_checker **checkers;
>
> if (in_exception_text(addr))
> return -EINVAL;
> @@ -74,9 +75,11 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> insn = __opcode_thumb32_compose(insn, inst2);
> decode_insn = thumb32_probes_decode_insn;
> actions = kprobes_t32_actions;
> + checkers = kprobes_t32_checkers;
> } else {
> decode_insn = thumb16_probes_decode_insn;
> actions = kprobes_t16_actions;
> + checkers = kprobes_t16_checkers;
> }
> #else /* !CONFIG_THUMB2_KERNEL */
> thumb = false;
> @@ -85,12 +88,13 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> insn = __mem_to_opcode_arm(*p->addr);
> decode_insn = arm_probes_decode_insn;
> actions = kprobes_arm_actions;
> + checkers = kprobes_arm_checkers;
> #endif
>
> p->opcode = insn;
> p->ainsn.insn = tmp_insn;
>
> - switch ((*decode_insn)(insn, &p->ainsn, true, actions)) {
> + switch ((*decode_insn)(insn, &p->ainsn, true, actions, NULL)) {
> case INSN_REJECTED: /* not supported */
> return -EINVAL;
>
> diff --git a/arch/arm/kernel/kprobes.h b/arch/arm/kernel/kprobes.h
> index 9a2712e..05f3b40 100644
> --- a/arch/arm/kernel/kprobes.h
> +++ b/arch/arm/kernel/kprobes.h
> @@ -36,16 +36,19 @@ kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_probes_insn *asi,
> typedef enum probes_insn (kprobe_decode_insn_t)(probes_opcode_t,
> struct arch_probes_insn *,
> bool,
> - const union decode_action *);
> + const union decode_action *,
> + const struct decode_checker *[*]);
>
> #ifdef CONFIG_THUMB2_KERNEL
>
> extern const union decode_action kprobes_t32_actions[];
> extern const union decode_action kprobes_t16_actions[];
> -
> +extern const struct decode_checker *kprobes_t32_checkers[];
> +extern const struct decode_checker *kprobes_t16_checkers[];
> #else /* !CONFIG_THUMB2_KERNEL */
>
> extern const union decode_action kprobes_arm_actions[];
> +extern const struct decode_checker *kprobes_arm_checkers[];
>
> #endif
>
> diff --git a/arch/arm/kernel/probes-arm.c b/arch/arm/kernel/probes-arm.c
> index 8eaef81..125feda 100644
> --- a/arch/arm/kernel/probes-arm.c
> +++ b/arch/arm/kernel/probes-arm.c
> @@ -725,10 +725,11 @@ static void __kprobes arm_singlestep(probes_opcode_t insn,
> */
> enum probes_insn __kprobes
> arm_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> - bool emulate, const union decode_action *actions)
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[])
> {
> asi->insn_singlestep = arm_singlestep;
> asi->insn_check_cc = probes_condition_checks[insn>>28];
> return probes_decode_insn(insn, asi, probes_decode_arm_table, false,
> - emulate, actions);
> + emulate, actions, checkers);
> }
> diff --git a/arch/arm/kernel/probes-arm.h b/arch/arm/kernel/probes-arm.h
> index ace6572..8b11ec4 100644
> --- a/arch/arm/kernel/probes-arm.h
> +++ b/arch/arm/kernel/probes-arm.h
> @@ -68,6 +68,7 @@ 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,
> - const union decode_action *actions);
> + const union decode_action *actions,
> + const struct decode_checker *checkers[]);
>
> #endif
> diff --git a/arch/arm/kernel/probes-thumb.c b/arch/arm/kernel/probes-thumb.c
> index 4131351..a5022b4 100644
> --- a/arch/arm/kernel/probes-thumb.c
> +++ b/arch/arm/kernel/probes-thumb.c
> @@ -863,20 +863,22 @@ static void __kprobes thumb32_singlestep(probes_opcode_t opcode,
>
> enum probes_insn __kprobes
> thumb16_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> - bool emulate, const union decode_action *actions)
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[])
> {
> asi->insn_singlestep = thumb16_singlestep;
> asi->insn_check_cc = thumb_check_cc;
> return probes_decode_insn(insn, asi, probes_decode_thumb16_table, true,
> - emulate, actions);
> + emulate, actions, checkers);
> }
>
> enum probes_insn __kprobes
> thumb32_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> - bool emulate, const union decode_action *actions)
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[])
> {
> asi->insn_singlestep = thumb32_singlestep;
> asi->insn_check_cc = thumb_check_cc;
> return probes_decode_insn(insn, asi, probes_decode_thumb32_table, true,
> - emulate, actions);
> + emulate, actions, checkers);
> }
> diff --git a/arch/arm/kernel/probes-thumb.h b/arch/arm/kernel/probes-thumb.h
> index 7c6f6eb..ccfe3e4 100644
> --- a/arch/arm/kernel/probes-thumb.h
> +++ b/arch/arm/kernel/probes-thumb.h
> @@ -89,9 +89,11 @@ extern const union decode_item probes_decode_thumb16_table[];
>
> enum probes_insn __kprobes
> thumb16_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> - bool emulate, const union decode_action *actions);
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[]);
> enum probes_insn __kprobes
> thumb32_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> - bool emulate, const union decode_action *actions);
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[]);
>
> #endif
> diff --git a/arch/arm/kernel/probes.c b/arch/arm/kernel/probes.c
> index a8ab540..02598da 100644
> --- a/arch/arm/kernel/probes.c
> +++ b/arch/arm/kernel/probes.c
> @@ -342,6 +342,31 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
> [DECODE_TYPE_REJECT] = sizeof(struct decode_reject)
> };
>
> +static int run_checkers(const struct decode_checker *checkers[],
> + int action, probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + const struct decode_checker **p;
> +
> + if (!checkers)
> + return INSN_GOOD;
> +
> + p = checkers;
> + while (*p != NULL) {
> + int retval;
> + probes_check_t *checker_func = (*p)[action].checker;
> +
> + retval = INSN_GOOD;
> + if (checker_func)
> + retval = checker_func(insn, asi, h);
> + if (retval == INSN_REJECTED)
> + return retval;
> + p++;
> + }
> + return INSN_GOOD;
> +}
> +
> /*
> * probes_decode_insn operates on data tables in order to decode an ARM
> * architecture instruction onto which a kprobe has been placed.
> @@ -388,11 +413,17 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
> int __kprobes
> probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> const union decode_item *table, bool thumb,
> - bool emulate, const union decode_action *actions)
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[])
> {
> const struct decode_header *h = (struct decode_header *)table;
> const struct decode_header *next;
> bool matched = false;
> + /*
> + * @insn can be modified by decode_regs. Save its original
> + * value for checkers.
> + */
> + probes_opcode_t origin_insn = insn;
>
> if (emulate)
> insn = prepare_emulated_insn(insn, asi, thumb);
> @@ -422,18 +453,36 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> }
>
> case DECODE_TYPE_CUSTOM: {
> + int err;
> struct decode_custom *d = (struct decode_custom *)h;
> + int action = d->decoder.action;
> +
> + err = run_checkers(checkers, action, origin_insn, asi, h);
> + if (err == INSN_REJECTED)
> + return INSN_REJECTED;
> return actions[d->decoder.action].decoder(insn, asi, h);
Now action is a local variable, the above "d->decoder.action" can just
be replaced with "action". There are several other places below where
this applies too. (I'm sure the compiler will generate the same code
either way, but seems tidier to me)
> }
>
> case DECODE_TYPE_SIMULATE: {
> + int err;
> struct decode_simulate *d = (struct decode_simulate *)h;
> + int action = d->handler.action;
> +
> + err = run_checkers(checkers, action, origin_insn, asi, h);
> + if (err == INSN_REJECTED)
> + return INSN_REJECTED;
> asi->insn_handler = actions[d->handler.action].handler;
s/d->handler.action/action/ ^^^
> return INSN_GOOD_NO_SLOT;
> }
>
> case DECODE_TYPE_EMULATE: {
> + int err;
> struct decode_emulate *d = (struct decode_emulate *)h;
> + int action = d->handler.action;
> +
> + err = run_checkers(checkers, action, origin_insn, asi, h);
> + if (err == INSN_REJECTED)
> + return INSN_REJECTED;
>
> if (!emulate)
> return actions[d->handler.action].decoder(insn,
s/d->handler.action/action/ ^^^
After changing the above, the line will be short enough that it doesn't
need splitting across two lines so will read:
if (!emulate)
return actions[action].decoder(insn, asi, h);
Finally, there is one more "d->handler.action" immediately after this
that can be replaced but it isn't shown in this patch context.
> diff --git a/arch/arm/kernel/probes.h b/arch/arm/kernel/probes.h
> index dba9f24..b4bf1f5 100644
> --- a/arch/arm/kernel/probes.h
> +++ b/arch/arm/kernel/probes.h
> @@ -314,6 +314,14 @@ union decode_action {
> probes_custom_decode_t *decoder;
> };
>
> +typedef enum probes_insn (probes_check_t)(probes_opcode_t,
> + struct arch_probes_insn *,
> + const struct decode_header *);
> +
> +struct decode_checker {
> + probes_check_t *checker;
> +};
> +
> #define DECODE_END \
> {.bits = DECODE_TYPE_END}
>
> @@ -402,6 +410,7 @@ probes_insn_handler_t probes_emulate_none;
> int __kprobes
> probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> const union decode_item *table, bool thumb, bool emulate,
> - const union decode_action *actions);
> + const union decode_action *actions,
> + const struct decode_checker **checkers);
>
> #endif
> diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
> index 56adf9c..372585a 100644
> --- a/arch/arm/kernel/uprobes.c
> +++ b/arch/arm/kernel/uprobes.c
> @@ -88,7 +88,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> auprobe->ixol[1] = __opcode_to_mem_arm(UPROBE_SS_ARM_INSN);
>
> ret = arm_probes_decode_insn(insn, &auprobe->asi, false,
> - uprobes_probes_actions);
> + uprobes_probes_actions, NULL);
> switch (ret) {
> case INSN_REJECTED:
> return -EINVAL;
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 1/3] ARM: kprobes: introduces checker
2014-11-18 6:19 ` [PATCH v2 1/3] ARM: kprobes: introduces checker Wang Nan
2014-11-20 11:11 ` Jon Medhurst (Tixy)
@ 2014-11-20 11:57 ` Masami Hiramatsu
2014-11-20 12:14 ` Masami Hiramatsu
2 siblings, 0 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2014-11-20 11:57 UTC (permalink / raw)
To: linux-arm-kernel
(2014/11/18 15:19), Wang Nan wrote:
> This patch introdces 'checker' to decoding phase, and calls checkers
> when instruction decoding. This allows further decoding for specific
> instructions.
>
> v1 -> v2:
> - kprobe checker stubs are introduced in this patch.
Hmm, could you state this patch just introduce a "stub" call of the
checker in the patch description? (not only version comment)
Other parts are OK to me.
Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Thank you,
>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
> arch/arm/kernel/kprobes-arm.c | 2 ++
> arch/arm/kernel/kprobes-thumb.c | 3 +++
> arch/arm/kernel/kprobes.c | 6 ++++-
> arch/arm/kernel/kprobes.h | 7 ++++--
> arch/arm/kernel/probes-arm.c | 5 ++--
> arch/arm/kernel/probes-arm.h | 3 ++-
> arch/arm/kernel/probes-thumb.c | 10 ++++----
> arch/arm/kernel/probes-thumb.h | 6 +++--
> arch/arm/kernel/probes.c | 51 ++++++++++++++++++++++++++++++++++++++++-
> arch/arm/kernel/probes.h | 11 ++++++++-
> arch/arm/kernel/uprobes.c | 2 +-
> 11 files changed, 91 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/kernel/kprobes-arm.c b/arch/arm/kernel/kprobes-arm.c
> index ac300c6..5a81275 100644
> --- a/arch/arm/kernel/kprobes-arm.c
> +++ b/arch/arm/kernel/kprobes-arm.c
> @@ -341,3 +341,5 @@ const union decode_action kprobes_arm_actions[NUM_PROBES_ARM_ACTIONS] = {
> [PROBES_BRANCH] = {.handler = simulate_bbl},
> [PROBES_LDMSTM] = {.decoder = kprobe_decode_ldmstm}
> };
> +
> +const struct decode_checker *kprobes_arm_checkers[] = {NULL};
> diff --git a/arch/arm/kernel/kprobes-thumb.c b/arch/arm/kernel/kprobes-thumb.c
> index 9495d7f..b8ba7d2 100644
> --- a/arch/arm/kernel/kprobes-thumb.c
> +++ b/arch/arm/kernel/kprobes-thumb.c
> @@ -664,3 +664,6 @@ const union decode_action kprobes_t32_actions[NUM_PROBES_T32_ACTIONS] = {
> [PROBES_T32_MUL_ADD_LONG] = {
> .handler = t32_emulate_rdlo12rdhi8rn16rm0_noflags},
> };
> +
> +const struct decode_checker *kprobes_t32_checkers[] = {NULL};
> +const struct decode_checker *kprobes_t16_checkers[] = {NULL};
> diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
> index 6d64420..f114a3f 100644
> --- a/arch/arm/kernel/kprobes.c
> +++ b/arch/arm/kernel/kprobes.c
> @@ -61,6 +61,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> kprobe_decode_insn_t *decode_insn;
> const union decode_action *actions;
> int is;
> + const struct decode_checker **checkers;
>
> if (in_exception_text(addr))
> return -EINVAL;
> @@ -74,9 +75,11 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> insn = __opcode_thumb32_compose(insn, inst2);
> decode_insn = thumb32_probes_decode_insn;
> actions = kprobes_t32_actions;
> + checkers = kprobes_t32_checkers;
> } else {
> decode_insn = thumb16_probes_decode_insn;
> actions = kprobes_t16_actions;
> + checkers = kprobes_t16_checkers;
> }
> #else /* !CONFIG_THUMB2_KERNEL */
> thumb = false;
> @@ -85,12 +88,13 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> insn = __mem_to_opcode_arm(*p->addr);
> decode_insn = arm_probes_decode_insn;
> actions = kprobes_arm_actions;
> + checkers = kprobes_arm_checkers;
> #endif
>
> p->opcode = insn;
> p->ainsn.insn = tmp_insn;
>
> - switch ((*decode_insn)(insn, &p->ainsn, true, actions)) {
> + switch ((*decode_insn)(insn, &p->ainsn, true, actions, NULL)) {
> case INSN_REJECTED: /* not supported */
> return -EINVAL;
>
> diff --git a/arch/arm/kernel/kprobes.h b/arch/arm/kernel/kprobes.h
> index 9a2712e..05f3b40 100644
> --- a/arch/arm/kernel/kprobes.h
> +++ b/arch/arm/kernel/kprobes.h
> @@ -36,16 +36,19 @@ kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_probes_insn *asi,
> typedef enum probes_insn (kprobe_decode_insn_t)(probes_opcode_t,
> struct arch_probes_insn *,
> bool,
> - const union decode_action *);
> + const union decode_action *,
> + const struct decode_checker *[*]);
>
> #ifdef CONFIG_THUMB2_KERNEL
>
> extern const union decode_action kprobes_t32_actions[];
> extern const union decode_action kprobes_t16_actions[];
> -
> +extern const struct decode_checker *kprobes_t32_checkers[];
> +extern const struct decode_checker *kprobes_t16_checkers[];
> #else /* !CONFIG_THUMB2_KERNEL */
>
> extern const union decode_action kprobes_arm_actions[];
> +extern const struct decode_checker *kprobes_arm_checkers[];
>
> #endif
>
> diff --git a/arch/arm/kernel/probes-arm.c b/arch/arm/kernel/probes-arm.c
> index 8eaef81..125feda 100644
> --- a/arch/arm/kernel/probes-arm.c
> +++ b/arch/arm/kernel/probes-arm.c
> @@ -725,10 +725,11 @@ static void __kprobes arm_singlestep(probes_opcode_t insn,
> */
> enum probes_insn __kprobes
> arm_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> - bool emulate, const union decode_action *actions)
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[])
> {
> asi->insn_singlestep = arm_singlestep;
> asi->insn_check_cc = probes_condition_checks[insn>>28];
> return probes_decode_insn(insn, asi, probes_decode_arm_table, false,
> - emulate, actions);
> + emulate, actions, checkers);
> }
> diff --git a/arch/arm/kernel/probes-arm.h b/arch/arm/kernel/probes-arm.h
> index ace6572..8b11ec4 100644
> --- a/arch/arm/kernel/probes-arm.h
> +++ b/arch/arm/kernel/probes-arm.h
> @@ -68,6 +68,7 @@ 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,
> - const union decode_action *actions);
> + const union decode_action *actions,
> + const struct decode_checker *checkers[]);
>
> #endif
> diff --git a/arch/arm/kernel/probes-thumb.c b/arch/arm/kernel/probes-thumb.c
> index 4131351..a5022b4 100644
> --- a/arch/arm/kernel/probes-thumb.c
> +++ b/arch/arm/kernel/probes-thumb.c
> @@ -863,20 +863,22 @@ static void __kprobes thumb32_singlestep(probes_opcode_t opcode,
>
> enum probes_insn __kprobes
> thumb16_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> - bool emulate, const union decode_action *actions)
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[])
> {
> asi->insn_singlestep = thumb16_singlestep;
> asi->insn_check_cc = thumb_check_cc;
> return probes_decode_insn(insn, asi, probes_decode_thumb16_table, true,
> - emulate, actions);
> + emulate, actions, checkers);
> }
>
> enum probes_insn __kprobes
> thumb32_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> - bool emulate, const union decode_action *actions)
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[])
> {
> asi->insn_singlestep = thumb32_singlestep;
> asi->insn_check_cc = thumb_check_cc;
> return probes_decode_insn(insn, asi, probes_decode_thumb32_table, true,
> - emulate, actions);
> + emulate, actions, checkers);
> }
> diff --git a/arch/arm/kernel/probes-thumb.h b/arch/arm/kernel/probes-thumb.h
> index 7c6f6eb..ccfe3e4 100644
> --- a/arch/arm/kernel/probes-thumb.h
> +++ b/arch/arm/kernel/probes-thumb.h
> @@ -89,9 +89,11 @@ extern const union decode_item probes_decode_thumb16_table[];
>
> enum probes_insn __kprobes
> thumb16_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> - bool emulate, const union decode_action *actions);
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[]);
> enum probes_insn __kprobes
> thumb32_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> - bool emulate, const union decode_action *actions);
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[]);
>
> #endif
> diff --git a/arch/arm/kernel/probes.c b/arch/arm/kernel/probes.c
> index a8ab540..02598da 100644
> --- a/arch/arm/kernel/probes.c
> +++ b/arch/arm/kernel/probes.c
> @@ -342,6 +342,31 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
> [DECODE_TYPE_REJECT] = sizeof(struct decode_reject)
> };
>
> +static int run_checkers(const struct decode_checker *checkers[],
> + int action, probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + const struct decode_checker **p;
> +
> + if (!checkers)
> + return INSN_GOOD;
> +
> + p = checkers;
> + while (*p != NULL) {
> + int retval;
> + probes_check_t *checker_func = (*p)[action].checker;
> +
> + retval = INSN_GOOD;
> + if (checker_func)
> + retval = checker_func(insn, asi, h);
> + if (retval == INSN_REJECTED)
> + return retval;
> + p++;
> + }
> + return INSN_GOOD;
> +}
> +
> /*
> * probes_decode_insn operates on data tables in order to decode an ARM
> * architecture instruction onto which a kprobe has been placed.
> @@ -388,11 +413,17 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
> int __kprobes
> probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> const union decode_item *table, bool thumb,
> - bool emulate, const union decode_action *actions)
> + bool emulate, const union decode_action *actions,
> + const struct decode_checker *checkers[])
> {
> const struct decode_header *h = (struct decode_header *)table;
> const struct decode_header *next;
> bool matched = false;
> + /*
> + * @insn can be modified by decode_regs. Save its original
> + * value for checkers.
> + */
> + probes_opcode_t origin_insn = insn;
>
> if (emulate)
> insn = prepare_emulated_insn(insn, asi, thumb);
> @@ -422,18 +453,36 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> }
>
> case DECODE_TYPE_CUSTOM: {
> + int err;
> struct decode_custom *d = (struct decode_custom *)h;
> + int action = d->decoder.action;
> +
> + err = run_checkers(checkers, action, origin_insn, asi, h);
> + if (err == INSN_REJECTED)
> + return INSN_REJECTED;
> return actions[d->decoder.action].decoder(insn, asi, h);
> }
>
> case DECODE_TYPE_SIMULATE: {
> + int err;
> struct decode_simulate *d = (struct decode_simulate *)h;
> + int action = d->handler.action;
> +
> + err = run_checkers(checkers, action, origin_insn, asi, h);
> + if (err == INSN_REJECTED)
> + return INSN_REJECTED;
> asi->insn_handler = actions[d->handler.action].handler;
> return INSN_GOOD_NO_SLOT;
> }
>
> case DECODE_TYPE_EMULATE: {
> + int err;
> struct decode_emulate *d = (struct decode_emulate *)h;
> + int action = d->handler.action;
> +
> + err = run_checkers(checkers, action, origin_insn, asi, h);
> + if (err == INSN_REJECTED)
> + return INSN_REJECTED;
>
> if (!emulate)
> return actions[d->handler.action].decoder(insn,
> diff --git a/arch/arm/kernel/probes.h b/arch/arm/kernel/probes.h
> index dba9f24..b4bf1f5 100644
> --- a/arch/arm/kernel/probes.h
> +++ b/arch/arm/kernel/probes.h
> @@ -314,6 +314,14 @@ union decode_action {
> probes_custom_decode_t *decoder;
> };
>
> +typedef enum probes_insn (probes_check_t)(probes_opcode_t,
> + struct arch_probes_insn *,
> + const struct decode_header *);
> +
> +struct decode_checker {
> + probes_check_t *checker;
> +};
> +
> #define DECODE_END \
> {.bits = DECODE_TYPE_END}
>
> @@ -402,6 +410,7 @@ probes_insn_handler_t probes_emulate_none;
> int __kprobes
> probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> const union decode_item *table, bool thumb, bool emulate,
> - const union decode_action *actions);
> + const union decode_action *actions,
> + const struct decode_checker **checkers);
>
> #endif
> diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
> index 56adf9c..372585a 100644
> --- a/arch/arm/kernel/uprobes.c
> +++ b/arch/arm/kernel/uprobes.c
> @@ -88,7 +88,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> auprobe->ixol[1] = __opcode_to_mem_arm(UPROBE_SS_ARM_INSN);
>
> ret = arm_probes_decode_insn(insn, &auprobe->asi, false,
> - uprobes_probes_actions);
> + uprobes_probes_actions, NULL);
> switch (ret) {
> case INSN_REJECTED:
> return -EINVAL;
>
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt at hitachi.com
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 1/3] ARM: kprobes: introduces checker
2014-11-18 6:19 ` [PATCH v2 1/3] ARM: kprobes: introduces checker Wang Nan
2014-11-20 11:11 ` Jon Medhurst (Tixy)
2014-11-20 11:57 ` Masami Hiramatsu
@ 2014-11-20 12:14 ` Masami Hiramatsu
2 siblings, 0 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2014-11-20 12:14 UTC (permalink / raw)
To: linux-arm-kernel
(2014/11/18 15:19), Wang Nan wrote:
> diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
> index 6d64420..f114a3f 100644
> --- a/arch/arm/kernel/kprobes.c
> +++ b/arch/arm/kernel/kprobes.c
> @@ -61,6 +61,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> kprobe_decode_insn_t *decode_insn;
> const union decode_action *actions;
> int is;
> + const struct decode_checker **checkers;
>
> if (in_exception_text(addr))
> return -EINVAL;
> @@ -74,9 +75,11 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> insn = __opcode_thumb32_compose(insn, inst2);
> decode_insn = thumb32_probes_decode_insn;
> actions = kprobes_t32_actions;
> + checkers = kprobes_t32_checkers;
> } else {
> decode_insn = thumb16_probes_decode_insn;
> actions = kprobes_t16_actions;
> + checkers = kprobes_t16_checkers;
> }
> #else /* !CONFIG_THUMB2_KERNEL */
> thumb = false;
> @@ -85,12 +88,13 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> insn = __mem_to_opcode_arm(*p->addr);
> decode_insn = arm_probes_decode_insn;
> actions = kprobes_arm_actions;
> + checkers = kprobes_arm_checkers;
> #endif
>
> p->opcode = insn;
> p->ainsn.insn = tmp_insn;
>
> - switch ((*decode_insn)(insn, &p->ainsn, true, actions)) {
> + switch ((*decode_insn)(insn, &p->ainsn, true, actions, NULL)) {
BTW, even if checkers always empty at this point, we should pass the checkers to this function.
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt at hitachi.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] ARM: kprobes: collects stack consumption for store instructions
2014-11-18 6:19 [PATCH v2 0/3] ARM: kprobes: introduces instruction checker Wang Nan
2014-11-18 6:19 ` [PATCH v2 1/3] ARM: kprobes: introduces checker Wang Nan
@ 2014-11-18 6:19 ` Wang Nan
2014-11-20 15:22 ` Jon Medhurst (Tixy)
2014-11-18 6:19 ` [PATCH v2 3/3] ARM: kprobes: disallow probing stack consuming instructions Wang Nan
2 siblings, 1 reply; 8+ messages in thread
From: Wang Nan @ 2014-11-18 6:19 UTC (permalink / raw)
To: linux-arm-kernel
This patch use previous introduced checker on store instructions,
record stack consumption informations to arch_probes_insn. With such
information, kprobe opt can decide how much stack needs to be
protected.
v1 -> v2:
- Bugfix and code improvements following Tixy's suggestion. See:
http://www.spinics.net/lists/arm-kernel/msg372912.html
v2 -> v3:
- Totaly reconstructed following Tixy' instruction. See:
https://lkml.org/lkml/2014/10/27/662 .
Add his SOB.
Signed-off-by: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
arch/arm/include/asm/probes.h | 1 +
arch/arm/kernel/Makefile | 6 +-
arch/arm/kernel/kprobes-arm.c | 3 +-
arch/arm/kernel/kprobes-thumb.c | 5 +-
arch/arm/kernel/kprobes.c | 2 +-
arch/arm/kernel/probes-checkers-arm.c | 99 ++++++++++++++++++++++++++++
arch/arm/kernel/probes-checkers-common.c | 87 +++++++++++++++++++++++++
arch/arm/kernel/probes-checkers-thumb.c | 108 +++++++++++++++++++++++++++++++
arch/arm/kernel/probes-checkers.h | 54 ++++++++++++++++
arch/arm/kernel/probes.c | 10 +++
10 files changed, 368 insertions(+), 7 deletions(-)
create mode 100644 arch/arm/kernel/probes-checkers-arm.c
create mode 100644 arch/arm/kernel/probes-checkers-common.c
create mode 100644 arch/arm/kernel/probes-checkers-thumb.c
create mode 100644 arch/arm/kernel/probes-checkers.h
diff --git a/arch/arm/include/asm/probes.h b/arch/arm/include/asm/probes.h
index 806cfe6..ccf9af3 100644
--- a/arch/arm/include/asm/probes.h
+++ b/arch/arm/include/asm/probes.h
@@ -38,6 +38,7 @@ struct arch_probes_insn {
probes_check_cc *insn_check_cc;
probes_insn_singlestep_t *insn_singlestep;
probes_insn_fn_t *insn_fn;
+ int stack_space;
};
#endif
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 38ddd9f..45aed4b 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -52,11 +52,11 @@ obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o insn.o
obj-$(CONFIG_JUMP_LABEL) += jump_label.o insn.o patch.o
obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
obj-$(CONFIG_UPROBES) += probes.o probes-arm.o uprobes.o uprobes-arm.o
-obj-$(CONFIG_KPROBES) += probes.o kprobes.o kprobes-common.o patch.o
+obj-$(CONFIG_KPROBES) += probes.o kprobes.o kprobes-common.o patch.o probes-checkers-common.o
ifdef CONFIG_THUMB2_KERNEL
-obj-$(CONFIG_KPROBES) += kprobes-thumb.o probes-thumb.o
+obj-$(CONFIG_KPROBES) += kprobes-thumb.o probes-thumb.o probes-checkers-thumb.o
else
-obj-$(CONFIG_KPROBES) += kprobes-arm.o probes-arm.o
+obj-$(CONFIG_KPROBES) += kprobes-arm.o probes-arm.o probes-checkers-arm.o
endif
obj-$(CONFIG_ARM_KPROBES_TEST) += test-kprobes.o
test-kprobes-objs := kprobes-test.o
diff --git a/arch/arm/kernel/kprobes-arm.c b/arch/arm/kernel/kprobes-arm.c
index 5a81275..56f78ce 100644
--- a/arch/arm/kernel/kprobes-arm.c
+++ b/arch/arm/kernel/kprobes-arm.c
@@ -64,6 +64,7 @@
#include "kprobes.h"
#include "probes-arm.h"
+#include "probes-checkers.h"
#if __LINUX_ARM_ARCH__ >= 6
#define BLX(reg) "blx "reg" \n\t"
@@ -342,4 +343,4 @@ const union decode_action kprobes_arm_actions[NUM_PROBES_ARM_ACTIONS] = {
[PROBES_LDMSTM] = {.decoder = kprobe_decode_ldmstm}
};
-const struct decode_checker *kprobes_arm_checkers[] = {NULL};
+const struct decode_checker *kprobes_arm_checkers[] = {arm_stack_checker, NULL};
diff --git a/arch/arm/kernel/kprobes-thumb.c b/arch/arm/kernel/kprobes-thumb.c
index b8ba7d2..dbfd3ca 100644
--- a/arch/arm/kernel/kprobes-thumb.c
+++ b/arch/arm/kernel/kprobes-thumb.c
@@ -15,6 +15,7 @@
#include "kprobes.h"
#include "probes-thumb.h"
+#include "probes-checkers.h"
/* These emulation encodings are functionally equivalent... */
#define t32_emulate_rd8rn16rm0ra12_noflags \
@@ -665,5 +666,5 @@ const union decode_action kprobes_t32_actions[NUM_PROBES_T32_ACTIONS] = {
.handler = t32_emulate_rdlo12rdhi8rn16rm0_noflags},
};
-const struct decode_checker *kprobes_t32_checkers[] = {NULL};
-const struct decode_checker *kprobes_t16_checkers[] = {NULL};
+const struct decode_checker *kprobes_t32_checkers[] = {t32_stack_checker, NULL};
+const struct decode_checker *kprobes_t16_checkers[] = {t16_stack_checker, NULL};
diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
index f114a3f..d7bee4b 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/kernel/kprobes.c
@@ -94,7 +94,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
p->opcode = insn;
p->ainsn.insn = tmp_insn;
- switch ((*decode_insn)(insn, &p->ainsn, true, actions, NULL)) {
+ switch ((*decode_insn)(insn, &p->ainsn, true, actions, checkers)) {
case INSN_REJECTED: /* not supported */
return -EINVAL;
diff --git a/arch/arm/kernel/probes-checkers-arm.c b/arch/arm/kernel/probes-checkers-arm.c
new file mode 100644
index 0000000..b6e0bdf
--- /dev/null
+++ b/arch/arm/kernel/probes-checkers-arm.c
@@ -0,0 +1,99 @@
+/*
+ * arch/arm/kernel/probes-checkers-arm.c
+ *
+ * Copyright (C) 2014 Huawei Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include "probes.h"
+#include "probes-arm.h"
+#include "probes-checkers.h"
+
+static enum probes_insn __kprobes arm_check_stack(probes_opcode_t insn,
+ struct arch_probes_insn *asi,
+ const struct decode_header *h)
+{
+ /*
+ * PROBES_LDRSTRD, PROBES_LDMSTM, PROBES_STORE,
+ * PROBES_STORE_EXTRA may get here. Simply mark all normal
+ * insns as STACK_USE_NONE.
+ */
+ static const union decode_item table[] = {
+ /*
+ * 'str{,d,b,h}, rx, [sp, ry]' should be marked as UNKNOWN
+ *
+ * x
+ * STR (register) cccc 011x x0x0 xxxx xxxx xxxx xxxx xxxx
+ * STRB (register) cccc 011x x1x0 xxxx xxxx xxxx xxxx xxxx
+ */
+ DECODE_OR (0x0e10000f, 0x0600000d),
+ DECODE_OR (0x0e1f0000, 0x060d0000),
+
+ /*
+ * x
+ * STRD (register) cccc 000x x0x0 xxxx xxxx xxxx 1111 xxxx
+ * STRH (register) cccc 000x x0x0 xxxx xxxx xxxx 1011 xxxx
+ */
+ DECODE_OR (0x0e5000bf, 0x000000bd),
+ DECODE_CUSTOM (0x0e5f00b0, 0x000d00b0, STACK_USE_UNKNOWN),
+
+ /*
+ * For PROBES_LDMSTMDX, only stmdx sp, [...] need to examine
+ *
+ * Bit B/A (bit 24) encodes arithmetic operation order. 1 means
+ * before, 0 means after.
+ * Bit I/D (bit 23) encodes arithmetic operation. 1 means
+ * increment, 0 means decrement.
+ *
+ * So:
+ * B I
+ * / /
+ * A D | Rn |
+ * stmdx sp, [...] cccc 100x 00x0 xxxx xxxx xxxx xxxx xxxx
+ */
+ DECODE_CUSTOM (0x0edf0000, 0x080d0000, STACK_USE_STMDX),
+
+ /* P U W | Rn | Rt | imm12 |*/
+ /* STR (immediate) cccc 010x x0x0 1101 xxxx xxxx xxxx xxxx */
+ /* STRB (immediate) cccc 010x x1x0 1101 xxxx xxxx xxxx xxxx */
+ /* P U W | Rn | Rt |imm4| |imm4|*/
+ /* STRD (immediate) cccc 000x x1x0 1101 xxxx xxxx 1111 xxxx */
+ /* STRH (immediate) cccc 000x x1x0 1101 xxxx xxxx 1011 xxxx */
+ /*
+ * index = (P == '1'); add = (U == '1').
+ * Above insns with:
+ * index == 0 (str{,d,h} rx, [sp], #+/-imm) or
+ * add == 1 (str{,d,h} rx, [sp, #+<imm>])
+ * should be STACK_USE_NONE.
+ * Only str{,b,d,h} rx,[sp,#-n] (P == 1 and U == 0) are
+ * required to be examined.
+ */
+ /* str{,b} rx,[sp,#-n] cccc 0101 0xx0 1101 xxxx xxxx xxxx xxxx */
+ DECODE_CUSTOM (0x0f9f0000, 0x050d0000, STACK_USE_FIXED_XXX),
+
+ /* str{d,h}rx,[sp,#-n] cccc 0001 01x0 1101 xxxx xxxx 1x11 xxxx */
+ DECODE_CUSTOM (0x0fdf0000, 0x014d0000, STACK_USE_FIXED_X0X),
+
+ /* fall through */
+ DECODE_CUSTOM (0, 0, STACK_USE_NONE),
+ DECODE_END
+ };
+
+ return probes_decode_insn(insn, asi, table, false, false, stack_check_actions, NULL);
+}
+
+const struct decode_checker arm_stack_checker[NUM_PROBES_ARM_ACTIONS] = {
+ [PROBES_LDRSTRD] = {.checker = arm_check_stack},
+ [PROBES_STORE_EXTRA] = {.checker = arm_check_stack},
+ [PROBES_STORE] = {.checker = arm_check_stack},
+ [PROBES_LDMSTM] = {.checker = arm_check_stack},
+};
diff --git a/arch/arm/kernel/probes-checkers-common.c b/arch/arm/kernel/probes-checkers-common.c
new file mode 100644
index 0000000..e46f844
--- /dev/null
+++ b/arch/arm/kernel/probes-checkers-common.c
@@ -0,0 +1,87 @@
+/*
+ * arch/arm/kernel/probes-checkers-common.c
+ *
+ * Copyright (C) 2014 Huawei Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include "probes.h"
+#include "probes-arm.h"
+#include "probes-checkers.h"
+
+enum probes_insn checker_stack_use_none(probes_opcode_t insn,
+ struct arch_probes_insn *asi,
+ const struct decode_header *h)
+{
+ asi->stack_space = 0;
+ return INSN_GOOD_NO_SLOT;
+}
+
+enum probes_insn checker_stack_use_unknown(probes_opcode_t insn,
+ struct arch_probes_insn *asi,
+ const struct decode_header *h)
+{
+ asi->stack_space = -1;
+ return INSN_GOOD_NO_SLOT;
+}
+
+#ifdef CONFIG_THUMB2_KERNEL
+enum probes_insn checker_stack_use_imm_0xx(probes_opcode_t insn,
+ struct arch_probes_insn *asi,
+ const struct decode_header *h)
+{
+ int imm = insn & 0xff;
+ asi->stack_space = imm;
+ return INSN_GOOD_NO_SLOT;
+}
+#else
+enum probes_insn checker_stack_use_imm_x0x(probes_opcode_t insn,
+ struct arch_probes_insn *asi,
+ const struct decode_header *h)
+{
+ int imm = ((insn & 0xf00) >> 4) + (insn & 0xf);
+ asi->stack_space = imm;
+ return INSN_GOOD_NO_SLOT;
+}
+#endif
+
+enum probes_insn checker_stack_use_imm_xxx(probes_opcode_t insn,
+ struct arch_probes_insn *asi,
+ const struct decode_header *h)
+{
+ int imm = insn & 0xfff;
+ asi->stack_space = imm;
+ return INSN_GOOD_NO_SLOT;
+}
+
+enum probes_insn checker_stack_use_stmdx(probes_opcode_t insn,
+ struct arch_probes_insn *asi,
+ const struct decode_header *h)
+{
+ unsigned int reglist = insn & 0xffff;
+ int pbit = insn & (1 << 24);
+ asi->stack_space = (hweight32(reglist) - (!pbit ? 1 : 0)) * 4;
+
+ return INSN_GOOD_NO_SLOT;
+}
+
+const union decode_action stack_check_actions[] = {
+ [STACK_USE_NONE] = {.decoder = checker_stack_use_none},
+ [STACK_USE_UNKNOWN] = {.decoder = checker_stack_use_unknown},
+#ifdef CONFIG_THUMB2_KERNEL
+ [STACK_USE_FIXED_0XX] = {.decoder = checker_stack_use_imm_0xx},
+#else
+ [STACK_USE_FIXED_X0X] = {.decoder = checker_stack_use_imm_x0x},
+#endif
+ [STACK_USE_FIXED_XXX] = {.decoder = checker_stack_use_imm_xxx},
+ [STACK_USE_STMDX] = {.decoder = checker_stack_use_stmdx},
+};
diff --git a/arch/arm/kernel/probes-checkers-thumb.c b/arch/arm/kernel/probes-checkers-thumb.c
new file mode 100644
index 0000000..f5732c0
--- /dev/null
+++ b/arch/arm/kernel/probes-checkers-thumb.c
@@ -0,0 +1,108 @@
+/*
+ * arch/arm/kernel/probes-checkers-arm.c
+ *
+ * Copyright (C) 2014 Huawei Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include "probes.h"
+#include "probes-thumb.h"
+#include "probes-checkers.h"
+
+static enum probes_insn __kprobes t32_check_stack(probes_opcode_t insn,
+ struct arch_probes_insn *asi,
+ const struct decode_header *h)
+{
+ /*
+ * PROBES_T32_LDMSTM, PROBES_T32_LDRDSTRD and PROBES_T32_LDRSTR
+ * may get here. Simply mark all normal insns as STACK_USE_NONE.
+ */
+ static const union decode_item table[] = {
+
+ /*
+ * First, filter out all ldr insns to make our life easier.
+ * Following load insns may come here:
+ * LDM, LDRD, LDR.
+ * In T32 encoding, bit 20 is enough for distinguishing
+ * load and store. All load insns have this bit set, when
+ * all store insns have this bit clear.
+ */
+ DECODE_CUSTOM (0x00100000, 0x00100000, STACK_USE_NONE),
+
+ /*
+ * Mark all 'str{,b,h}, rx, [ra, ab]' as STACK_USE_UNKNOWN
+ * if ra or rb is sp. T32 doesn't encode STRD.
+ */
+ /* | Rn | Rt | | Rm |*/
+ /* STR (register) 1111 1000 0100 xxxx xxxx 0000 00xx xxxx */
+ /* STRB (register) 1111 1000 0000 xxxx xxxx 0000 00xx xxxx */
+ /* STRH (register) 1111 1000 0010 xxxx xxxx 0000 00xx xxxx */
+ /* INVALID INSN 1111 1000 0110 xxxx xxxx 0000 00xx xxxx */
+ /* By Introducing INVALID INSN, bit 21 and 22 can be ignore. */
+ DECODE_OR (0xff9f0fc0, 0xf80d0000),
+ DECODE_CUSTOM (0xff900fcf, 0xf800000d, STACK_USE_UNKNOWN),
+
+
+ /* | Rn | Rt | PUW| imm8 |*/
+ /* STR (imm 8) 1111 1000 0100 xxxx xxxx 110x xxxx xxxx */
+ /* STRB (imm 8) 1111 1000 0000 xxxx xxxx 110x xxxx xxxx */
+ /* STRH (imm 8) 1111 1000 0010 xxxx xxxx 110x xxxx xxxx */
+ /* INVALID INSN 1111 1000 0110 xxxx xxxx 110x xxxx xxxx */
+ /* Only consider U == 0 and P == 1: strx rx, [sp, #-<imm>] */
+ DECODE_CUSTOM (0xff9f0e00, 0xf80d0c00, STACK_USE_FIXED_0XX),
+
+ /* For str{,b,h} (imm 12), offset is always positive, so ignore them. */
+
+ /* P U W | Rn | Rt | Rt2| imm8 |*/
+ /* STRD (immediate) 1110 1001 01x0 1101 xxxx xxxx xxxx xxxx */
+ /* Only consider U == 0 and P == 1 */
+ DECODE_CUSTOM (0xffdf0000, 0xe94d0000, STACK_USE_FIXED_0XX),
+
+ /* | Rn | */
+ /* STMDB 1110 1001 00x0 1101 xxxx xxxx xxxx xxxx */
+ DECODE_CUSTOM (0xffdf0000, 0xe94d0000, STACK_USE_STMDX),
+
+ /* fall through */
+ DECODE_CUSTOM (0, 0, STACK_USE_NONE),
+ DECODE_END
+ };
+
+ return probes_decode_insn(insn, asi, table, false, false, stack_check_actions, NULL);
+}
+
+const struct decode_checker t32_stack_checker[NUM_PROBES_T32_ACTIONS] = {
+ [PROBES_T32_LDMSTM] = {.checker = t32_check_stack},
+ [PROBES_T32_LDRDSTRD] = {.checker = t32_check_stack},
+ [PROBES_T32_LDRSTR] = {.checker = t32_check_stack},
+};
+
+/*
+ * See following comments. This insn must be 'push'.
+ */
+static enum probes_insn __kprobes t16_check_stack(probes_opcode_t insn,
+ struct arch_probes_insn *asi,
+ const struct decode_header *h)
+{
+ unsigned int reglist = insn & 0x1ff;
+ asi->stack_space = hweight32(reglist) * 4;
+ return INSN_GOOD;
+}
+
+/*
+ * T16 encoding is simple: only push insn is possible to use extra
+ * Other insns, like str, can only use r0-r7 as Rn.
+ */
+const struct decode_checker t16_stack_checker[NUM_PROBES_T16_ACTIONS] = {
+ [PROBES_T16_PUSH] = {.checker = t16_check_stack},
+};
+
+
diff --git a/arch/arm/kernel/probes-checkers.h b/arch/arm/kernel/probes-checkers.h
new file mode 100644
index 0000000..69e8171
--- /dev/null
+++ b/arch/arm/kernel/probes-checkers.h
@@ -0,0 +1,54 @@
+/*
+ * arch/arm/kernel/probes-checkers-common.c
+ *
+ * Copyright (C) 2014 Huawei Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+#ifndef _ARM_KERNEL_PROBES_CHECKERS_H
+#define _ARM_KERNEL_PROBES_CHECKERS_H
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include "probes.h"
+
+extern probes_check_t checker_stack_use_none;
+extern probes_check_t checker_stack_use_unknown;
+#ifdef CONFIG_THUMB2_KERNEL
+extern probes_check_t checker_stack_use_imm_0xx;
+#else
+extern probes_check_t checker_stack_use_imm_x0x;
+#endif
+extern probes_check_t checker_stack_use_imm_xxx;
+extern probes_check_t checker_stack_use_stmdx;
+
+enum {
+ STACK_USE_NONE,
+ STACK_USE_UNKNOWN,
+#ifdef CONFIG_THUMB2_KERNEL
+ STACK_USE_FIXED_0XX,
+#else
+ STACK_USE_FIXED_X0X,
+#endif
+ STACK_USE_FIXED_XXX,
+ STACK_USE_STMDX,
+ NUM_STACK_USE_TYPES
+};
+
+extern const union decode_action stack_check_actions[];
+
+#ifndef CONFIG_THUMB2_KERNEL
+extern const struct decode_checker arm_stack_checker[];
+#else
+#endif
+extern const struct decode_checker t32_stack_checker[];
+extern const struct decode_checker t16_stack_checker[];
+#endif
+
diff --git a/arch/arm/kernel/probes.c b/arch/arm/kernel/probes.c
index 02598da..9cd42c9 100644
--- a/arch/arm/kernel/probes.c
+++ b/arch/arm/kernel/probes.c
@@ -425,6 +425,16 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
*/
probes_opcode_t origin_insn = insn;
+ /*
+ * stack_space is initialized to 0 here. Checker functions
+ * should update is value if they find this is a stack store
+ * instruction: positive value means bytes of stack usage,
+ * negitive value means unable to determine stack usage
+ * statically. For instruction doesn't store to stack, checker
+ * do nothing with it.
+ */
+ asi->stack_space = 0;
+
if (emulate)
insn = prepare_emulated_insn(insn, asi, thumb);
--
1.8.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 2/3] ARM: kprobes: collects stack consumption for store instructions
2014-11-18 6:19 ` [PATCH v2 2/3] ARM: kprobes: collects stack consumption for store instructions Wang Nan
@ 2014-11-20 15:22 ` Jon Medhurst (Tixy)
0 siblings, 0 replies; 8+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-11-20 15:22 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 2014-11-18 at 14:19 +0800, Wang Nan wrote:
> This patch use previous introduced checker on store instructions,
> record stack consumption informations to arch_probes_insn.
The above sentence has some grammar issues, can I suggest...
"This patch uses the previously introduced checker functionality on
store instructions to record their stack consumption information to
arch_probes_insn."
Hope you don't mind me correcting things like this.
> With such
> information, kprobe opt can decide how much stack needs to be
> protected.
>
> v1 -> v2:
> - Bugfix and code improvements following Tixy's suggestion. See:
> http://www.spinics.net/lists/arm-kernel/msg372912.html
>
> v2 -> v3:
> - Totaly reconstructed following Tixy' instruction. See:
> https://lkml.org/lkml/2014/10/27/662 .
> Add his SOB.
As I mentioned in other patch, the version summary shouldn't go in the
commit message.
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> ---
Apart from one error in the decoding tables, all my comments below are
minor things about comments and formatting. So with those fixed, your
next (perhaps final) version of this patch can add
Reviewed-by: Jon Medhurst <tixy@linaro.org>
Thanks for your work on this. And I'm glad you managed to come up with
fairly straightforward decoding tables, I was a bit worried that my
suggestion to use those might turn out impractical.
> arch/arm/include/asm/probes.h | 1 +
> arch/arm/kernel/Makefile | 6 +-
> arch/arm/kernel/kprobes-arm.c | 3 +-
> arch/arm/kernel/kprobes-thumb.c | 5 +-
> arch/arm/kernel/kprobes.c | 2 +-
> arch/arm/kernel/probes-checkers-arm.c | 99 ++++++++++++++++++++++++++++
> arch/arm/kernel/probes-checkers-common.c | 87 +++++++++++++++++++++++++
> arch/arm/kernel/probes-checkers-thumb.c | 108 +++++++++++++++++++++++++++++++
> arch/arm/kernel/probes-checkers.h | 54 ++++++++++++++++
> arch/arm/kernel/probes.c | 10 +++
> 10 files changed, 368 insertions(+), 7 deletions(-)
> create mode 100644 arch/arm/kernel/probes-checkers-arm.c
> create mode 100644 arch/arm/kernel/probes-checkers-common.c
> create mode 100644 arch/arm/kernel/probes-checkers-thumb.c
> create mode 100644 arch/arm/kernel/probes-checkers.h
>
> diff --git a/arch/arm/include/asm/probes.h b/arch/arm/include/asm/probes.h
> index 806cfe6..ccf9af3 100644
> --- a/arch/arm/include/asm/probes.h
> +++ b/arch/arm/include/asm/probes.h
> @@ -38,6 +38,7 @@ struct arch_probes_insn {
> probes_check_cc *insn_check_cc;
> probes_insn_singlestep_t *insn_singlestep;
> probes_insn_fn_t *insn_fn;
> + int stack_space;
> };
>
> #endif
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 38ddd9f..45aed4b 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -52,11 +52,11 @@ obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o insn.o
> obj-$(CONFIG_JUMP_LABEL) += jump_label.o insn.o patch.o
> obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
> obj-$(CONFIG_UPROBES) += probes.o probes-arm.o uprobes.o uprobes-arm.o
> -obj-$(CONFIG_KPROBES) += probes.o kprobes.o kprobes-common.o patch.o
> +obj-$(CONFIG_KPROBES) += probes.o kprobes.o kprobes-common.o patch.o probes-checkers-common.o
> ifdef CONFIG_THUMB2_KERNEL
> -obj-$(CONFIG_KPROBES) += kprobes-thumb.o probes-thumb.o
> +obj-$(CONFIG_KPROBES) += kprobes-thumb.o probes-thumb.o probes-checkers-thumb.o
> else
> -obj-$(CONFIG_KPROBES) += kprobes-arm.o probes-arm.o
> +obj-$(CONFIG_KPROBES) += kprobes-arm.o probes-arm.o probes-checkers-arm.o
> endif
> obj-$(CONFIG_ARM_KPROBES_TEST) += test-kprobes.o
> test-kprobes-objs := kprobes-test.o
> diff --git a/arch/arm/kernel/kprobes-arm.c b/arch/arm/kernel/kprobes-arm.c
> index 5a81275..56f78ce 100644
> --- a/arch/arm/kernel/kprobes-arm.c
> +++ b/arch/arm/kernel/kprobes-arm.c
> @@ -64,6 +64,7 @@
>
> #include "kprobes.h"
> #include "probes-arm.h"
> +#include "probes-checkers.h"
>
> #if __LINUX_ARM_ARCH__ >= 6
> #define BLX(reg) "blx "reg" \n\t"
> @@ -342,4 +343,4 @@ const union decode_action kprobes_arm_actions[NUM_PROBES_ARM_ACTIONS] = {
> [PROBES_LDMSTM] = {.decoder = kprobe_decode_ldmstm}
> };
>
> -const struct decode_checker *kprobes_arm_checkers[] = {NULL};
> +const struct decode_checker *kprobes_arm_checkers[] = {arm_stack_checker, NULL};
> diff --git a/arch/arm/kernel/kprobes-thumb.c b/arch/arm/kernel/kprobes-thumb.c
> index b8ba7d2..dbfd3ca 100644
> --- a/arch/arm/kernel/kprobes-thumb.c
> +++ b/arch/arm/kernel/kprobes-thumb.c
> @@ -15,6 +15,7 @@
>
> #include "kprobes.h"
> #include "probes-thumb.h"
> +#include "probes-checkers.h"
>
> /* These emulation encodings are functionally equivalent... */
> #define t32_emulate_rd8rn16rm0ra12_noflags \
> @@ -665,5 +666,5 @@ const union decode_action kprobes_t32_actions[NUM_PROBES_T32_ACTIONS] = {
> .handler = t32_emulate_rdlo12rdhi8rn16rm0_noflags},
> };
>
> -const struct decode_checker *kprobes_t32_checkers[] = {NULL};
> -const struct decode_checker *kprobes_t16_checkers[] = {NULL};
> +const struct decode_checker *kprobes_t32_checkers[] = {t32_stack_checker, NULL};
> +const struct decode_checker *kprobes_t16_checkers[] = {t16_stack_checker, NULL};
> diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
> index f114a3f..d7bee4b 100644
> --- a/arch/arm/kernel/kprobes.c
> +++ b/arch/arm/kernel/kprobes.c
> @@ -94,7 +94,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> p->opcode = insn;
> p->ainsn.insn = tmp_insn;
>
> - switch ((*decode_insn)(insn, &p->ainsn, true, actions, NULL)) {
> + switch ((*decode_insn)(insn, &p->ainsn, true, actions, checkers)) {
> case INSN_REJECTED: /* not supported */
> return -EINVAL;
>
> diff --git a/arch/arm/kernel/probes-checkers-arm.c b/arch/arm/kernel/probes-checkers-arm.c
> new file mode 100644
> index 0000000..b6e0bdf
> --- /dev/null
> +++ b/arch/arm/kernel/probes-checkers-arm.c
> @@ -0,0 +1,99 @@
> +/*
> + * arch/arm/kernel/probes-checkers-arm.c
> + *
> + * Copyright (C) 2014 Huawei Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include "probes.h"
> +#include "probes-arm.h"
> +#include "probes-checkers.h"
> +
> +static enum probes_insn __kprobes arm_check_stack(probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + /*
> + * PROBES_LDRSTRD, PROBES_LDMSTM, PROBES_STORE,
> + * PROBES_STORE_EXTRA may get here. Simply mark all normal
> + * insns as STACK_USE_NONE.
> + */
> + static const union decode_item table[] = {
> + /*
> + * 'str{,d,b,h}, rx, [sp, ry]' should be marked as UNKNOWN
We are also checking for when ry==sp so the above comment should be
replaced with something like...
* 'str{,d,b,h}, rx, [ry, rz]' should be marked as UNKNOWN
* if ry or rz is sp
Or probably best to use the naming convention for registers that is
closer to what ARM documentation uses...
* 'STR{,D,B,H}, Rt, [Rn, Rm]' should be marked as UNKNOWN
* if Rn or Rm is SP
> + *
> + * x
> + * STR (register) cccc 011x x0x0 xxxx xxxx xxxx xxxx xxxx
> + * STRB (register) cccc 011x x1x0 xxxx xxxx xxxx xxxx xxxx
> + */
> + DECODE_OR (0x0e10000f, 0x0600000d),
> + DECODE_OR (0x0e1f0000, 0x060d0000),
> +
> + /*
> + * x
> + * STRD (register) cccc 000x x0x0 xxxx xxxx xxxx 1111 xxxx
> + * STRH (register) cccc 000x x0x0 xxxx xxxx xxxx 1011 xxxx
> + */
> + DECODE_OR (0x0e5000bf, 0x000000bd),
> + DECODE_CUSTOM (0x0e5f00b0, 0x000d00b0, STACK_USE_UNKNOWN),
> +
> + /*
> + * For PROBES_LDMSTMDX, only stmdx sp, [...] need to examine
Think PROBES_LDMSTMDX should be PROBES_LDMSTM as that is what the enum
is called.
> + *
> + * Bit B/A (bit 24) encodes arithmetic operation order. 1 means
> + * before, 0 means after.
> + * Bit I/D (bit 23) encodes arithmetic operation. 1 means
> + * increment, 0 means decrement.
> + *
> + * So:
> + * B I
> + * / /
> + * A D | Rn |
> + * stmdx sp, [...] cccc 100x 00x0 xxxx xxxx xxxx xxxx xxxx
> + */
> + DECODE_CUSTOM (0x0edf0000, 0x080d0000, STACK_USE_STMDX),
> +
> + /* P U W | Rn | Rt | imm12 |*/
> + /* STR (immediate) cccc 010x x0x0 1101 xxxx xxxx xxxx xxxx */
> + /* STRB (immediate) cccc 010x x1x0 1101 xxxx xxxx xxxx xxxx */
> + /* P U W | Rn | Rt |imm4| |imm4|*/
> + /* STRD (immediate) cccc 000x x1x0 1101 xxxx xxxx 1111 xxxx */
> + /* STRH (immediate) cccc 000x x1x0 1101 xxxx xxxx 1011 xxxx */
> + /*
> + * index = (P == '1'); add = (U == '1').
> + * Above insns with:
> + * index == 0 (str{,d,h} rx, [sp], #+/-imm) or
> + * add == 1 (str{,d,h} rx, [sp, #+<imm>])
> + * should be STACK_USE_NONE.
> + * Only str{,b,d,h} rx,[sp,#-n] (P == 1 and U == 0) are
> + * required to be examined.
> + */
> + /* str{,b} rx,[sp,#-n] cccc 0101 0xx0 1101 xxxx xxxx xxxx xxxx */
> + DECODE_CUSTOM (0x0f9f0000, 0x050d0000, STACK_USE_FIXED_XXX),
> +
> + /* str{d,h}rx,[sp,#-n] cccc 0001 01x0 1101 xxxx xxxx 1x11 xxxx */
> + DECODE_CUSTOM (0x0fdf0000, 0x014d0000, STACK_USE_FIXED_X0X),
The above should also be checking the value of bits 4,5 and 7, i.e. be
DECODE_CUSTOM (0x0fdf00b0, 0x014d00b0, STACK_USE_FIXED_X0X),
> +
> + /* fall through */
> + DECODE_CUSTOM (0, 0, STACK_USE_NONE),
> + DECODE_END
> + };
> +
> + return probes_decode_insn(insn, asi, table, false, false, stack_check_actions, NULL);
> +}
> +
> +const struct decode_checker arm_stack_checker[NUM_PROBES_ARM_ACTIONS] = {
> + [PROBES_LDRSTRD] = {.checker = arm_check_stack},
> + [PROBES_STORE_EXTRA] = {.checker = arm_check_stack},
> + [PROBES_STORE] = {.checker = arm_check_stack},
> + [PROBES_LDMSTM] = {.checker = arm_check_stack},
> +};
> diff --git a/arch/arm/kernel/probes-checkers-common.c b/arch/arm/kernel/probes-checkers-common.c
> new file mode 100644
> index 0000000..e46f844
> --- /dev/null
> +++ b/arch/arm/kernel/probes-checkers-common.c
> @@ -0,0 +1,87 @@
> +/*
> + * arch/arm/kernel/probes-checkers-common.c
> + *
> + * Copyright (C) 2014 Huawei Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include "probes.h"
> +#include "probes-arm.h"
> +#include "probes-checkers.h"
> +
> +enum probes_insn checker_stack_use_none(probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + asi->stack_space = 0;
> + return INSN_GOOD_NO_SLOT;
> +}
> +
> +enum probes_insn checker_stack_use_unknown(probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + asi->stack_space = -1;
> + return INSN_GOOD_NO_SLOT;
> +}
> +
> +#ifdef CONFIG_THUMB2_KERNEL
> +enum probes_insn checker_stack_use_imm_0xx(probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + int imm = insn & 0xff;
> + asi->stack_space = imm;
> + return INSN_GOOD_NO_SLOT;
> +}
> +#else
> +enum probes_insn checker_stack_use_imm_x0x(probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + int imm = ((insn & 0xf00) >> 4) + (insn & 0xf);
> + asi->stack_space = imm;
> + return INSN_GOOD_NO_SLOT;
> +}
> +#endif
> +
> +enum probes_insn checker_stack_use_imm_xxx(probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + int imm = insn & 0xfff;
> + asi->stack_space = imm;
> + return INSN_GOOD_NO_SLOT;
> +}
> +
> +enum probes_insn checker_stack_use_stmdx(probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + unsigned int reglist = insn & 0xffff;
> + int pbit = insn & (1 << 24);
> + asi->stack_space = (hweight32(reglist) - (!pbit ? 1 : 0)) * 4;
> +
> + return INSN_GOOD_NO_SLOT;
> +}
> +
> +const union decode_action stack_check_actions[] = {
> + [STACK_USE_NONE] = {.decoder = checker_stack_use_none},
> + [STACK_USE_UNKNOWN] = {.decoder = checker_stack_use_unknown},
> +#ifdef CONFIG_THUMB2_KERNEL
> + [STACK_USE_FIXED_0XX] = {.decoder = checker_stack_use_imm_0xx},
> +#else
> + [STACK_USE_FIXED_X0X] = {.decoder = checker_stack_use_imm_x0x},
> +#endif
> + [STACK_USE_FIXED_XXX] = {.decoder = checker_stack_use_imm_xxx},
> + [STACK_USE_STMDX] = {.decoder = checker_stack_use_stmdx},
> +};
> diff --git a/arch/arm/kernel/probes-checkers-thumb.c b/arch/arm/kernel/probes-checkers-thumb.c
> new file mode 100644
> index 0000000..f5732c0
> --- /dev/null
> +++ b/arch/arm/kernel/probes-checkers-thumb.c
> @@ -0,0 +1,108 @@
> +/*
> + * arch/arm/kernel/probes-checkers-arm.c
> + *
> + * Copyright (C) 2014 Huawei Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include "probes.h"
> +#include "probes-thumb.h"
> +#include "probes-checkers.h"
> +
> +static enum probes_insn __kprobes t32_check_stack(probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + /*
> + * PROBES_T32_LDMSTM, PROBES_T32_LDRDSTRD and PROBES_T32_LDRSTR
> + * may get here. Simply mark all normal insns as STACK_USE_NONE.
> + */
> + static const union decode_item table[] = {
> +
> + /*
> + * First, filter out all ldr insns to make our life easier.
> + * Following load insns may come here:
> + * LDM, LDRD, LDR.
> + * In T32 encoding, bit 20 is enough for distinguishing
> + * load and store. All load insns have this bit set, when
> + * all store insns have this bit clear.
> + */
> + DECODE_CUSTOM (0x00100000, 0x00100000, STACK_USE_NONE),
> +
> + /*
> + * Mark all 'str{,b,h}, rx, [ra, ab]' as STACK_USE_UNKNOWN
There's a typo above, s/ab/rb/ ^^
> + * if ra or rb is sp. T32 doesn't encode STRD.
> + */
> + /* | Rn | Rt | | Rm |*/
> + /* STR (register) 1111 1000 0100 xxxx xxxx 0000 00xx xxxx */
> + /* STRB (register) 1111 1000 0000 xxxx xxxx 0000 00xx xxxx */
> + /* STRH (register) 1111 1000 0010 xxxx xxxx 0000 00xx xxxx */
> + /* INVALID INSN 1111 1000 0110 xxxx xxxx 0000 00xx xxxx */
> + /* By Introducing INVALID INSN, bit 21 and 22 can be ignore. */
Another typo, s/ignore/ignored/ ^^^^^^
> + DECODE_OR (0xff9f0fc0, 0xf80d0000),
> + DECODE_CUSTOM (0xff900fcf, 0xf800000d, STACK_USE_UNKNOWN),
> +
> +
> + /* | Rn | Rt | PUW| imm8 |*/
> + /* STR (imm 8) 1111 1000 0100 xxxx xxxx 110x xxxx xxxx */
> + /* STRB (imm 8) 1111 1000 0000 xxxx xxxx 110x xxxx xxxx */
> + /* STRH (imm 8) 1111 1000 0010 xxxx xxxx 110x xxxx xxxx */
> + /* INVALID INSN 1111 1000 0110 xxxx xxxx 110x xxxx xxxx */
> + /* Only consider U == 0 and P == 1: strx rx, [sp, #-<imm>] */
> + DECODE_CUSTOM (0xff9f0e00, 0xf80d0c00, STACK_USE_FIXED_0XX),
> +
> + /* For str{,b,h} (imm 12), offset is always positive, so ignore them. */
> +
> + /* P U W | Rn | Rt | Rt2| imm8 |*/
> + /* STRD (immediate) 1110 1001 01x0 1101 xxxx xxxx xxxx xxxx */
> + /* Only consider U == 0 and P == 1 */
> + DECODE_CUSTOM (0xffdf0000, 0xe94d0000, STACK_USE_FIXED_0XX),
> +
> + /* | Rn | */
> + /* STMDB 1110 1001 00x0 1101 xxxx xxxx xxxx xxxx */
> + DECODE_CUSTOM (0xffdf0000, 0xe94d0000, STACK_USE_STMDX),
> +
> + /* fall through */
> + DECODE_CUSTOM (0, 0, STACK_USE_NONE),
> + DECODE_END
> + };
> +
> + return probes_decode_insn(insn, asi, table, false, false, stack_check_actions, NULL);
> +}
> +
> +const struct decode_checker t32_stack_checker[NUM_PROBES_T32_ACTIONS] = {
> + [PROBES_T32_LDMSTM] = {.checker = t32_check_stack},
> + [PROBES_T32_LDRDSTRD] = {.checker = t32_check_stack},
> + [PROBES_T32_LDRSTR] = {.checker = t32_check_stack},
> +};
> +
> +/*
> + * See following comments. This insn must be 'push'.
> + */
> +static enum probes_insn __kprobes t16_check_stack(probes_opcode_t insn,
> + struct arch_probes_insn *asi,
> + const struct decode_header *h)
> +{
> + unsigned int reglist = insn & 0x1ff;
> + asi->stack_space = hweight32(reglist) * 4;
> + return INSN_GOOD;
> +}
> +
> +/*
> + * T16 encoding is simple: only push insn is possible to use extra
Above comment doesn't quite make sense to me, how about something
like...
* T16 encoding is simple: only the push insn can need extra stack space
> + * Other insns, like str, can only use r0-r7 as Rn.
> + */
> +const struct decode_checker t16_stack_checker[NUM_PROBES_T16_ACTIONS] = {
> + [PROBES_T16_PUSH] = {.checker = t16_check_stack},
> +};
> +
> +
Those blank lines at end of the file aren't needed. (I only noticed that
because "git am" warned about them as whitespace errors.)
> diff --git a/arch/arm/kernel/probes-checkers.h b/arch/arm/kernel/probes-checkers.h
> new file mode 100644
> index 0000000..69e8171
> --- /dev/null
> +++ b/arch/arm/kernel/probes-checkers.h
> @@ -0,0 +1,54 @@
> +/*
> + * arch/arm/kernel/probes-checkers-common.c
> + *
> + * Copyright (C) 2014 Huawei Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +#ifndef _ARM_KERNEL_PROBES_CHECKERS_H
> +#define _ARM_KERNEL_PROBES_CHECKERS_H
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include "probes.h"
> +
> +extern probes_check_t checker_stack_use_none;
> +extern probes_check_t checker_stack_use_unknown;
> +#ifdef CONFIG_THUMB2_KERNEL
> +extern probes_check_t checker_stack_use_imm_0xx;
> +#else
> +extern probes_check_t checker_stack_use_imm_x0x;
> +#endif
> +extern probes_check_t checker_stack_use_imm_xxx;
> +extern probes_check_t checker_stack_use_stmdx;
> +
> +enum {
> + STACK_USE_NONE,
> + STACK_USE_UNKNOWN,
> +#ifdef CONFIG_THUMB2_KERNEL
> + STACK_USE_FIXED_0XX,
> +#else
> + STACK_USE_FIXED_X0X,
> +#endif
> + STACK_USE_FIXED_XXX,
> + STACK_USE_STMDX,
> + NUM_STACK_USE_TYPES
> +};
> +
> +extern const union decode_action stack_check_actions[];
> +
> +#ifndef CONFIG_THUMB2_KERNEL
> +extern const struct decode_checker arm_stack_checker[];
> +#else
> +#endif
> +extern const struct decode_checker t32_stack_checker[];
> +extern const struct decode_checker t16_stack_checker[];
> +#endif
> +
> diff --git a/arch/arm/kernel/probes.c b/arch/arm/kernel/probes.c
> index 02598da..9cd42c9 100644
> --- a/arch/arm/kernel/probes.c
> +++ b/arch/arm/kernel/probes.c
> @@ -425,6 +425,16 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
> */
> probes_opcode_t origin_insn = insn;
>
> + /*
> + * stack_space is initialized to 0 here. Checker functions
> + * should update is value if they find this is a stack store
> + * instruction: positive value means bytes of stack usage,
> + * negitive value means unable to determine stack usage
> + * statically. For instruction doesn't store to stack, checker
> + * do nothing with it.
> + */
> + asi->stack_space = 0;
> +
> if (emulate)
> insn = prepare_emulated_insn(insn, asi, thumb);
>
Thanks
--
Tixy
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] ARM: kprobes: disallow probing stack consuming instructions
2014-11-18 6:19 [PATCH v2 0/3] ARM: kprobes: introduces instruction checker Wang Nan
2014-11-18 6:19 ` [PATCH v2 1/3] ARM: kprobes: introduces checker Wang Nan
2014-11-18 6:19 ` [PATCH v2 2/3] ARM: kprobes: collects stack consumption for store instructions Wang Nan
@ 2014-11-18 6:19 ` Wang Nan
2 siblings, 0 replies; 8+ messages in thread
From: Wang Nan @ 2014-11-18 6:19 UTC (permalink / raw)
To: linux-arm-kernel
This patch prohibit probing instructions for which the stack
requirement are unable to be determined statically. Some test cases
are found not work again after the modification, this patch also
removes them.
v1 -> v2:
- Use MAX_STACK_SIZE macro instead of hard coded stack size.
Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
arch/arm/include/asm/kprobes.h | 1 -
arch/arm/include/asm/probes.h | 12 ++++++++++++
arch/arm/kernel/entry-armv.S | 3 ++-
arch/arm/kernel/kprobes-test-arm.c | 16 ++++++++++------
arch/arm/kernel/kprobes.c | 9 +++++++++
5 files changed, 33 insertions(+), 8 deletions(-)
diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h
index 49fa0df..56f9ac6 100644
--- a/arch/arm/include/asm/kprobes.h
+++ b/arch/arm/include/asm/kprobes.h
@@ -22,7 +22,6 @@
#define __ARCH_WANT_KPROBES_INSN_SLOT
#define MAX_INSN_SIZE 2
-#define MAX_STACK_SIZE 64 /* 32 would probably be OK */
#define flush_insn_slot(p) do { } while (0)
#define kretprobe_blacklist_size 0
diff --git a/arch/arm/include/asm/probes.h b/arch/arm/include/asm/probes.h
index ccf9af3..f0a1ee8 100644
--- a/arch/arm/include/asm/probes.h
+++ b/arch/arm/include/asm/probes.h
@@ -19,6 +19,8 @@
#ifndef _ASM_PROBES_H
#define _ASM_PROBES_H
+#ifndef __ASSEMBLY__
+
typedef u32 probes_opcode_t;
struct arch_probes_insn;
@@ -41,4 +43,14 @@ struct arch_probes_insn {
int stack_space;
};
+#endif /* __ASSEMBLY__ */
+
+/*
+ * We assume one instruction can consume at most 64 bytes stack, which is
+ * 'push {r0-r15}'. Instructions consume more or unknown stack space like
+ * 'str r0, [sp, #-80]' and 'str r0, [sp, r1]' should be prohibit to probe.
+ * Both kprobe and jprobe use this macro.
+ */
+#define MAX_STACK_SIZE 64
+
#endif
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 2f5555d..672b219 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -31,6 +31,7 @@
#include "entry-header.S"
#include <asm/entry-macro-multi.S>
+#include <asm/probes.h>
/*
* Interrupt handling.
@@ -249,7 +250,7 @@ __und_svc:
@ If a kprobe is about to simulate a "stmdb sp..." instruction,
@ it obviously needs free stack space which then will belong to
@ the saved context.
- svc_entry 64
+ svc_entry MAX_STACK_SIZE
#else
svc_entry
#endif
diff --git a/arch/arm/kernel/kprobes-test-arm.c b/arch/arm/kernel/kprobes-test-arm.c
index cb14242..d8ad5bb 100644
--- a/arch/arm/kernel/kprobes-test-arm.c
+++ b/arch/arm/kernel/kprobes-test-arm.c
@@ -476,7 +476,8 @@ void kprobe_arm_test_cases(void)
TEST_GROUP("Extra load/store instructions")
TEST_RPR( "strh r",0, VAL1,", [r",1, 48,", -r",2, 24,"]")
- TEST_RPR( "streqh r",14,VAL2,", [r",13,0, ", r",12, 48,"]")
+ TEST_RPR( "streqh r",14,VAL2,", [r",11,0, ", r",12, 48,"]")
+ TEST_UNSUPPORTED( "streqh r14, [r13, r12]")
TEST_RPR( "strh r",1, VAL1,", [r",2, 24,", r",3, 48,"]!")
TEST_RPR( "strneh r",12,VAL2,", [r",11,48,", -r",10,24,"]!")
TEST_RPR( "strh r",2, VAL1,", [r",3, 24,"], r",4, 48,"")
@@ -565,7 +566,8 @@ void kprobe_arm_test_cases(void)
#if __LINUX_ARM_ARCH__ >= 5
TEST_RPR( "strd r",0, VAL1,", [r",1, 48,", -r",2,24,"]")
- TEST_RPR( "strccd r",8, VAL2,", [r",13,0, ", r",12,48,"]")
+ TEST_RPR( "strccd r",8, VAL2,", [r",11,0, ", r",12,48,"]")
+ TEST_UNSUPPORTED( "strccd r8, [r13, r12]")
TEST_RPR( "strd r",4, VAL1,", [r",2, 24,", r",3, 48,"]!")
TEST_RPR( "strcsd r",12,VAL2,", [r",11,48,", -r",10,24,"]!")
TEST_RPR( "strd r",2, VAL1,", [r",5, 24,"], r",4,48,"")
@@ -638,13 +640,15 @@ void kprobe_arm_test_cases(void)
TEST_RP( "str"byte" r",2, VAL1,", [r",3, 24,"], #48") \
TEST_RP( "str"byte" r",10,VAL2,", [r",9, 64,"], #-48") \
TEST_RPR("str"byte" r",0, VAL1,", [r",1, 48,", -r",2, 24,"]") \
- TEST_RPR("str"byte" r",14,VAL2,", [r",13,0, ", r",12, 48,"]") \
+ TEST_RPR("str"byte" r",14,VAL2,", [r",11,0, ", r",12, 48,"]") \
+ TEST_UNSUPPORTED("str"byte" r14, [r13, r12]") \
TEST_RPR("str"byte" r",1, VAL1,", [r",2, 24,", r",3, 48,"]!") \
TEST_RPR("str"byte" r",12,VAL2,", [r",11,48,", -r",10,24,"]!") \
TEST_RPR("str"byte" r",2, VAL1,", [r",3, 24,"], r",4, 48,"") \
TEST_RPR("str"byte" r",10,VAL2,", [r",9, 48,"], -r",11,24,"") \
TEST_RPR("str"byte" r",0, VAL1,", [r",1, 24,", r",2, 32,", asl #1]")\
- TEST_RPR("str"byte" r",14,VAL2,", [r",13,0, ", r",12, 32,", lsr #2]")\
+ TEST_RPR("str"byte" r",14,VAL2,", [r",11,0, ", r",12, 32,", lsr #2]")\
+ TEST_UNSUPPORTED("str"byte" r14, [r13, r12, lsr #2]")\
TEST_RPR("str"byte" r",1, VAL1,", [r",2, 24,", r",3, 32,", asr #3]!")\
TEST_RPR("str"byte" r",12,VAL2,", [r",11,24,", r",10, 4,", ror #31]!")\
TEST_P( "ldr"byte" r0, [r",0, 24,", #-2]") \
@@ -668,12 +672,12 @@ void kprobe_arm_test_cases(void)
LOAD_STORE("")
TEST_P( "str pc, [r",0,0,", #15*4]")
- TEST_R( "str pc, [sp, r",2,15*4,"]")
+ TEST_UNSUPPORTED( "str pc, [sp, r2]")
TEST_BF( "ldr pc, [sp, #15*4]")
TEST_BF_R("ldr pc, [sp, r",2,15*4,"]")
TEST_P( "str sp, [r",0,0,", #13*4]")
- TEST_R( "str sp, [sp, r",2,13*4,"]")
+ TEST_UNSUPPORTED( "str sp, [sp, r2]")
TEST_BF( "ldr sp, [sp, #13*4]")
TEST_BF_R("ldr sp, [sp, r",2,13*4,"]")
diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
index d7bee4b..30498436 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/kernel/kprobes.c
@@ -115,6 +115,15 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
break;
}
+ /*
+ * Never instrument insn like 'str r0, [sp, +/-r1]'. Also, insn likes
+ * 'str r0, [sp, #-68]' should also be prohibited.
+ * See __und_svc.
+ */
+ if ((p->ainsn.stack_space < 0) ||
+ (p->ainsn.stack_space > MAX_STACK_SIZE))
+ return -EINVAL;
+
return 0;
}
--
1.8.4
^ permalink raw reply related [flat|nested] 8+ messages in thread