Re: [PATCH 15/16] x86/entry: Adjust guest paths to be shadow stack compatible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2020 00:58, Andrew Cooper wrote:
> The SYSCALL/SYSEXIT paths need to use {SET,CLR}SSBSY.

I take it you mean SYSRET, not SYSEXIT. I do think though that you
also need to deal with the SYSENTER entry point we have.

> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -198,7 +198,7 @@ ENTRY(cr4_pv32_restore)
>  
>  /* See lstar_enter for entry register state. */
>  ENTRY(cstar_enter)
> -        /* sti could live here when we don't switch page tables below. */
> +        ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK

I don't see why you delete the comment here (or elsewhere). While
I recall you not really wanting them there, I still think they're
useful to have, and they shouldn't be deleted as a side effect of
an entirely unrelated change. Of course they need to live after
your insertions then.

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -194,6 +194,15 @@ restore_all_guest:
>          movq  8(%rsp),%rcx            # RIP
>          ja    iret_exit_to_guest
>  
> +        /* Clear the supervisor shadow stack token busy bit. */
> +.macro rag_clrssbsy
> +        push %rax
> +        rdsspq %rax
> +        clrssbsy (%rax)
> +        pop %rax
> +.endm
> +        ALTERNATIVE "", rag_clrssbsy, X86_FEATURE_XEN_SHSTK

In principle you could get away without spilling %rax:

        cmpl  $1,%ecx
        ja    iret_exit_to_guest

        /* Clear the supervisor shadow stack token busy bit. */
.macro rag_clrssbsy
        rdsspq %rcx
        clrssbsy (%rcx)
.endm
        ALTERNATIVE "", rag_clrssbsy, X86_FEATURE_XEN_SHSTK
        movq  8(%rsp),%rcx            # RIP
        cmpw  $FLAT_USER_CS32,16(%rsp)# CS
        movq  32(%rsp),%rsp           # RSP
        je    1f
        sysretq
1:      sysretl

        ALIGN
/* No special register assumptions. */
iret_exit_to_guest:
        movq  8(%rsp),%rcx            # RIP
        andl  $~(X86_EFLAGS_IOPL|X86_EFLAGS_NT|X86_EFLAGS_VM),24(%rsp)
        ...

Also - what about CLRSSBSY failing? It would seem easier to diagnose
this right here than when getting presumably #DF upon next entry into
Xen. At the very least I think it deserves a comment if an error case
does not get handled.

Somewhat similar for SETSSBSY, except there things get complicated by
it raising #CP instead of setting EFLAGS.CF: Aiui it would require us
to handle #CP on an IST stack in order to avoid #DF there.

> @@ -877,6 +886,14 @@ handle_ist_exception:
>          movl  $UREGS_kernel_sizeof/8,%ecx
>          movq  %rdi,%rsp
>          rep   movsq
> +
> +        /* Switch Shadow Stacks */
> +.macro ist_switch_shstk
> +        rdsspq %rdi
> +        clrssbsy (%rdi)
> +        setssbsy
> +.endm

Could you extend the comment to mention the caveat that you point
out in the description?

Jan