[PATCH v3 3/3] ARM: kprobes: disallow probing stack consuming instructions

Jon Medhurst (Tixy) tixy at linaro.org
Fri Nov 21 09:59:57 PST 2014


On Fri, 2014-11-21 at 14:35 +0800, Wang Nan wrote:
> This patch prohibit probing instructions for which the stack

Needs an 's' on the end of 'prohibit'

> requirement are unable to be determined statically. Some test cases

and another 's' on the end of 'requirement'

> are found not work again after the modification, this patch also
> removes them.
> 
> Signed-off-by: Wang Nan <wangnan0 at huawei.com>

Reviewed-by: Jon Medhurst <tixy at linaro.org>

I think we also need to add new test cases to cover the stack store
instructions. I have already produced these (which is how I found the
Thumb decoding bugs) so I will follow up with a separate patch with
those. It would be good if you could that it to the end of your series.

Thanks

-- 
Tixy

> ---
> 
> v1 -> v2:
>  - Use MAX_STACK_SIZE macro instead of hard coded stack size.
> 
> v2 -> v3:
>  - Commit message improvements.
> ---
>  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;
>  }
>  





More information about the linux-arm-kernel mailing list