linux-kernel - Re: [PATCH 02/16] x86/dumpstack: Add get_stack

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:   Mon, 20 Nov 2017 13:07:16 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Andy Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>,
        Borislav Petkov <bpetkov@...e.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Brian Gerst <brgerst@...il.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 02/16] x86/dumpstack: Add get_stack_info() support for the
 SYSENTER stack

On Mon, Nov 20, 2017 at 1:00 PM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> On Mon, Nov 20, 2017 at 12:46:13PM -0800, Andy Lutomirski wrote:
>> On Mon, Nov 20, 2017 at 12:42 PM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
>> > On Mon, Nov 20, 2017 at 09:07:33AM -0800, Andy Lutomirski wrote:
>> >> +bool in_SYSENTER_stack(unsigned long *stack, struct stack_info *info)
>> >
>> > Can you make it lowercase for consistency with the other in_*_stack()
>> > functions?  For example, in_irq_stack() is all lowercase even though
>> > "IRQ" is normally written in uppercase.
>> >
>> > But also, I'm wondering whether this get_stack_info() support is even
>> > really needed.
>> >
>> > As currently written, the trampoline code doesn't have any ORC data
>> > associated with it.  So the unwinder would never have the need to
>> > actually read the SYSENTER stack.
>> >
>> > You _could_ add an UNWIND_HINT_IRET_REGS annotation after the simulated
>> > iret frame is written, which would allow the unwinder to dump those regs
>> > when unwinding from an NMI.
>>
>> There's some ORC data in the non-trampoline  SYSENTER path
>
> But that's *after* the stack switch to the real kernel stack, right?

Hmm, maybe you're right.

>
>> but, more importantly, the OOPS unwinder will just bail without this
>> patch.  With the patch, we get a valid unwind, except that everything
>> has a ?  in front.
>
> Hm.  I can't even fathom how that's possible.  Are you talking about the
> "unwind from NMI to SYSENTER stack" path?  Or any unwind to a syscall?
> Either way I'm baffled...  If the unwinder only encounters the SYSENTER
> stack at the end, how could that cause everything beforehand to have a
> question mark?

I mean that, if I put a ud2 or other bug in the code that runs on the
SYSENTER stack, without this patch, I get a totally blank call trace.

>
>> > But there's only a tiny window where that would be possible: only a few
>> > instructions.  I'm not sure that would be worth the effort, unless we
>> > got to the point where we expect to have 100% unwinder coverage.  But
>> > that's currently unrealistic anyway because of generated code and
>> > runtime patching.
>>
>> I tripped it myself several times when debugging this code.
>
> Again I don't see how this patch would help if there's no ORC data for
> the code which uses the SYSENTER stack.  I must be missing something.