Re: [PATCH v2 10/14] x86/extable: Adjust extable handling to be shadow stack compatible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2020 21:18, Andrew Cooper wrote:
> @@ -400,6 +400,18 @@ unsigned long get_stack_trace_bottom(unsigned long sp)
>      }
>  }
>  
> +static unsigned long get_shstk_bottom(unsigned long sp)
> +{
> +    switch ( get_stack_page(sp) )
> +    {
> +#ifdef CONFIG_XEN_SHSTK
> +    case 0:  return ROUNDUP(sp, IST_SHSTK_SIZE) - sizeof(unsigned long);
> +    case 5:  return ROUNDUP(sp, PAGE_SIZE)      - sizeof(unsigned long);

PRIMARY_SHSTK_SLOT please and, if introduced as suggested for the
earlier patch, IST_SHSTK_SLOT in the earlier line.

> @@ -763,6 +775,56 @@ static void do_reserved_trap(struct cpu_user_regs *regs)
>            trapnr, vec_name(trapnr), regs->error_code);
>  }
>  
> +static void extable_shstk_fixup(struct cpu_user_regs *regs, unsigned long 
> fixup)
> +{
> +    unsigned long ssp, *ptr, *base;
> +
> +    asm ( "rdsspq %0" : "=r" (ssp) : "0" (1) );
> +    if ( ssp == 1 )
> +        return;
> +
> +    ptr = _p(ssp);
> +    base = _p(get_shstk_bottom(ssp));
> +
> +    for ( ; ptr < base; ++ptr )
> +    {
> +        /*
> +         * Search for %rip.  The shstk currently looks like this:
> +         *
> +         *   ...  [Likely pointed to by SSP]
> +         *   %cs  [== regs->cs]
> +         *   %rip [== regs->rip]
> +         *   SSP  [Likely points to 3 slots higher, above %cs]
> +         *   ...  [call tree to this function, likely 2/3 slots]
> +         *
> +         * and we want to overwrite %rip with fixup.  There are two
> +         * complications:
> +         *   1) We cant depend on SSP values, because they won't differ by 3
> +         *      slots if the exception is taken on an IST stack.
> +         *   2) There are synthetic (unrealistic but not impossible) 
> scenarios
> +         *      where %rip can end up in the call tree to this function, so 
> we
> +         *      can't check against regs->rip alone.
> +         *
> +         * Check for both reg->rip and regs->cs matching.

Nit: regs->rip

> +         */
> +
> +        if ( ptr[0] == regs->rip && ptr[1] == regs->cs )
> +        {
> +            asm ( "wrssq %[fix], %[stk]"
> +                  : [stk] "=m" (*ptr)

Could this be ptr[0], to match the if()?

Considering how important it is that we don't fix up the wrong stack
location here, I continue to wonder if we wouldn't better also
include the SSP value on the stack in the checking, at the very
least by way of an ASSERT() or BUG_ON(). Since, with how the code is
currently written, this would require a somewhat odd looking ptr[-1]
I also wonder whether "while ( ++ptr < base )" as the loop header
wouldn't be better. The first entry on the stack can't be the RIP
we look for anyway, can it?

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -708,7 +708,16 @@ exception_with_ints_disabled:
>          call  search_pre_exception_table
>          testq %rax,%rax                 # no fixup code for faulting EIP?
>          jz    1b
> -        movq  %rax,UREGS_rip(%rsp)
> +        movq  %rax,UREGS_rip(%rsp)      # fixup regular stack
> +
> +#ifdef CONFIG_XEN_SHSTK
> +        mov    $1, %edi
> +        rdsspq %rdi
> +        cmp    $1, %edi
> +        je     .L_exn_shstk_done
> +        wrssq  %rax, (%rdi)             # fixup shadow stack

According to the comment in extable_shstk_fixup(), isn't the value
to be replaced at 8(%rdi)?

Jan