Re: [PATCH v2 6/8] x86/entry: add unwind hint annotations

From: Andy Lutomirski <luto@kernel.org>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>, X86 ML <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	live-patching@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Jiri Slaby <jslaby@suse.cz>, Ingo Molnar <mingo@kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Mike Galbraith <efault@gmx.de>
Subject: Re: [PATCH v2 6/8] x86/entry: add unwind hint annotations
Date: Thu, 29 Jun 2017 14:09:54 -0700	[thread overview]
Message-ID: <CALCETrUNhPgrQV9Sj+ZP3A_jSHbww4XPpBLJQ15OmH3nsHBsdg@mail.gmail.com> (raw)
In-Reply-To: <20170629190559.ttw52ahwtsjynayx@treble>

On Thu, Jun 29, 2017 at 12:05 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Jun 29, 2017 at 11:50:18AM -0700, Andy Lutomirski wrote:
>> On Thu, Jun 29, 2017 at 10:53 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > There's a bug here that will need a small change to the entry code.
>> >
>> > Mike Galbraith reported:
>> >
>> >   WARNING: can't dereference registers at ffffc900089d7e08 for ip ffffffff81740bbb
>> >
>> > After some looking I found that it's caused by the following code
>> > snippet in the 'interrupt' macro in entry_64.S:
>> >
>> >         /*
>> >          * Save previous stack pointer, optionally switch to interrupt stack.
>> >          * irq_count is used to check if a CPU is already on an interrupt stack
>> >          * or not. While this is essentially redundant with preempt_count it is
>> >          * a little cheaper to use a separate counter in the PDA (short of
>> >          * moving irq_enter into assembly, which would be too much work)
>> >          */
>> >         movq    %rsp, %rdi
>> >         incl    PER_CPU_VAR(irq_count)
>> >         cmovzq  PER_CPU_VAR(irq_stack_ptr), %rsp
>> >         UNWIND_HINT_REGS base=rdi
>> >         pushq   %rdi
>> >         UNWIND_HINT_REGS indirect=1
>> >
>> > The problem is that it's changing the stack pointer *before* writing the
>> > previous stack pointer (push %rdi).  So when unwinding from an NMI which
>> > hit between the rsp write and the rdi push, the unwinder tries to access
>> > the regs on the previous stack (by reading rdi), but the previous stack
>> > pointer isn't there yet, so the access is considered out of bounds.
>>
>> Ugh, that code.  Does this problem go away with this patch applied:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry_ist&id=2231ec7e0bcc1a2bc94a17081511ab54cc6badd1
>>
>> If so, want to update the patch for new kernels (shouldn't conflict
>> with anything except your unwind hints)?
>
> I don't think that patch will fix it, because it still updates rsp
> *before* writing the old rsp on the new stack.  So there's still a
> window where the "previous stack" pointer is missing.

But it's in a register.  Is undwarf not able to grok that?  I have no
fundamental problem with pushing it to the new stack first, but the
actual asm is nastier because we don't have an addressing mode that's
*(*(gs:blahblahblah)) = reg.

At least my patch makes all the copied of this code identical so the
problem can be solved only once.