[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 29.05.2020 21:43, Andrew Cooper wrote:
> On 28/05/2020 17:15, Jan Beulich wrote:
>> On 27.05.2020 21:18, Andrew Cooper wrote:
>>> @@ -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.
>>> +         */
>>> +
>>> +        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.

I don't see my suggestion in conflict with that point. I didn't
suggest an check using == ; instead what I'm thinking about here
is something as weak as "Does this point somewhere into the
stack range for this CPU?" After all there are only a limited
set of classes of entries that can be on a shadow stack:
- LIP (Xen .text, livepatching area)
- CS  (<= 0xffff)
- SSP (within stack range for the CPU)
- supervisor token (a single precise address)
- padding (zero)
The number ranges covered by these classes are entirely disjoint,
so qualifying all three slots accordingly can be done without any
risk of getting an entry wrong.

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

How, when there's the return address of this function plus
an SSP value preceding it?

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.