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

Message ID d6b0691226623b3499d43dff1e31971ea46df9f9.1494871248.git.naveen.n.rao@linux.vnet.ibm.com
State Superseded
Headers show

Commit Message

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

Comments

Masami Hiramatsu May 17, 2017, 1:21 a.m. | #1
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
>
Naveen N. Rao May 30, 2017, 6:36 p.m. | #2
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

Patch

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));