Re: [PATCH 10/16] x86/cpu: Adjust reset_stack_and_jump() to be shadow stack compatible

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/05/2020 14:17, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
> unless you have verified the sender and know the content is safe.
>
> On 02.05.2020 00:58, Andrew Cooper wrote:
>> We need to unwind up to the supervisor token.  See the comment for details.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> ---
>>  xen/include/asm-x86/current.h | 42 
>> +++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
>> index 99b66a0087..2a7b728b1e 100644
>> --- a/xen/include/asm-x86/current.h
>> +++ b/xen/include/asm-x86/current.h
>> @@ -124,13 +124,49 @@ unsigned long get_stack_dump_bottom (unsigned long sp);
>>  # define CHECK_FOR_LIVEPATCH_WORK ""
>>  #endif
>>  
>> +#ifdef CONFIG_XEN_SHSTK
>> +/*
>> + * We need to unwind the primary shadow stack to its supervisor token, 
>> located
>> + * at 0x5ff8 from the base of the stack blocks.
>> + *
>> + * Read the shadow stack pointer, subtract it from 0x5ff8, divide by 8 to 
>> get
>> + * the number of slots needing popping.
>> + *
>> + * INCSSPQ can't pop more than 255 entries.  We shouldn't ever need to pop
>> + * that many entries, and getting this wrong will cause us to #DF later.
>> + */
>> +# define SHADOW_STACK_WORK                      \
>> +    "mov $1, %[ssp];"                           \
>> +    "rdsspd %[ssp];"                            \
>> +    "cmp $1, %[ssp];"                           \
>> +    "je 1f;" /* CET not active?  Skip. */       \
>> +    "mov $"STR(0x5ff8)", %[val];"               \
> As per comments on earlier patches, I think it would be nice if
> this wasn't a literal number here, but tied to actual stack
> layout via some suitable expression. An option might be to use
> 0xff8 (or the constant to be introduced for it in the earlier
> patch) here and ...
>
>> +    "and $"STR(STACK_SIZE - 1)", %[ssp];"       \
> ... PAGE_SIZE here.

It is important to use STACK_SIZE here and not PAGE_SIZE to trigger...

>> +    "sub %[ssp], %[val];"                       \
>> +    "shr $3, %[val];"                           \
>> +    "cmp $255, %[val];"                         \
>> +    "jle 2f;"                                   \

... this condition if we try to reset&jump from more than 4k away from
0x5ff8, e.g. from a IST stack.

Whatever happens we're going to crash, but given that we're talking
about imm32's here,

> Perhaps better "jbe", treating the unsigned values as such?

What I really want is actually to opencode an UNLIKLEY() region seeing
none of our infrastructure works inside inline asm.  Same for...

>
>> +    "ud2a;"                                     \

... this to turn into a real BUG.

~Andrew