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
