diff mbox

[4/4] ARM: kprobes: disallow probing stack consuming instructions

Message ID 1414219373-20070-5-git-send-email-wangnan0@huawei.com
State New
Headers show

Commit Message

Wang Nan Oct. 25, 2014, 6:42 a.m. UTC 
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.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 arch/arm/kernel/kprobes-test-arm.c | 16 ++++++++++------
 arch/arm/kernel/kprobes.c          |  8 ++++++++
 2 files changed, 18 insertions(+), 6 deletions(-)

Comments

Jon Medhurst (Tixy) Oct. 28, 2014, noon UTC | #1 
On Sat, 2014-10-25 at 14:42 +0800, Wang Nan wrote:
> 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.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
[...] 
> diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
> index 618531d..59f5e64 100644
> --- a/arch/arm/kernel/kprobes.c
> +++ b/arch/arm/kernel/kprobes.c
> @@ -124,6 +124,14 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  		break;
>  	}
>  
> +	/*
> +	 * Unable to instrument insn like 'str r0, [sp, +/-r1]'.
> +	 * __und_svc protects 64 bytes stack, so instrumenting insn
> +	 * likes 'str r0, [sp, #-68]' should be prohibited.
> +	 */
> +	if ((p->ainsn.stack_space < 0) || (p->ainsn.stack_space > 64))

Rather than using the hard coded value 64 it would be safest to create a
#define for the value (in arch/arm/include/asm/kprobes.h) and use that
define in __und_svc too.

Actually, there is already a define there for MAX_STACK_SIZE which is
used as the size of the stack for jprobes and the amount copied by
setjmp_pre_handler and longjmp_break_handler, which looks like it has
the same purpose as we want i.e. allowing space for probed instructions
to push things onto the stack. So we should probably use that define in
this patch and update __und_svc too.

Also, jprobe_return has this comment

 * We allocate some slack between the original SP and start of
 * our fabricated regs. To be precise we want to have worst case
 * covered which is STMFD with all 16 regs so we allocate 2 *
 * sizeof(struct_pt_regs)).
 *
 * This is to prevent any simulated instruction from writing
 * over the regs when they are accessing the stack.

which looking at the code should probably more accurately specified as
sizeof(struct_pt_regs)+MAX_STACK_SIZE rather than
2*sizeof(struct_pt_regs)

I've not looked at the jprobes specific code before, so it would be good
to have someone else's option on what I save above about how
MAX_STACK_SIZE is and should be used.
diff mbox

Patch

diff --git a/arch/arm/kernel/kprobes-test-arm.c b/arch/arm/kernel/kprobes-test-arm.c
index 264c064..59f9b25 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,"")
@@ -639,13 +641,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]")			\
@@ -669,12 +673,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 618531d..59f5e64 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/kernel/kprobes.c
@@ -124,6 +124,14 @@  int __kprobes arch_prepare_kprobe(struct kprobe *p)
 		break;
 	}
 
+	/*
+	 * Unable to instrument insn like 'str r0, [sp, +/-r1]'.
+	 * __und_svc protects 64 bytes stack, so instrumenting insn
+	 * likes 'str r0, [sp, #-68]' should be prohibited.
+	 */
+	if ((p->ainsn.stack_space < 0) || (p->ainsn.stack_space > 64))
+		return -EINVAL;
+
 	return 0;
 }