Re: [PATCH v2 12/14] x86/entry: Adjust guest paths to be shadow stack compatible

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/05/2020 13:40, Jan Beulich wrote:
> On 27.05.2020 21:18, Andrew Cooper wrote:
>> The SYSCALL/SYSENTER/SYSRET paths need to use {SET,CLR}SSBSY.  The IRET to
>> guest paths must not.  In the SYSRET path, re-position the mov which loads 
>> rip
>> into %rcx so we can use %rcx for CLRSSBSY, rather than spilling another
>> register to the stack.
>>
>> While we can in principle detect shadow stack corruption and a failure to
>> clear the supervisor token busy bit in the SYSRET path (by inspecting the
>> carry flag following CLRSSBSY), we cannot detect similar problems for the 
>> IRET
>> path (IRET is specified not to fault in this case).
>>
>> We will double fault at some point later, when next trying to enter Xen, due
>> to an already-set supervisor shadow stack busy bit.  As SYSRET is a uncommon
>> path anyway, avoid the added complexity for no appreciable gain.
> I'm okay with the avoidance of complexity, but why is the SYSRET path
> uncommon? Almost all hypercall returns ought to take that path?

But hypercalls returns aren't the majority of returns to guest context.

IRET from Xen IPIs, or from event channel injections hitting guest
userspace, are the most common in a non-idle system.

>
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -191,9 +191,16 @@ restore_all_guest:
>>          sarq  $47,%rcx
>>          incl  %ecx
>>          cmpl  $1,%ecx
>> -        movq  8(%rsp),%rcx            # RIP
>>          ja    iret_exit_to_guest
> This removal from the shared part of the exit path needs to be
> reflected on both of the sides of the JA, i.e. ...
>
>>  
>> +        /* 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
> ... not just here, but also like this (with the JA above changed
> to target the new label):
>
>          ALIGN
>  /* No special register assumptions. */
> +.Liret_exit_to_guest:
> +        movq  8(%rsp),%rcx            # RIP
>  iret_exit_to_guest:
>          andl  $~(X86_EFLAGS_IOPL|X86_EFLAGS_NT|X86_EFLAGS_VM),24(%rsp)
>          orl   $X86_EFLAGS_IF,24(%rsp)
>
> Granted it's mostly cosmetic, as the IRETQ ought to fault, but
> it's still a use of IRET in place of SYSRET, and hence we better
> get guest register state right. With this or a functionally
> identical adjustment (or a clarification on what makes you
> convinced this adjustment isn't needed)
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Ah yes.  I really ought to retroactively create an XSA-7 PoC for this.

Will fix.

~Andrew