We need to unwind up to the supervisor token.  See the comment for details.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 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];"               \
+    "and $"STR(STACK_SIZE - 1)", %[ssp];"       \
+    "sub %[ssp], %[val];"                       \
+    "shr $3, %[val];"                           \
+    "cmp $255, %[val];"                         \
+    "jle 2f;"                                   \
+    "ud2a;"                                     \
+    "2: incsspq %q[val];"                       \
+    "1:"
+#else
+# define SHADOW_STACK_WORK ""
+#endif
+
 #define switch_stack_and_jump(fn, instr)                                \
     ({                                                                  \
+        unsigned int tmp;                                               \
         __asm__ __volatile__ (                                          \
-            "mov %0,%%"__OP"sp;"                                        \
+            "cmc;"                                                      \
+            SHADOW_STACK_WORK                                           \
+            "mov %[stk], %%rsp;"                                        \
             instr                                                       \
-             "jmp %c1"                                                  \
-            : : "r" (guest_cpu_user_regs()), "i" (fn) : "memory" );     \
+            "jmp %c[fun];"                                              \
+            : [val] "=&r" (tmp),                                        \
+              [ssp] "=&r" (tmp)                                         \
+            : [stk] "r" (guest_cpu_user_regs()),                        \
+              [fun] "i" (fn)                                            \
+            : "memory" );                                               \
         unreachable();                                                  \
     })
 
-- 
2.11.0

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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  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.

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

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

> +    "ud2a;"                                     \
> +    "2: incsspq %q[val];"                       \
> +    "1:"
> +#else
> +# define SHADOW_STACK_WORK ""
> +#endif
> +
>  #define switch_stack_and_jump(fn, instr)                                \
>      ({                                                                  \
> +        unsigned int tmp;                                               \
>          __asm__ __volatile__ (                                          \
> -            "mov %0,%%"__OP"sp;"                                        \
> +            "cmc;"                                                      \
> +            SHADOW_STACK_WORK                                           \
> +            "mov %[stk], %%rsp;"                                        \
>              instr                                                       \
> -             "jmp %c1"                                                  \
> -            : : "r" (guest_cpu_user_regs()), "i" (fn) : "memory" );     \
> +            "jmp %c[fun];"                                              \
> +            : [val] "=&r" (tmp),                                        \
> +              [ssp] "=&r" (tmp)                                         \

See my concern on the earlier similar construct.

Jan

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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  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