Re: [PATCH 2/4] x86/kprobes: Fix frame pointer annotations

[Masami Hiramatsu]

On Fri, 10 May 2019 14:40:54 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Fri, May 10, 2019 at 01:58:31PM +0900, Masami Hiramatsu wrote:
> > On Thu, 9 May 2019 19:14:16 +0200
> > Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > > > > --- a/arch/x86/kernel/kprobes/core.c
> > > > > +++ b/arch/x86/kernel/kprobes/core.c
> > > > > @@ -731,29 +731,8 @@ asm(
> > > > >  	".global kretprobe_trampoline\n"
> > > > >  	".type kretprobe_trampoline, @function\n"
> > > > >  	"kretprobe_trampoline:\n"
> 
> > > > Here, we need a gap for storing ret-ip, because kretprobe_trampoline is
> > > > the address which is returned from the target function. We have no 
> > > > "ret-ip" here at this point. So something like
> > > > 
> > > > +	"push $0\n"	/* This is a gap, will be filled with real return address*/
> > > 
> > > The trampoline already provides a gap, trampoline_handler() will need to
> > > use int3_emulate_push() if it wants to inject something on the return
> > > stack.
> > 
> > I guess you mean the int3 case. This trampoline is used as a return destination.
> 
> > When the target function is called, kretprobe interrupts the first instruction,
> > and replace the return address with this trampoline. When a "ret" instruction
> > is done, it returns to this trampoline. Thus the stack frame start with
> > previous context here. As you described above,
> 
> I would prefer to change that to inject an extra return address, instead
> of replacing it. With the new exception stuff we can actually do that.
> 
> So on entry we then go from:
> 
> 	<previous context>
> 	RET-IP
> 
> to
> 
> 	<previous context>
> 	RET-IP
> 	return-trampoline
> 
> So when the function returns, it falls into the trampoline instead.

Is that really possible? On x86-64, most parameters are passed by registers,
but x86-32 (and x86-64 in rare case) some parameters can be passed by stack.
If we change the stack layout in the function prologue, the code in
function body can not access those parameters on stack.

Thank you,

> 
> > > > > +	 * On entry the stack looks like:
> > > > > +	 *
> > > > > +	 *   2*4(%esp) <previous context>
> > > > > +	 *   1*4(%esp) RET-IP
> > > > > +	 *   0*4(%esp) func
> > 
> > From this trampoline call, the stack looks like:
> > 
> > 	 *   1*4(%esp) <previous context>
> > 	 *   0*4(%esp) func
> > 
> > So we need one more push.
> 
> And then the stack looks just right at this point.
> 
> > > > > +	"push trampoline_handler\n"
> > > > > +	"jmp call_to_exception_trampoline\n"
> > > > >  	".size kretprobe_trampoline, .-kretprobe_trampoline\n"
> > > > >  );
> > > > >  NOKPROBE_SYMBOL(kretprobe_trampoline);


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>