Re: [PATCH 12/16] x86/extable: Adjust extable handling to be shadow stack compatible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2020 00:58, Andrew Cooper wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -778,6 +778,28 @@ static bool exception_fixup(struct cpu_user_regs *regs, 
> bool print)
>                 vec_name(regs->entry_vector), regs->error_code,
>                 _p(regs->rip), _p(regs->rip), _p(fixup));
>  
> +    if ( IS_ENABLED(CONFIG_XEN_SHSTK) )
> +    {
> +        unsigned long ssp;
> +
> +        asm ("rdsspq %0" : "=r" (ssp) : "0" (1) );
> +        if ( ssp != 1 )
> +        {
> +            unsigned long *ptr = _p(ssp);
> +
> +            /* Search for %rip in the shadow stack, ... */
> +            while ( *ptr != regs->rip )
> +                ptr++;

Wouldn't it be better to bound the loop, as it shouldn't search past
(strictly speaking not even to) the next page boundary? Also you
don't care about the top of the stack (being the to be restored SSP),
do you? I.e. maybe

            while ( *++ptr != regs->rip )
                ;

?

And then - isn't searching for a specific RIP value alone prone to
error, in case a it matches an ordinary return address? I.e.
wouldn't you better search for a matching RIP accompanied by a
suitable pointer into the shadow stack and a matching CS value?
Otherwise, ...

> +            ASSERT(ptr[1] == __HYPERVISOR_CS);

... also assert that ptr[-1] points into the shadow stack?

> --- 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
> +.L_exn_shstk_done:
> +#endif

Again avoid the conditional jump by using alternatives patching?

Jan