linux-kernel - Re: [PATCH v2 3/7] x86/entry/64: Simplify idtentry a little

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:   Thu, 4 Jul 2019 14:54:37 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andrew Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Juergen Gross <jgross@...e.com>,
        LKML <linux-kernel@...r.kernel.org>,
        He Zhe <zhe.he@...driver.com>,
        Joel Fernandes <joel@...lfernandes.org>, devel@...ukata.com
Subject: Re: [PATCH v2 3/7] x86/entry/64: Simplify idtentry a little

On Thu, Jul 4, 2019 at 1:03 PM Peter Zijlstra <peterz@...radead.org> wrote:
>
> There's a bunch of duplication in idtentry, namely the
> .Lfrom_usermode_switch_stack is a paranoid=0 copy of the normal flow.
>
> Make this explicit by creating a idtentry_part helper macro.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  arch/x86/entry/entry_64.S |  100 +++++++++++++++++++++-------------------------
>  1 file changed, 47 insertions(+), 53 deletions(-)
>
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -865,6 +865,51 @@ apicinterrupt IRQ_WORK_VECTOR                      irq_work
>   */
>  #define CPU_TSS_IST(x) PER_CPU_VAR(cpu_tss_rw) + (TSS_ist + (x) * 8)
>
> +.macro idtentry_part do_sym, has_error_code:req, paranoid:req, shift_ist=-1, ist_offset=0
> +
> +       .if \paranoid
> +       call    paranoid_entry
> +       /* returned flag: ebx=0: need swapgs on exit, ebx=1: don't need it */
> +       .else
> +       call    error_entry
> +       .endif
> +       UNWIND_HINT_REGS
> +
> +       .if \paranoid
> +       .if \shift_ist != -1
> +       TRACE_IRQS_OFF_DEBUG                    /* reload IDT in case of recursion */
> +       .else
> +       TRACE_IRQS_OFF
> +       .endif
> +       .endif
> +
> +       movq    %rsp, %rdi                      /* pt_regs pointer */
> +
> +       .if \has_error_code
> +       movq    ORIG_RAX(%rsp), %rsi            /* get error code */
> +       movq    $-1, ORIG_RAX(%rsp)             /* no syscall to restart */
> +       .else
> +       xorl    %esi, %esi                      /* no error code */
> +       .endif
> +
> +       .if \shift_ist != -1
> +       subq    $\ist_offset, CPU_TSS_IST(\shift_ist)
> +       .endif
> +
> +       call    \do_sym
> +
> +       .if \shift_ist != -1
> +       addq    $\ist_offset, CPU_TSS_IST(\shift_ist)
> +       .endif
> +
> +       .if \paranoid
> +       jmp     paranoid_exit
> +       .else
> +       jmp     error_exit
> +       .endif
> +
> +.endm
> +
>  /**
>   * idtentry - Generate an IDT entry stub
>   * @sym:               Name of the generated entry point
> @@ -935,46 +980,7 @@ ENTRY(\sym)
>  .Lfrom_usermode_no_gap_\@:
>         .endif
>
> -       .if \paranoid
> -       call    paranoid_entry
> -       .else
> -       call    error_entry
> -       .endif
> -       UNWIND_HINT_REGS
> -       /* returned flag: ebx=0: need swapgs on exit, ebx=1: don't need it */
> -
> -       .if \paranoid
> -       .if \shift_ist != -1
> -       TRACE_IRQS_OFF_DEBUG                    /* reload IDT in case of recursion */
> -       .else
> -       TRACE_IRQS_OFF
> -       .endif
> -       .endif
> -
> -       movq    %rsp, %rdi                      /* pt_regs pointer */
> -
> -       .if \has_error_code
> -       movq    ORIG_RAX(%rsp), %rsi            /* get error code */
> -       movq    $-1, ORIG_RAX(%rsp)             /* no syscall to restart */
> -       .else
> -       xorl    %esi, %esi                      /* no error code */
> -       .endif
> -
> -       .if \shift_ist != -1
> -       subq    $\ist_offset, CPU_TSS_IST(\shift_ist)
> -       .endif
> -
> -       call    \do_sym
> -
> -       .if \shift_ist != -1
> -       addq    $\ist_offset, CPU_TSS_IST(\shift_ist)
> -       .endif
> -
> -       .if \paranoid
> -       jmp     paranoid_exit
> -       .else
> -       jmp     error_exit
> -       .endif
> +       idtentry_part \do_sym, \has_error_code, \paranoid, \shift_ist, \ist_offset
>
>         .if \paranoid == 1
>         /*
> @@ -983,21 +989,9 @@ ENTRY(\sym)
>          * run in real process context if user_mode(regs).
>          */
>  .Lfrom_usermode_switch_stack_\@:
> -       call    error_entry
> -
> -       movq    %rsp, %rdi                      /* pt_regs pointer */
> -
> -       .if \has_error_code
> -       movq    ORIG_RAX(%rsp), %rsi            /* get error code */
> -       movq    $-1, ORIG_RAX(%rsp)             /* no syscall to restart */
> -       .else
> -       xorl    %esi, %esi                      /* no error code */
> +       idtentry_part \do_sym, \has_error_code, 0

Nice!  You are adding an extra UNWIND_HINT_REGS that wasn't here
before, but I think that's fine.  However, can you pleace make it
paranoid=0 instead of just 0?  You could go all the way verbose and
say do_sym=\do_sym, etc, but that seems like overkill.

Other than that nitpick, Acked-by: Andy Lutomirski <luto@...nel.org>

--Andy