Re: [PATCH 06/16] x86/traps: Implement #CP handler and extend #PF for shadow stacks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/05/2020 15:10, Jan Beulich wrote:
> On 02.05.2020 00:58, Andrew Cooper wrote:
>> @@ -1457,6 +1451,10 @@ void do_page_fault(struct cpu_user_regs *regs)
>>      {
>>          enum pf_type pf_type = spurious_page_fault(addr, regs);
>>  
>> +        /* Any fault on a shadow stack access is a bug in Xen. */
>> +        if ( error_code & PFEC_shstk )
>> +            goto fatal;
> Not going through the full spurious_page_fault() in this case
> would seem desirable, as would be at least a respective
> adjustment to __page_fault_type(). Perhaps such an adjustment
> could then avoid the change (and the need for goto) here?

This seems to do a lot of things which have little/nothing to do with
spurious faults.

In particular, we don't need to disable interrupts to look at
PFEC_shstk, or RSVD for that matter.

>> @@ -1906,6 +1905,43 @@ void do_debug(struct cpu_user_regs *regs)
>>      pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
>>  }
>>  
>> +void do_entry_CP(struct cpu_user_regs *regs)
> If there's a plan to change other exception handlers to this naming
> scheme too, I can live with the intermediate inconsistency.

Yes.  This isn't the first time I've introduced this naming scheme
either, but the emulator patch got stuck in trivialities.

> Otherwise, though, I'd prefer a name closer to what other handlers
> use, e.g. do_control_prot(). Same for the asm entry point then.

These names are impossible to search for, because there is no
consistency even amongst the similarly named ones.

>
>> @@ -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 == TRAP_copro_seg || vec == TRAP_spurious_int || \
>> +                vec == TRAP_virtualisation || (vec > X86_EXC_CP && vec < 
>> TRAP_nr)
> Use the shorter X86_EXC_VE here, since you don't want/need to
> retain what was there before? (I'd be fine with you changing
> the two other TRAP_* too at this occasion.)

Ok.

Having played this game several times now, I'm considering reworking how
do_reserved_trap() gets set up, because we've now got two places which
can go wrong and result in an unhelpful assert early on boot, rather
than a build-time failure.  (The one thing I'm not sure on how to do is
usefully turn this into a build time failure.)

>
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -68,6 +68,7 @@
>>  #define PFEC_reserved_bit   (_AC(1,U) << 3)
>>  #define PFEC_insn_fetch     (_AC(1,U) << 4)
>>  #define PFEC_prot_key       (_AC(1,U) << 5)
>> +#define PFEC_shstk         (_AC(1,U) << 6)
> One too few padding blanks?

Oops.

~Andrew