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

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

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