Re: [PATCH 08/16] x86/shstk: Create shadow stacks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2020 00:58, Andrew Cooper wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -748,6 +748,25 @@ void load_system_tables(void)
>               .bitmap = IOBMP_INVALID_OFFSET,
>       };
>  
> +     /* Set up the shadow stack IST. */
> +     if ( cpu_has_xen_shstk ) {

This being a Linux style function, you want to omit the blanks
immediately inside the parentheses bother here and in the for()
below.

> +             unsigned int i;
> +             uint64_t *ist_ssp = this_cpu(tss_page).ist_ssp;
> +
> +             /* Must point at the supervisor stack token. */
> +             ist_ssp[IST_MCE] = stack_top + (IST_MCE * 0x400) - 8;
> +             ist_ssp[IST_NMI] = stack_top + (IST_NMI * 0x400) - 8;
> +             ist_ssp[IST_DB]  = stack_top + (IST_DB  * 0x400) - 8;
> +             ist_ssp[IST_DF]  = stack_top + (IST_DF  * 0x400) - 8;

Introduce a constant for 0x400, to then also be used in the
invocations of write_sss_token()?

> +             /* Poision unused entries. */
> +             for ( i = IST_MAX;
> +                   i < ARRAY_SIZE(this_cpu(tss_page).ist_ssp); ++i )
> +                     ist_ssp[i] = 0x8600111111111111ul;

IST_MAX == IST_DF, so you're overwriting one token here.

> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -434,7 +434,8 @@ struct __packed tss64 {
>      uint16_t :16, bitmap;
>  };
>  struct tss_page {
> -    struct tss64 __aligned(PAGE_SIZE) tss;
> +    uint64_t __aligned(PAGE_SIZE) ist_ssp[8];
> +    struct tss64 tss;
>  };

Just curious - any particular reason you put this ahead of the TSS?

Jan