linux-kernel - Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags

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: <CALCETrUmV7_khBps8F9V7QjvO4+WF+7hJb-GBhpvr8j=zZV8Fg@mail.gmail.com>
Date:   Fri, 1 Dec 2017 08:22:02 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>
Cc:     "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, X86 ML <x86@...nel.org>,
        Juergen Gross <jgross@...e.com>,
        Andrew Lutomirski <luto@...nel.org>
Subject: Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to
 access eflags

On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
<boris.ostrovsky@...cle.com> wrote:
> Commit 1d3e53e8624a ("x86/entry/64: Refactor IRQ stacks and make
> them NMI-safe") added DEBUG_ENTRY_ASSERT_IRQS_OFF macro that acceses
> eflags using 'pushfq' instruction when testing for IF bit. On PV Xen
> guests looking at IF flag directly will always see it set, resulting
> in 'ud2'.
>
> Introduce SAVE_FLAGS() macro that will use appropriate save_fl pv op
> when running paravirt.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@...cle.com>
> ---
> V2:
> * Preserve %rax in DEBUG_ENTRY_ASSERT_IRQS_OFF
> * Return (pop) %rax in SAVE_FLAGS for !CONFIG_PARAVIRT (irqflags.h)
>
>  arch/x86/entry/entry_64.S        |    7 ++++---
>  arch/x86/include/asm/irqflags.h  |    3 +++
>  arch/x86/include/asm/paravirt.h  |    9 +++++++++
>  arch/x86/kernel/asm-offsets_64.c |    3 +++
>  4 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index f81d50d..c208dc1 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -466,12 +466,13 @@ END(irq_entries_start)
>
>  .macro DEBUG_ENTRY_ASSERT_IRQS_OFF
>  #ifdef CONFIG_DEBUG_ENTRY
> -       pushfq
> -       testl $X86_EFLAGS_IF, (%rsp)
> +       pushq %rax
> +       SAVE_FLAGS(CLBR_ANY)
> +       testl $X86_EFLAGS_IF, %eax

Confused.  You're both using CLBR_ANY and RAX.  Did you perhaps mean CLBR_NONE?

>         jz .Lokay_\@
>         ud2
>  .Lokay_\@:
> -       addq $8, %rsp
> +       popq %rax
>  #endif
>  .endm
>
> diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
> index c8ef23f..89f0895 100644
> --- a/arch/x86/include/asm/irqflags.h
> +++ b/arch/x86/include/asm/irqflags.h
> @@ -142,6 +142,9 @@ static inline notrace unsigned long arch_local_irq_save(void)
>         swapgs;                                 \
>         sysretl
>
> +#ifdef CONFIG_DEBUG_ENTRY
> +#define SAVE_FLAGS(x)          pushfq; popq %rax
> +#endif
>  #else
>  #define INTERRUPT_RETURN               iret
>  #define ENABLE_INTERRUPTS_SYSEXIT      sti; sysexit
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index 283efca..892df37 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -927,6 +927,15 @@ static inline notrace unsigned long arch_local_irq_save(void)
>         PARA_SITE(PARA_PATCH(pv_cpu_ops, PV_CPU_usergs_sysret64),       \
>                   CLBR_NONE,                                            \
>                   jmp PARA_INDIRECT(pv_cpu_ops+PV_CPU_usergs_sysret64))
> +
> +#ifdef CONFIG_DEBUG_ENTRY
> +#define SAVE_FLAGS(clobbers)                                        \
> +       PARA_SITE(PARA_PATCH(pv_irq_ops, PV_IRQ_save_fl), clobbers, \
> +                 PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);        \
> +                 call PARA_INDIRECT(pv_irq_ops+PV_IRQ_save_fl);    \
> +                 PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE);)
> +#endif
> +
>  #endif /* CONFIG_X86_32 */
>
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/x86/kernel/asm-offsets_64.c b/arch/x86/kernel/asm-offsets_64.c
> index 630212f..e3a5175 100644
> --- a/arch/x86/kernel/asm-offsets_64.c
> +++ b/arch/x86/kernel/asm-offsets_64.c
> @@ -23,6 +23,9 @@ int main(void)
>  #ifdef CONFIG_PARAVIRT
>         OFFSET(PV_CPU_usergs_sysret64, pv_cpu_ops, usergs_sysret64);
>         OFFSET(PV_CPU_swapgs, pv_cpu_ops, swapgs);
> +#ifdef CONFIG_DEBUG_ENTRY
> +       OFFSET(PV_IRQ_save_fl, pv_irq_ops, save_fl);
> +#endif
>         BLANK();
>  #endif
>
> --
> 1.7.1
>