Re: [PATCH v2 14/14] x86/shstk: Activate Supervisor Shadow Stacks

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 29/05/2020 14:09, Jan Beulich wrote:
> On 27.05.2020 21:18, Andrew Cooper wrote:
>> With all other plumbing in place, activate shadow stacks when possible.  Note
>> that this requires prohibiting the use of PV32.  Compatibility can be
>> maintained if necessary via PV-Shim.
> In the revision log you say "Discuss CET-SS disabling PV32", and I
> agree both here and in the command line doc you mention the "that"
> aspect. But what about the "why"? Aiui "is incompatible" or
> "requires" are too strong statements: It could be made work (by
> disabling / enabling CET on the way out of / back into Xen), but
> besides losing some of the intended protection that way, it would
> be quite a bit of overhead. So it's more like a design decision,
> and it would be nice to express it like this at least in the
> commit message.

For starters, the guest kernel and Xen share the single
MSR_S_CET.SHSKT_EN bit, as they are both supervisor in the eyes of the
processor.

We can't use the PV32_CR4 trick to turn CET.SS off on return to guest
kernel context, because (unlike SMAP/SMEP), the race condition with a
late NMI would manifest as #CP against the IRET, not a spurious page fault.

Furthermore, an IRET to Ring 3 and an IRET to Ring 1 now differ by three
words on the shadow stack.  An IRET to Ring 1 is a supervisor return, so
performs consistency checks on %cs/%lip/SSP on the shadow stack.  We
could in theory shuffle the shadow stack by 3 words on context switch.

It might theoretically be possible to make something which functioned
correctly with a PV guest kernel which doesn't understand a
paravirtualised version of supervisor shadow stacks, but I quickly
concluded that it isn't even worth the effort to figure for certain.

>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -664,6 +664,13 @@ static void __init noreturn reinit_bsp_stack(void)
>>      stack_base[0] = stack;
>>      memguard_guard_stack(stack);
>>  
>> +    if ( cpu_has_xen_shstk )
>> +    {
>> +        wrmsrl(MSR_PL0_SSP, (unsigned long)stack + 0x5ff8);
> Please replace this remaining literal number accordingly.
>
>> @@ -1801,6 +1823,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>  
>>      alternative_branches();
>>  
>> +    /* Defer CR4.CET until alternatives have finished playing with CR4.WP */
>> +    if ( cpu_has_xen_shstk )
>> +        set_in_cr4(X86_CR4_CET);
> Nit: The comment still wants changing to CR0.WP.
>
> With these taken care of in some form
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew