Re: [PATCH v4 1/4] x86: record SSP at non-guest entry points

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/07/2026 11:26 am, Jan Beulich wrote:
> --- a/xen/arch/x86/include/asm/asm_defns.h
> +++ b/xen/arch/x86/include/asm/asm_defns.h
> @@ -220,9 +220,11 @@ static always_inline void stac(void)
>  
>  #ifdef __ASSEMBLER__
>  /*
> - * Push and clear GPRs
> + * Push and clear GPRs.
> + *
> + * Use sites may override ssp to 0. It should never be overridden to 1.
>   */
> -.macro PUSH_AND_CLEAR_GPRS
> +.macro PUSH_AND_CLEAR_GPRS ssp=IS_ENABLED(CONFIG_XEN_SHSTK)
>          push  %rdi
>          xor   %edi, %edi
>          push  %rsi
> @@ -233,6 +235,9 @@ static always_inline void stac(void)
>          xor   %ecx, %ecx
>          push  %rax
>          xor   %eax, %eax
> + .if \ssp
> +        rdsspq %rcx
> + .endif
>          push  %r8
>          xor   %r8d, %r8d
>          push  %r9
> @@ -259,12 +264,18 @@ static always_inline void stac(void)
>          xor   %r14d, %r14d
>          push  %r15
>          xor   %r15d, %r15d
> +#ifdef CONFIG_XEN_SHSTK
> +        push  %rcx
> +#endif
>  .endm

I don't like the ssp= parameter here.  It's invasive at may callsites,
doesn't 0 the value in all Ring3 frames anyway, and the asymmetry makes
the construct fragile.  Furthermore, the manual register scheduling
really isn't necessary on CPUs where it's not a NOP.

I suggest having this:

+#ifdef CONFIG_XEN_SHSTK
+        rdsspq %rcx
+        push  %rcx
+#endif

always filling in the value.  rdsspq is pretty close to free (frankly, I
would be surprised if it wasn't subject to mov elimination in the
pipeline, given how tightly coupled SSP needs to be to RSP for CALL/RET
uops to work).

>  
>  /*
>   * POP GPRs from a UREGS_* frame on the stack.  Does not modify flags.
>   */
>  .macro POP_GPRS skip_rax=0
> +#ifdef CONFIG_XEN_SHSTK
> +        pop   %rcx
> +#endif
>          pop   %r15
>          pop   %r14
>          pop   %r13
> --- a/xen/arch/x86/include/asm/cpu-user-regs.h
> +++ b/xen/arch/x86/include/asm/cpu-user-regs.h
> @@ -11,6 +11,15 @@
>   */
>  struct cpu_user_regs
>  {
> +#ifdef CONFIG_XEN_SHSTK
> +    /*
> +     * This points _at_ the corresponding shadow stack frame; it is _not_ the
> +     * outer context's SSP.  That, if the outer context has CET-SS enabled,
> +     * is stored in the top slot of the pointed to shadow stack.
> +     */
> +    uint64_t entry_ssp;
> +#endif

This needs to say "only valid for interrupt Ring0" or so.  Even as
written using the ssp= parameter, there will be a non-zero value here on
some Ring3 entries.

~Andrew