linux-kernel - Re: [PATCH -tip 0/5] kprobes: Fix stacktrace in kretprobes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20210311092018.2d0e54d2c891850e549d16fe@kernel.org>
Date:   Thu, 11 Mar 2021 09:20:18 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Daniel Xu <dxu@...uu.xyz>, Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...nel.org>, X86 ML <x86@...nel.org>,
        linux-kernel@...r.kernel.org, bpf@...r.kernel.org, kuba@...nel.org,
        mingo@...hat.com, ast@...nel.org, tglx@...utronix.de,
        kernel-team@...com, yhs@...com
Subject: Re: [PATCH -tip 0/5] kprobes: Fix stacktrace in kretprobes

On Wed, 10 Mar 2021 12:31:13 -0600
Josh Poimboeuf <jpoimboe@...hat.com> wrote:

> On Thu, Mar 11, 2021 at 12:55:09AM +0900, Masami Hiramatsu wrote:
> > +#ifdef CONFIG_KRETPROBES
> > +static unsigned long orc_kretprobe_correct_ip(struct unwind_state *state)
> > +{
> > +	return kretprobe_find_ret_addr(
> > +			(unsigned long)kretprobe_trampoline_addr(),
> > +			state->task, &state->kr_iter);
> > +}
> > +
> > +static bool is_kretprobe_trampoline_address(unsigned long ip)
> > +{
> > +	return ip == (unsigned long)kretprobe_trampoline_addr();
> > +}
> > +#else
> > +static unsigned long orc_kretprobe_correct_ip(struct unwind_state *state)
> > +{
> > +	return state->ip;
> > +}
> > +
> > +static bool is_kretprobe_trampoline_address(unsigned long ip)
> > +{
> > +	return false;
> > +}
> > +#endif
> > +
> 
> Can this code go in a kprobes file?  I'd rather not clutter ORC with it,
> and maybe it would be useful for other arches or unwinders.

Yes, anyway dummy kretprobe_find_ret_addr() and kretprobe_trampoline_addr()
should be defined !CONFIG_KRETPROBES case.

> 
> >  bool unwind_next_frame(struct unwind_state *state)
> >  {
> >  	unsigned long ip_p, sp, tmp, orig_ip = state->ip, prev_sp = state->sp;
> > @@ -536,6 +561,18 @@ bool unwind_next_frame(struct unwind_state *state)
> >  
> >  		state->ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
> >  						  state->ip, (void *)ip_p);
> > +		/*
> > +		 * There are special cases when the stack unwinder is called
> > +		 * from the kretprobe handler or the interrupt handler which
> > +		 * occurs in the kretprobe trampoline code. In those cases,
> > +		 * %sp is shown on the stack instead of the return address.
> > +		 * Or, when the unwinder find the return address is replaced
> > +		 * by kretprobe_trampoline.
> > +		 * In those cases, correct address can be found in kretprobe.
> > +		 */
> > +		if (state->ip == sp ||
> 
> Why is the 'state->ip == sp' needed?

As I commented above, until kretprobe_trampoline writes back the real
address to the stack, sp value is there (which has been pushed by the
'pushq %rsp' at the entry of kretprobe_trampoline.)

        ".type kretprobe_trampoline, @function\n"
        "kretprobe_trampoline:\n"
        /* We don't bother saving the ss register */
        "       pushq %rsp\n"				// THIS
        "       pushfq\n"

Thus, from inside the kretprobe handler, like ftrace, you'll see
the sp value instead of the real return address.

> > +		    is_kretprobe_trampoline_address(state->ip))
> > +			state->ip = orc_kretprobe_correct_ip(state);
> 
> This is similar in concept to ftrace_graph_ret_addr(), right?  Would it
> be possible to have a similar API?  Like
> 
> 		state->ip = kretprobe_ret_addr(state->task, &state->kr_iter, state->ip);

OK, but,

> and without the conditional.

As I said, it is not possible because "state->ip == sp" check depends on
ORC unwinder.

> >  		state->sp = sp;
> >  		state->regs = NULL;
> > @@ -649,6 +686,12 @@ void __unwind_start(struct unwind_state *state, struct task_struct *task,
> >  		state->full_regs = true;
> >  		state->signal = true;
> >  
> > +		/*
> > +		 * When the unwinder called with regs from kretprobe handler,
> > +		 * the regs->ip starts from kretprobe_trampoline address.
> > +		 */
> > +		if (is_kretprobe_trampoline_address(state->ip))
> > +			state->ip = orc_kretprobe_correct_ip(state);
> 
> Shouldn't __kretprobe_trampoline_handler() just set regs->ip to
> 'correct_ret_addr' before passing the regs to the handler?  I'd think
> that would be a less surprising value for regs->ip than
> '&kretprobe_trampoline'.

Hmm, actually current implementation on x86 mimics the behevior of
the int3 exception (which many architectures still do).

Previously the kretprobe_trampoline is a place holder like this.

        "kretprobe_trampoline:\n"
        "       nop\n"

And arch_init_kprobes() puts a kprobe (int3) there.
So in that case regs->ip should be kretprobe_trampoline.
User handler (usually architecutre independent) finds the
correct_ret_addr in kretprobe_instance.ret_addr field.

> And it would make the unwinder just work automatically when unwinding
> from the handler using the regs.
> 
> It would also work when unwinding from the handler's stack, if we put an
> UNWIND_HINT_REGS after saving the regs.

At that moment, the real return address is not identified. So we can not
put it.

> 
> The only (rare) case it wouldn't work would be unwinding from an
> interrupt before regs->ip gets set properly.  In which case we'd still
> need the above call to orc_kretprobe_correct_ip() or so.


Thank you,

-- 
Masami Hiramatsu <mhiramat@...nel.org>