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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 11 May 2020 18:20:07 +0100
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=andrew.cooper3@xxxxxxxxxx; spf=Pass smtp.mailfrom=Andrew.Cooper3@xxxxxxxxxx; spf=None smtp.helo=postmaster@xxxxxxxxxxxxxxx; dmarc=pass (p=none dis=none) d=citrix.com
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Mon, 11 May 2020 17:20:16 +0000
  • Ironport-sdr: 2lDwPoN6XwKW149XCgLpOVaYISXx3JP8MRpoHwDL3QnAsLr/LdrTBu+d6TjHE0zUkisYk8FYjM kwZcmUgCJKsDycWv9gOgLXHEPtahQBPta0cMaDQgC8eN7TiMXvSo36StWSS9gRkoiHghuKSp15 HMlLiCr0+utEgtzdmbkvHFCD385NVKX2hKYue9X/CC3YDF6EUW+Bi8SWzD1Js7Ll3pgDiWA9gE /APtBN76qln9diU4uWQCDk3fAIkBmhdeBLuL2P6aRN5JBxWAo7oQ474wUM50PhwLV+aIlu8Sra 6eU=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

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