linux-kernel - Re: [Xen-devel] [PATCH v1] xen: get rid of paravirt op adjust_exception

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: <53f936d4-c6e0-3e9b-4291-8f16cbb87779@citrix.com>
Date:   Wed, 26 Jul 2017 15:31:27 +0100
From:   Andrew Cooper <andrew.cooper3@...rix.com>
To:     Andy Lutomirski <luto@...capital.net>
CC:     Juergen Gross <jgross@...e.com>, X86 ML <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [Xen-devel] [PATCH v1] xen: get rid of paravirt op
 adjust_exception_frame

On 26/07/17 15:09, Andy Lutomirski wrote:
> On Wed, Jul 26, 2017 at 7:01 AM, Andrew Cooper
> <andrew.cooper3@...rix.com> wrote:
>> On 26/07/17 14:48, Andy Lutomirski wrote:
>>>>  /* Runs on exception stack */
>>>> -ENTRY(nmi)
>>>> -       /*
>>>> -        * Fix up the exception frame if we're on Xen.
>>>> -        * PARAVIRT_ADJUST_EXCEPTION_FRAME is guaranteed to push at most
>>>> -        * one value to the stack on native, so it may clobber the rdx
>>>> -        * scratch slot, but it won't clobber any of the important
>>>> -        * slots past it.
>>>> -        *
>>>> -        * Xen is a different story, because the Xen frame itself overlaps
>>>> -        * the "NMI executing" variable.
>>>> -        */
>>> I would keep this comment.  The Xen frame really is in the way AFAICT.
>> (For reasons best explained by the original authors) there is only ever
>> a single stack which a PV guest registers with Xen, which functions
>> equivalently to tss.sp0.  There is no support for stack switching via
>> task switch or IST.
>>
>> Therefore, nested NMIs won't clobber the top of this stack.
>>
> Does that mean that nested NMIs on Xen just nest normally without
> clobbering each other?

Yes.

> If so, that's neat, although I wonder how we
> don't get crashes due to this:
>
> /* Normal 64-bit system call target */
> ENTRY(xen_syscall_target)
>         undo_xen_syscall
>         <-- NMI right here
>         jmp entry_SYSCALL_64_after_swapgs
> ENDPROC(xen_syscall_target)

I'm going to go out on a limb here and say "because no has hit that
condition yet" (or at least managed to diagnose such a crash).

PV domU's don't get given NMIs.  PV dom0 might, depending on how Xen is
handling the NMI itself.  On XenServer at least, dom0 never gets handed
an NMI.

I expect is a sufficiently rarely used path that noone has noticed if it
is indeed broken.

~Andrew

>
> I think it would be nice if Xen could instead enter the native syscall
> path a bit later like this:
>
> ENTRY(entry_SYSCALL_64)
>         /*
>          * Interrupts are off on entry.
>          * We do not frame this tiny irq-off block with TRACE_IRQS_OFF/ON,
>          * it is too small to ever cause noticeable irq latency.
>          */
>         SWAPGS_UNSAFE_STACK
>         /*
>          * A hypervisor implementation might want to use a label
>          * after the swapgs, so that it can do the swapgs
>          * for the guest and jump here on syscall.
>          */
> GLOBAL(entry_SYSCALL_64_after_swapgs)
>
>         movq    %rsp, PER_CPU_VAR(rsp_scratch)
>         movq    PER_CPU_VAR(cpu_current_top_of_stack), %rsp
>
>         TRACE_IRQS_OFF
>
>         /* Construct struct pt_regs on stack */
>         pushq   $__USER_DS                      /* pt_regs->ss */
>         pushq   PER_CPU_VAR(rsp_scratch)        /* pt_regs->sp */
>         pushq   %r11                            /* pt_regs->flags */
>         pushq   $__USER_CS                      /* pt_regs->cs */
>         pushq   %rcx                            /* pt_regs->ip */
>
> <-- Xen enters here
>
> then we wouldn't have all this funny business.  And Xen could
> completely skip the nmi nesting code.