Re: [PATCH 02/16] x86/entry/32: Enter the kernel via trampoline stack

[Andy Lutomirski]

On Tue, Jan 16, 2018 at 8:36 AM, Joerg Roedel <joro@xxxxxxxxxx> wrote:
> From: Joerg Roedel <jroedel@xxxxxxx>
>
> Use the sysenter stack as a trampoline stack to enter the
> kernel. The sysenter stack is already in the cpu_entry_area
> and will be mapped to userspace when PTI is enabled.
>
> Signed-off-by: Joerg Roedel <jroedel@xxxxxxx>
> ---
>  arch/x86/entry/entry_32.S        | 89 +++++++++++++++++++++++++++++++++++-----
>  arch/x86/include/asm/switch_to.h |  6 +--
>  arch/x86/kernel/asm-offsets_32.c |  4 +-
>  arch/x86/kernel/cpu/common.c     |  5 ++-
>  arch/x86/kernel/process.c        |  2 -
>  arch/x86/kernel/process_32.c     |  6 +++
>  6 files changed, 91 insertions(+), 21 deletions(-)
>
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index eb8c5615777b..5a7bdb73be9f 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -222,6 +222,47 @@
>  .endm
>
>  /*
> + * Switch from the entry-trampline stack to the kernel stack of the
> + * running task.
> + *
> + * nr_regs is the number of dwords to push from the entry stack to the
> + * task stack. If it is > 0 it expects an irq frame at the bottom of the
> + * stack.
> + *
> + * check_user != 0 it will add a check to only switch stacks if the
> + * kernel entry was from user-space.
> + */
> +.macro SWITCH_TO_KERNEL_STACK nr_regs=0 check_user=0

How about marking nr_regs with :req to force everyone to be explicit?

> +
> +       .if \check_user > 0 && \nr_regs > 0
> +       testb $3, (\nr_regs - 4)*4(%esp)                /* CS */
> +       jz .Lend_\@
> +       .endif
> +
> +       pushl %edi
> +       movl  %esp, %edi
> +
> +       /*
> +        * TSS_sysenter_stack is the offset from the bottom of the
> +        * entry-stack
> +        */
> +       movl  TSS_sysenter_stack + ((\nr_regs + 1) * 4)(%esp), %esp

This is incomprehensible.  You're adding what appears to be the offset
of sysenter_stack within the TSS to something based on esp and
dereferencing that to get the new esp.  That't not actually what
you're doing, but please change asm_offsets.c (as in my previous
email) to avoid putting serious arithmetic in it and then do the
arithmetic right here so that it's possible to follow what's going on.

> +
> +       /* Copy the registers over */
> +       .if \nr_regs > 0
> +       i = 0
> +       .rept \nr_regs
> +       pushl (\nr_regs - i) * 4(%edi)
> +       i = i + 1
> +       .endr
> +       .endif
> +
> +       mov (%edi), %edi
> +
> +.Lend_\@:
> +.endm
> +
> +/*
>   * %eax: prev task
>   * %edx: next task
>   */
> @@ -401,7 +442,9 @@ ENTRY(xen_sysenter_target)
>   * 0(%ebp) arg6
>   */
>  ENTRY(entry_SYSENTER_32)
> -       movl    TSS_sysenter_stack(%esp), %esp
> +       /* Kernel stack is empty */
> +       SWITCH_TO_KERNEL_STACK

This would be more readable if you put nr_regs in here.

> +
>  .Lsysenter_past_esp:
>         pushl   $__USER_DS              /* pt_regs->ss */
>         pushl   %ebp                    /* pt_regs->sp (stashed in bp) */
> @@ -521,6 +564,10 @@ ENDPROC(entry_SYSENTER_32)
>  ENTRY(entry_INT80_32)
>         ASM_CLAC
>         pushl   %eax                    /* pt_regs->orig_ax */
> +
> +       /* Stack layout: ss, esp, eflags, cs, eip, orig_eax */
> +       SWITCH_TO_KERNEL_STACK nr_regs=6 check_user=1
> +

Why check_user?

> @@ -655,6 +702,10 @@ END(irq_entries_start)
>  common_interrupt:
>         ASM_CLAC
>         addl    $-0x80, (%esp)                  /* Adjust vector into the [-256, -1] range */
> +
> +       /* Stack layout: ss, esp, eflags, cs, eip, vector */
> +       SWITCH_TO_KERNEL_STACK nr_regs=6 check_user=1

LGTM.

>  ENTRY(nmi)
>         ASM_CLAC
> +
> +       /* Stack layout: ss, esp, eflags, cs, eip */
> +       SWITCH_TO_KERNEL_STACK nr_regs=5 check_user=1

This is wrong, I think.  If you get an nmi in kernel mode but while
still on the sysenter stack, you blow up.  IIRC we have some crazy
code already to handle this (for nmi and #DB), and maybe that's
already adequate or can be made adequate, but at the very least this
needs a big comment explaining why it's okay.

> diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
> index eb5f7999a893..20e5f7ab8260 100644
> --- a/arch/x86/include/asm/switch_to.h
> +++ b/arch/x86/include/asm/switch_to.h
> @@ -89,13 +89,9 @@ static inline void refresh_sysenter_cs(struct thread_struct *thread)
>  /* This is used when switching tasks or entering/exiting vm86 mode. */
>  static inline void update_sp0(struct task_struct *task)
>  {
> -       /* On x86_64, sp0 always points to the entry trampoline stack, which is constant: */
> -#ifdef CONFIG_X86_32
> -       load_sp0(task->thread.sp0);
> -#else
> +       /* sp0 always points to the entry trampoline stack, which is constant: */
>         if (static_cpu_has(X86_FEATURE_XENPV))
>                 load_sp0(task_top_of_stack(task));
> -#endif
>  }
>
>  #endif /* _ASM_X86_SWITCH_TO_H */
> diff --git a/arch/x86/kernel/asm-offsets_32.c b/arch/x86/kernel/asm-offsets_32.c
> index 654229bac2fc..7270dd834f4b 100644
> --- a/arch/x86/kernel/asm-offsets_32.c
> +++ b/arch/x86/kernel/asm-offsets_32.c
> @@ -47,9 +47,11 @@ void foo(void)
>         BLANK();
>
>         /* Offset from the sysenter stack to tss.sp0 */
> -       DEFINE(TSS_sysenter_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp0) -
> +       DEFINE(TSS_sysenter_stack, offsetof(struct cpu_entry_area, tss.x86_tss.sp1) -
>                offsetofend(struct cpu_entry_area, entry_stack_page.stack));



>
> +       OFFSET(TSS_sp1, tss_struct, x86_tss.sp1);
> +
>  #ifdef CONFIG_CC_STACKPROTECTOR
>         BLANK();
>         OFFSET(stack_canary_offset, stack_canary, canary);
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index ef29ad001991..20a71c914e59 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1649,11 +1649,12 @@ void cpu_init(void)
>         enter_lazy_tlb(&init_mm, curr);
>
>         /*
> -        * Initialize the TSS.  Don't bother initializing sp0, as the initial
> -        * task never enters user mode.
> +        * Initialize the TSS.  sp0 points to the entry trampoline stack
> +        * regardless of what task is running.
>          */
>         set_tss_desc(cpu, &get_cpu_entry_area(cpu)->tss.x86_tss);
>         load_TR_desc();
> +       load_sp0((unsigned long)(cpu_entry_stack(cpu) + 1));

It's high time we unified the 32-bit and 64-bit versions of the code.
This isn't necessarily needed for your series, though.

> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index 5224c6099184..452eeac00b80 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -292,6 +292,12 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>         this_cpu_write(cpu_current_top_of_stack,
>                        (unsigned long)task_stack_page(next_p) +
>                        THREAD_SIZE);
> +       /*
> +        * TODO: Find a way to let cpu_current_top_of_stack point to
> +        * cpu_tss_rw.x86_tss.sp1. Doing so now results in stack corruption with
> +        * iret exceptions.
> +        */
> +       this_cpu_write(cpu_tss_rw.x86_tss.sp1, next_p->thread.sp0);

Do you know what the issue is?

As a general comment, the interaction between this patch and vm86 is a
bit scary.  In vm86 mode, the kernel gets entered with extra stuff on
the stack, which may screw up all your offsets.

--Andy