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

Submitted by Naveen N. Rao on May 15, 2017, 6:05 p.m.

Details

Message ID 0572aa31dd57bef6f158ac97d0b666a0c545356a.1494871248.git.naveen.n.rao@linux.vnet.ibm.com
State New
Headers show

Commit Message

Naveen N. Rao May 15, 2017, 6:05 p.m.
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(-)

Comments

Masami Hiramatsu May 17, 2017, 1:12 a.m.
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,
Michael Ellerman May 18, 2017, 5:22 a.m.
"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
Naveen N. Rao May 30, 2017, 6:14 p.m.
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
Naveen N. Rao May 30, 2017, 6:16 p.m.
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

Patch hide | download patch | download mbox

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;