linux-kernel - Re: [RFC PATCH 10/10] x86/unwind: add undwarf unwinder

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]

Date:   Wed, 14 Jun 2017 08:44:40 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Jiri Slaby <jslaby@...e.cz>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        live-patching@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [RFC PATCH 10/10] x86/unwind: add undwarf unwinder

On Wed, Jun 14, 2017 at 01:45:41PM +0200, Jiri Slaby wrote:
> On 06/01/2017, 07:44 AM, Josh Poimboeuf wrote:
> > --- /dev/null
> > +++ b/arch/x86/kernel/unwind_undwarf.c
> > @@ -0,0 +1,402 @@
> ...
> > +void __unwind_start(struct unwind_state *state, struct task_struct *task,
> > +		    struct pt_regs *regs, unsigned long *first_frame)
> > +{
> > +	memset(state, 0, sizeof(*state));
> > +	state->task = task;
> > +
> > +	if (regs) {
> > +		if (user_mode(regs)) {
> > +			state->stack_info.type = STACK_TYPE_UNKNOWN;
> > +			return;
> > +		}
> > +
> > +		state->ip = regs->ip;
> > +		state->sp = kernel_stack_pointer(regs);
> > +		state->bp = regs->bp;
> > +		state->regs = regs;
> > +
> > +	} else if (task == current) {
> > +		register void *__sp asm(_ASM_SP);
> > +
> > +		asm volatile("lea (%%rip), %0\n\t"
> > +			     "mov %%rsp, %1\n\t"
> > +			     "mov %%rbp, %2\n\t"
> > +			     : "=r" (state->ip), "=r" (state->sp),
> > +			       "=r" (state->bp), "+r" (__sp));
> 
> Maybe I don't understand this completely, but what is __sp used for here?

This tells gcc "if this function saves the frame pointer, make sure it's
saved before inserting this inline asm."

But on second thought, it shouldn't be needed.  Either way it can use
the undwarf data to find the previous bp.  I'm struggling to remember
why I thought this was needed in the first place...

> > +		state->regs = NULL;
> > +
> > +	} else {
> 
> In DWARF unwinder, we also used to do here:
> 
> +#ifdef CONFIG_SMP
> +       } else if (task->on_cpu) {
> +               return;
> +#endif
>         } else {
> 
> > +		struct inactive_task_frame *frame = (void *)task->thread.sp;
> 
> Since there is no inactive_task_frame for tasks currently running (on
> other CPUs). At least this always held in the past.
> 
> Though, the test is indeed racy.

Yeah, it's indeed racy, but it's probably a good idea to add the check
anyway.  There are other checks to prevent going off the rails, but we
should try to detect it early when we can.  The frame pointer unwinder
could probably use a similar check.

-- 
Josh