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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.05.2020 23:17, Andrew Cooper wrote:
> On 29/05/2020 20:43, Andrew Cooper wrote:
>> On 28/05/2020 17:15, Jan Beulich wrote:
>>> On 27.05.2020 21:18, Andrew Cooper wrote:
>>>> +
>>>> +        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.
> 
> Also (which I forgot first time around),
> 
> The ptr[1] == regs->cs check is 64 bits wide, and the way to get that on
> the shadow stack would be to execute a call instruction ending at at
> 0xe008 linear, or from a bad WRSSQ edit, both of which are serious
> errors and worthy of hitting the BUG().

Maybe you misunderstood me then: By suggesting more strict checking
I'm actually asking for it to become more likely to hit that BUG(),
not less likely. (This is despite agreeing that the very limited
range of CS values on the stack already makes it practically
impossible to mistake the frame found.)

Jan