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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/05/2020 17:15, Jan Beulich wrote:
> 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().

Well no, for the reason discussed in point 1.

Its not technically an issue right now, but there is no possible way to
BUILD_BUG_ON() someone turning an exception into IST, or stopping the
use of the extable infrastructure on a #DB.

Such a check would lie in wait and either provide an unexpected tangent
to someone debugging a complicated issue (I do use #DB for a fair bit),
or become a security vulnerability.

> 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?

Yes it can.

>> --- 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)?

Hmm - I think you're right.  I thought I had this covered by unit tests.

~Andrew