Re: [PATCH v2 05/14] x86/shstk: Re-layout the stack block for shadow stacks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2020 21:18, Andrew Cooper wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -365,20 +365,15 @@ static void show_guest_stack(struct vcpu *v, const 
> struct cpu_user_regs *regs)
>  /*
>   * Notes for get_stack_trace_bottom() and get_stack_dump_bottom()
>   *
> - * Stack pages 0 - 3:
> + * Stack pages 1 - 4:
>   *   These are all 1-page IST stacks.  Each of these stacks have an exception
>   *   frame and saved register state at the top.  The interesting bound for a
>   *   trace is the word adjacent to this, while the bound for a dump is the
>   *   very top, including the exception frame.
>   *
> - * Stack pages 4 and 5:
> - *   None of these are particularly interesting.  With MEMORY_GUARD, page 5 
> is
> - *   explicitly not present, so attempting to dump or trace it is
> - *   counterproductive.  Without MEMORY_GUARD, it is possible for a call 
> chain
> - *   to use the entire primary stack and wander into page 5.  In this case,
> - *   consider these pages an extension of the primary stack to aid debugging
> - *   hopefully rare situations where the primary stack has effective been
> - *   overflown.
> + * Stack pages 0 and 5:
> + *   Shadow stacks.  These are mapped read-only, and used by CET-SS capable
> + *   processors.  They will never contain regular stack data.

I don't mind the comment getting put in place already here, but will it
reflect reality even when CET-SS is not in use, in that the pages then
still are mapped r/o rather than being left unmapped to act as guard
pages not only for stack pushes but also for stack pops? At which point
the "dump or trace it is counterproductive" remark would still apply in
this case, and hence may better be retained.

> @@ -392,13 +387,10 @@ unsigned long get_stack_trace_bottom(unsigned long sp)
>  {
>      switch ( get_stack_page(sp) )
>      {
> -    case 0 ... 3:
> +    case 1 ... 4:
>          return ROUNDUP(sp, PAGE_SIZE) -
>              offsetof(struct cpu_user_regs, es) - sizeof(unsigned long);
>  
> -#ifndef MEMORY_GUARD
> -    case 4 ... 5:
> -#endif
>      case 6 ... 7:
>          return ROUNDUP(sp, STACK_SIZE) -
>              sizeof(struct cpu_info) - sizeof(unsigned long);
> @@ -412,12 +404,9 @@ unsigned long get_stack_dump_bottom(unsigned long sp)
>  {
>      switch ( get_stack_page(sp) )
>      {
> -    case 0 ... 3:
> +    case 1 ... 4:
>          return ROUNDUP(sp, PAGE_SIZE) - sizeof(unsigned long);
>  
> -#ifndef MEMORY_GUARD
> -    case 4 ... 5:
> -#endif
>      case 6 ... 7:
>          return ROUNDUP(sp, STACK_SIZE) - sizeof(unsigned long);

The need to adjust these literal numbers demonstrates how fragile
this is. I admit I can't see a good way to get rid of the literal
numbers altogether, but could I talk you into switching to (for
the latter, as example)

    switch ( get_stack_page(sp) )
    {
    case 0: case PRIMARY_SHSTK_SLOT:
        return 0;

    case 1 ... 4:
        return ROUNDUP(sp, PAGE_SIZE) - sizeof(unsigned long);

    case 6 ... 7:
        return ROUNDUP(sp, STACK_SIZE) - sizeof(unsigned long);

    default:
        return sp - sizeof(unsigned long);
    }

? Of course this will need the callers to be aware they may get
back zero, but there are only very few (which made me notice the
functions would better be static). And the returning of zero may
then want changing (conditionally upon us using CET-SS) in a
later patch, where iirc you use the shadow stack for call trace
generation.

As a positive side effect this will yield a compile error if
PRIMARY_SHSTK_SLOT gets changed without adjusting these
functions.

> --- a/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -75,6 +75,9 @@
>  /* Primary stack is restricted to 8kB by guard pages. */
>  #define PRIMARY_STACK_SIZE 8192
>  
> +/* Primary shadow stack is slot 5 of 8, immediately under the primary stack. 
> */
> +#define PRIMARY_SHSTK_SLOT 5

Any reason to put it here rather than ...

> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -16,12 +16,12 @@
>   *
>   * 7 - Primary stack (with a struct cpu_info at the top)
>   * 6 - Primary stack
> - * 5 - Optionally not present (MEMORY_GUARD)
> - * 4 - Unused; optionally not present (MEMORY_GUARD)
> - * 3 - Unused; optionally not present (MEMORY_GUARD)
> - * 2 - MCE IST stack
> - * 1 - NMI IST stack
> - * 0 - Double Fault IST stack
> + * 5 - Primay Shadow Stack (read-only)
> + * 4 - #DF IST stack
> + * 3 - #DB IST stack
> + * 2 - NMI IST stack
> + * 1 - #MC IST stack
> + * 0 - IST Shadow Stacks (4x 1k, read-only)
>   */

... right below this comment?

Same question as above regarding the "read-only" here.

Jan