Re: [PATCH v2 04/14] x86/traps: Implement #CP handler and extend #PF for shadow stacks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/05/2020 13:03, Jan Beulich wrote:
> On 27.05.2020 21:18, Andrew Cooper wrote:
>> For now, any #CP exception or shadow stack #PF indicate a bug in Xen, but
>> attempt to recover from #CP if taken in guest context.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with one more question and a suggestion:
>
>> @@ -1445,8 +1447,10 @@ void do_page_fault(struct cpu_user_regs *regs)
>>       *
>>       * Anything remaining is an error, constituting corruption of the
>>       * pagetables and probably an L1TF vulnerable gadget.
>> +     *
>> +     * Any shadow stack access fault is a bug in Xen.
>>       */
>> -    if ( error_code & PFEC_reserved_bit )
>> +    if ( error_code & (PFEC_reserved_bit | PFEC_shstk) )
>>          goto fatal;
> Wouldn't you saying "any" imply putting this new check higher up, in
> particular ahead of the call to fixup_page_fault()?

Can do.

>
>> @@ -940,7 +944,8 @@ autogen_stubs: /* Automatically generated stubs. */
>>          entrypoint 1b
>>  
>>          /* Reserved exceptions, heading towards do_reserved_trap(). */
>> -        .elseif vec == TRAP_copro_seg || vec == TRAP_spurious_int || (vec > 
>> TRAP_simd_error && vec < TRAP_nr)
>> +        .elseif vec == X86_EXC_CSO || vec == X86_EXC_SPV || \
>> +                vec == X86_EXC_VE  || (vec > X86_EXC_CP && vec < TRAP_nr)
> Adding yet another || here adds to the fragility of the entire
> construct. Wouldn't it be better to implement do_entry_VE at
> this occasion, even its handling continues to end up in
> do_reserved_trap()? This would have the benefit of avoiding the
> pointless checking of %spl first thing in its handling. Feel
> free to keep the R-b if you decide to go this route.

I actually have a different plan, which deletes this entire clause, and
simplifies our autogen sanity checking somewhat.

For vectors which Xen has no implementation of (for whatever reason),
use DPL0, non-present descriptors, and redirect #NP[IDT] into
do_reserved_trap().

No need for any entry logic for the trivially fatal paths, or safety
against being uncertain about error codes.

However, its a little too close to 4.14 to clean this up now.

~Andrew