[PATCH 1/2] powerpc/jprobes: Save and restore the parameter save area

[PATCH 1/2] powerpc/jprobes: Save and restore the parameter save area

[Naveen N. Rao]

As pointed out in x86 setjmp_pre_handler(), we need to save and restore
the parameter save area since the jprobe hook might overwrite it. Since
there is no easy way to identify the size of the parameter save area,
we choose to save/restore a fixed 16 [double]word-sized area including
the stack frame header.

We introduce STACK_FRAME_PARM_SAVE to encode the offset of the parameter
save area from the stack frame pointer. Remove the similarly named
PARAMETER_SAVE_AREA_OFFSET in ptrace.c as those are currently not used
anywhere.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
Michael,
I've set the limit to 16 parameters as being a "reasonable" number, but
we could very well make this 24 or 32 if we want to be sure. Let me
know what you prefer.

Thanks,
Naveen

 arch/powerpc/include/asm/kprobes.h | 10 ++++++++++
 arch/powerpc/include/asm/ptrace.h  |  7 +++++++
 arch/powerpc/kernel/kprobes.c      |  7 +++++++
 arch/powerpc/kernel/ptrace.c       | 10 ----------
 4 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
index a83821f33ea3..b6960ef213ac 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -61,6 +61,15 @@ extern kprobe_opcode_t optprobe_template_end[];
 #define MAX_OPTINSN_SIZE	(optprobe_template_end - optprobe_template_entry)
 #define RELATIVEJUMP_SIZE	sizeof(kprobe_opcode_t)	/* 4 bytes */
 
+/* Save upto 16 parameters along with the stack frame header */
+#define MAX_STACK_SIZE		(STACK_FRAME_PARM_SAVE + (16 * sizeof(unsigned long)))
+#define MIN_STACK_SIZE(ADDR)					       \
+	(((MAX_STACK_SIZE) < (((unsigned long)current_thread_info()) + \
+			      THREAD_SIZE - (unsigned long)(ADDR)))    \
+	 ? (MAX_STACK_SIZE)					       \
+	 : (((unsigned long)current_thread_info()) +		       \
+	    THREAD_SIZE - (unsigned long)(ADDR)))
+
 #define flush_insn_slot(p)	do { } while (0)
 #define kretprobe_blacklist_size 0
 
@@ -90,6 +99,7 @@ struct kprobe_ctlblk {
 	unsigned long kprobe_saved_msr;
 	struct pt_regs jprobe_saved_regs;
 	struct prev_kprobe prev_kprobe;
+	u8 jprobes_stack[MAX_STACK_SIZE];
 };
 
 struct arch_optimized_insn {
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index e4923686e43a..a50d0514a181 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -49,8 +49,14 @@
 
 #ifdef PPC64_ELF_ABI_v2
 #define STACK_FRAME_MIN_SIZE	32
+/*
+ * The parameter save area on the stack is used to store arguments being passed
+ * to callee function and is located at fixed offset from stack pointer.
+ */
+#define STACK_FRAME_PARM_SAVE	32
 #else
 #define STACK_FRAME_MIN_SIZE	STACK_FRAME_OVERHEAD
+#define STACK_FRAME_PARM_SAVE	48
 #endif
 
 /* Size of dummy stack frame allocated when calling signal handler. */
@@ -67,6 +73,7 @@
 #define STACK_INT_FRAME_SIZE	(sizeof(struct pt_regs) + STACK_FRAME_OVERHEAD)
 #define STACK_FRAME_MARKER	2
 #define STACK_FRAME_MIN_SIZE	STACK_FRAME_OVERHEAD
+#define STACK_FRAME_PARM_SAVE	8
 
 /* Size of stack frame allocated when calling signal handler. */
 #define __SIGNAL_FRAMESIZE	64
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 4398ea60b4e0..19b053475758 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -37,6 +37,7 @@
 #include <asm/sstep.h>
 #include <asm/sections.h>
 #include <linux/uaccess.h>
+#include <asm/bitops.h>
 
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
@@ -598,8 +599,10 @@ int setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 {
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+	unsigned long sp = kernel_stack_pointer(regs);
 
 	memcpy(&kcb->jprobe_saved_regs, regs, sizeof(struct pt_regs));
+	memcpy(&kcb->jprobes_stack, (void *)sp, MIN_STACK_SIZE(sp));
 
 	/* setup return addr to the jprobe handler routine */
 	regs->nip = arch_deref_entry_point(jp->entry);
@@ -636,6 +639,7 @@ NOKPROBE_SYMBOL(jprobe_return_end);
 int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 {
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+	unsigned long sp;
 
 	/*
 	 * FIXME - we should ideally be validating that we got here 'cos
@@ -643,6 +647,9 @@ int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 	 * saved regs...
 	 */
 	memcpy(regs, &kcb->jprobe_saved_regs, sizeof(struct pt_regs));
+	sp = kernel_stack_pointer(regs);
+	memcpy((void *)sp, &kcb->jprobes_stack, MIN_STACK_SIZE(sp));
+
 	/* It's OK to start function graph tracing again */
 	unpause_graph_tracing();
 	preempt_enable_no_resched();
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 925a4ef90559..7631318b5f37 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -44,16 +44,6 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>
 
-/*
- * The parameter save area on the stack is used to store arguments being passed
- * to callee function and is located at fixed offset from stack pointer.
- */
-#ifdef CONFIG_PPC32
-#define PARAMETER_SAVE_AREA_OFFSET	24  /* bytes */
-#else /* CONFIG_PPC32 */
-#define PARAMETER_SAVE_AREA_OFFSET	48  /* bytes */
-#endif
-
 struct pt_regs_offset {
 	const char *name;
 	int offset;
-- 
2.12.2

[PATCH 2/2] powerpc/jprobes: Validate break handler invocation as being due to a jprobe_return()

[Naveen N. Rao]

Fix a circa 2005 FIXME by implementing a check to ensure that we
actually got into the jprobe break handler() due to the trap in
jprobe_return().

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 19b053475758..1ebeb8c482db 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -627,25 +627,23 @@ NOKPROBE_SYMBOL(setjmp_pre_handler);
 
 void __used jprobe_return(void)
 {
-	asm volatile("trap" ::: "memory");
+	asm volatile("jprobe_return_trap:\n"
+		     "trap\n"
+		     ::: "memory");
 }
 NOKPROBE_SYMBOL(jprobe_return);
 
-static void __used jprobe_return_end(void)
-{
-}
-NOKPROBE_SYMBOL(jprobe_return_end);
-
 int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 {
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 	unsigned long sp;
 
-	/*
-	 * FIXME - we should ideally be validating that we got here 'cos
-	 * of the "trap" in jprobe_return() above, before restoring the
-	 * saved regs...
-	 */
+	if (regs->nip != ppc_kallsyms_lookup_name("jprobe_return_trap")) {
+		WARN(1, "longjmp_break_handler NIP (0x%lx) does not match jprobe_return_trap (0x%lx)\n",
+				regs->nip, ppc_kallsyms_lookup_name("jprobe_return_trap"));
+		return 0;
+	}
+
 	memcpy(regs, &kcb->jprobe_saved_regs, sizeof(struct pt_regs));
 	sp = kernel_stack_pointer(regs);
 	memcpy((void *)sp, &kcb->jprobes_stack, MIN_STACK_SIZE(sp));
-- 
2.12.2

Re: [PATCH 1/2] powerpc/jprobes: Save and restore the parameter save area

[Masami Hiramatsu]

On Mon, 15 May 2017 23:35:03 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
> index a83821f33ea3..b6960ef213ac 100644
> --- a/arch/powerpc/include/asm/kprobes.h
> +++ b/arch/powerpc/include/asm/kprobes.h
> @@ -61,6 +61,15 @@ extern kprobe_opcode_t optprobe_template_end[];
>  #define MAX_OPTINSN_SIZE	(optprobe_template_end - optprobe_template_entry)
>  #define RELATIVEJUMP_SIZE	sizeof(kprobe_opcode_t)	/* 4 bytes */
>  
> +/* Save upto 16 parameters along with the stack frame header */
> +#define MAX_STACK_SIZE		(STACK_FRAME_PARM_SAVE + (16 * sizeof(unsigned long)))
> +#define MIN_STACK_SIZE(ADDR)					       \
> +	(((MAX_STACK_SIZE) < (((unsigned long)current_thread_info()) + \
> +			      THREAD_SIZE - (unsigned long)(ADDR)))    \
> +	 ? (MAX_STACK_SIZE)					       \
> +	 : (((unsigned long)current_thread_info()) +		       \
> +	    THREAD_SIZE - (unsigned long)(ADDR)))

Could you add CUR_STACK_SIZE(addr) as x86 does instead of repeating similar code?

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] powerpc/jprobes: Validate break handler invocation as being due to a jprobe_return()

[Masami Hiramatsu]

On Mon, 15 May 2017 23:35:04 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> Fix a circa 2005 FIXME by implementing a check to ensure that we
> actually got into the jprobe break handler() due to the trap in
> jprobe_return().
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/kprobes.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 19b053475758..1ebeb8c482db 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -627,25 +627,23 @@ NOKPROBE_SYMBOL(setjmp_pre_handler);
>  
>  void __used jprobe_return(void)
>  {
> -	asm volatile("trap" ::: "memory");
> +	asm volatile("jprobe_return_trap:\n"
> +		     "trap\n"
> +		     ::: "memory");
>  }
>  NOKPROBE_SYMBOL(jprobe_return);
>  
> -static void __used jprobe_return_end(void)
> -{
> -}
> -NOKPROBE_SYMBOL(jprobe_return_end);
> -
>  int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
>  {
>  	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>  	unsigned long sp;
>  
> -	/*
> -	 * FIXME - we should ideally be validating that we got here 'cos
> -	 * of the "trap" in jprobe_return() above, before restoring the
> -	 * saved regs...
> -	 */
> +	if (regs->nip != ppc_kallsyms_lookup_name("jprobe_return_trap")) {
> +		WARN(1, "longjmp_break_handler NIP (0x%lx) does not match jprobe_return_trap (0x%lx)\n",
> +				regs->nip, ppc_kallsyms_lookup_name("jprobe_return_trap"));
> +		return 0;

If you don't handle this break, you shouldn't warn it, because
program_check_exception() will continue to find how to handle it
by notify_die(). IOW, please return silently, or just add a
debug message.

Thank you,

> +	}
> +
>  	memcpy(regs, &kcb->jprobe_saved_regs, sizeof(struct pt_regs));
>  	sp = kernel_stack_pointer(regs);
>  	memcpy((void *)sp, &kcb->jprobes_stack, MIN_STACK_SIZE(sp));
> -- 
> 2.12.2
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 1/2] powerpc/jprobes: Save and restore the parameter save area

[Michael Ellerman]

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> As pointed out in x86 setjmp_pre_handler(), we need to save and restore
> the parameter save area since the jprobe hook might overwrite it. Since
> there is no easy way to identify the size of the parameter save area,
> we choose to save/restore a fixed 16 [double]word-sized area including
> the stack frame header.
>
> We introduce STACK_FRAME_PARM_SAVE to encode the offset of the parameter
> save area from the stack frame pointer. Remove the similarly named
> PARAMETER_SAVE_AREA_OFFSET in ptrace.c as those are currently not used
> anywhere.
>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> Michael,
> I've set the limit to 16 parameters as being a "reasonable" number, but
> we could very well make this 24 or 32 if we want to be sure. Let me
> know what you prefer.

That sounds incredibly fragile. Are we really just guessing at the size
required? What happens if we under estimate, do we crash, silently
corrupt data .. ?

cheers

Re: [PATCH 1/2] powerpc/jprobes: Save and restore the parameter save area

[Naveen N. Rao]

On 2017/05/18 03:22PM, Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
> > As pointed out in x86 setjmp_pre_handler(), we need to save and restore
> > the parameter save area since the jprobe hook might overwrite it. Since
> > there is no easy way to identify the size of the parameter save area,
> > we choose to save/restore a fixed 16 [double]word-sized area including
> > the stack frame header.
> >
> > We introduce STACK_FRAME_PARM_SAVE to encode the offset of the parameter
> > save area from the stack frame pointer. Remove the similarly named
> > PARAMETER_SAVE_AREA_OFFSET in ptrace.c as those are currently not used
> > anywhere.
> >
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> > Michael,
> > I've set the limit to 16 parameters as being a "reasonable" number, but
> > we could very well make this 24 or 32 if we want to be sure. Let me
> > know what you prefer.
> 
> That sounds incredibly fragile. Are we really just guessing at the size
> required? What happens if we under estimate, do we crash, silently
> corrupt data .. ?

There is a _chance_ of crashing/corrupting data, yes. The conditions 
required for that include:
- a function that takes > 8 parameters, hence requiring parameters to be 
  passed on the stack
- the jprobe hook replacing that function having to clobber one of those 
  parameters passed on the stack.

Assuming C code, I am not sure what conditions could trigger gcc to emit 
code that may do the latter. However, per the ABI, this is possible as 
the parameter save area is owned by the callee and the caller can't 
assume that it'll remain intact.

Given the slim probability, I felt that saving/restoring a fixed amount 
of stack is sufficient to mitigate this. The other option is to 
save/restore the entire stack frame, but that may be quite large and 
just a lot of overhead in most cases.

Thoughts?

- Naveen

Re: [PATCH 1/2] powerpc/jprobes: Save and restore the parameter save area

[Naveen N. Rao]

On 2017/05/17 10:12AM, Masami Hiramatsu wrote:
> On Mon, 15 May 2017 23:35:03 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
> > index a83821f33ea3..b6960ef213ac 100644
> > --- a/arch/powerpc/include/asm/kprobes.h
> > +++ b/arch/powerpc/include/asm/kprobes.h
> > @@ -61,6 +61,15 @@ extern kprobe_opcode_t optprobe_template_end[];
> >  #define MAX_OPTINSN_SIZE	(optprobe_template_end - optprobe_template_entry)
> >  #define RELATIVEJUMP_SIZE	sizeof(kprobe_opcode_t)	/* 4 bytes */
> >  
> > +/* Save upto 16 parameters along with the stack frame header */
> > +#define MAX_STACK_SIZE		(STACK_FRAME_PARM_SAVE + (16 * sizeof(unsigned long)))
> > +#define MIN_STACK_SIZE(ADDR)					       \
> > +	(((MAX_STACK_SIZE) < (((unsigned long)current_thread_info()) + \
> > +			      THREAD_SIZE - (unsigned long)(ADDR)))    \
> > +	 ? (MAX_STACK_SIZE)					       \
> > +	 : (((unsigned long)current_thread_info()) +		       \
> > +	    THREAD_SIZE - (unsigned long)(ADDR)))
> 
> Could you add CUR_STACK_SIZE(addr) as x86 does instead of repeating similar code?

Sure, thanks for calling me out on this ;D

- Naveen

Re: [PATCH 2/2] powerpc/jprobes: Validate break handler invocation as being due to a jprobe_return()

[Naveen N. Rao]

On 2017/05/17 10:21AM, Masami Hiramatsu wrote:
> On Mon, 15 May 2017 23:35:04 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > Fix a circa 2005 FIXME by implementing a check to ensure that we
> > actually got into the jprobe break handler() due to the trap in
> > jprobe_return().
> > 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/kprobes.c | 20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> > index 19b053475758..1ebeb8c482db 100644
> > --- a/arch/powerpc/kernel/kprobes.c
> > +++ b/arch/powerpc/kernel/kprobes.c
> > @@ -627,25 +627,23 @@ NOKPROBE_SYMBOL(setjmp_pre_handler);
> >  
> >  void __used jprobe_return(void)
> >  {
> > -	asm volatile("trap" ::: "memory");
> > +	asm volatile("jprobe_return_trap:\n"
> > +		     "trap\n"
> > +		     ::: "memory");
> >  }
> >  NOKPROBE_SYMBOL(jprobe_return);
> >  
> > -static void __used jprobe_return_end(void)
> > -{
> > -}
> > -NOKPROBE_SYMBOL(jprobe_return_end);
> > -
> >  int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
> >  {
> >  	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> >  	unsigned long sp;
> >  
> > -	/*
> > -	 * FIXME - we should ideally be validating that we got here 'cos
> > -	 * of the "trap" in jprobe_return() above, before restoring the
> > -	 * saved regs...
> > -	 */
> > +	if (regs->nip != ppc_kallsyms_lookup_name("jprobe_return_trap")) {
> > +		WARN(1, "longjmp_break_handler NIP (0x%lx) does not match jprobe_return_trap (0x%lx)\n",
> > +				regs->nip, ppc_kallsyms_lookup_name("jprobe_return_trap"));
> > +		return 0;
> 
> If you don't handle this break, you shouldn't warn it, because
> program_check_exception() will continue to find how to handle it
> by notify_die(). IOW, please return silently, or just add a
> debug message.

The only reason this can happen is if we hit a third-party installed 
trap while executing a jprobe hook. And given that we run with premption 
disabled, I felt that this is an unlikely scenario.

The reason for adding the check was to guard against issues with looking 
up the symbol (due to some of the ABI aspects). So, I felt a WARN() 
would be good to have and to make it obvious.

I do see your point though. I'm wondering if pr_info() would be better?

Thanks for the review,
- Naveen