| Message ID | 20200501225838.9866-7-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Support for CET Supervisor Shadow Stacks | expand |
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? > @@ -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. 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. > @@ -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.) > --- 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? Jan
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
On 11.05.2020 19:20, Andrew Cooper wrote: > 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. Perhaps even more so a reason to make spurious_page_fault() return a new enum pf_type enumerator? In any event your reply looks more like a "yes" to my suggestion than an objection, but I may be getting it entirely wrong ... Jan
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 737ab036d2..ddbe312f89 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -158,7 +158,9 @@ void (* const exception_table[TRAP_nr])(struct cpu_user_regs *regs) = { [TRAP_alignment_check] = do_trap, [TRAP_machine_check] = (void *)do_machine_check, [TRAP_simd_error] = do_trap, - [TRAP_virtualisation ... + [TRAP_virtualisation] = do_reserved_trap, + [X86_EXC_CP] = do_entry_CP, + [X86_EXC_CP + 1 ... (ARRAY_SIZE(exception_table) - 1)] = do_reserved_trap, }; @@ -1427,14 +1429,6 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs) return 0; } -/* - * #PF error code: - * Bit 0: Protection violation (=1) ; Page not present (=0) - * Bit 1: Write access - * Bit 2: User mode (=1) ; Supervisor mode (=0) - * Bit 3: Reserved bit violation - * Bit 4: Instruction fetch - */ void do_page_fault(struct cpu_user_regs *regs) { unsigned long addr; @@ -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; + if ( (pf_type == smep_fault) || (pf_type == smap_fault) ) { console_start_sync(); @@ -1476,6 +1474,7 @@ void do_page_fault(struct cpu_user_regs *regs) return; } + fatal: if ( debugger_trap_fatal(TRAP_page_fault, regs) ) return; @@ -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) +{ + static const char errors[][10] = { + [1] = "near ret", + [2] = "far/iret", + [3] = "endbranch", + [4] = "rstorssp", + [5] = "setssbsy", + }; + const char *err = "??"; + unsigned int ec = regs->error_code; + + if ( debugger_trap_entry(TRAP_debug, regs) ) + return; + + /* Decode ec if possible */ + if ( ec < ARRAY_SIZE(errors) && errors[ec][0] ) + err = errors[ec]; + + /* + * For now, only supervisors shadow stacks should be active. A #CP from + * guest context is probably a Xen bug, but kill the guest in an attempt + * to recover. + */ + if ( guest_mode(regs) ) + { + gprintk(XENLOG_ERR, "Hit #CP[%04x] in guest context %04x:%p\n", + ec, regs->cs, _p(regs->rip)); + ASSERT_UNREACHABLE(); + domain_crash(current->domain); + return; + } + + show_execution_state(regs); + panic("CONTROL-FLOW PROTECTION FAULT: #CP[%04x] %s\n", ec, err); +} + static void __init noinline __set_intr_gate(unsigned int n, uint32_t dpl, void *addr) { @@ -1995,6 +2031,7 @@ void __init init_idt_traps(void) set_intr_gate(TRAP_alignment_check,&alignment_check); set_intr_gate(TRAP_machine_check,&machine_check); set_intr_gate(TRAP_simd_error,&simd_coprocessor_error); + set_intr_gate(X86_EXC_CP, entry_CP); /* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */ enable_each_ist(idt_table); diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S index a3ce298529..6403c0ab92 100644 --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -795,6 +795,10 @@ ENTRY(alignment_check) movl $TRAP_alignment_check,4(%rsp) jmp handle_exception +ENTRY(entry_CP) + movl $X86_EXC_CP, 4(%rsp) + jmp handle_exception + ENTRY(double_fault) movl $TRAP_double_fault,4(%rsp) /* Set AC to reduce chance of further SMAP faults */ @@ -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) 1: test $8,%spl /* 64bit exception frames are 16 byte aligned, but the word */ jz 2f /* size is 8 bytes. Check whether the processor gave us an */ diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h index 12b55e1022..5e8a0fb649 100644 --- 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) #define PFEC_arch_mask (_AC(0xffff,U)) /* Architectural PFEC values. */ /* Internally used only flags. */ #define PFEC_page_paged (1U<<16) @@ -529,6 +530,7 @@ DECLARE_TRAP_HANDLER(coprocessor_error); DECLARE_TRAP_HANDLER(simd_coprocessor_error); DECLARE_TRAP_HANDLER_CONST(machine_check); DECLARE_TRAP_HANDLER(alignment_check); +DECLARE_TRAP_HANDLER(entry_CP); DECLARE_TRAP_HANDLER(entry_int82);
For now, any #CP exception or shadow stack #PF indicate a bug in Xen, but attempt to recover if taken in guest context. Drop the comment beside do_page_fault(). It's stale (missing PFEC_prot_key), and inaccurate (PFEC_present being set means just that, not necesserily a protection violation). Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/traps.c | 55 ++++++++++++++++++++++++++++++++++------- xen/arch/x86/x86_64/entry.S | 7 +++++- xen/include/asm-x86/processor.h | 2 ++ 3 files changed, 54 insertions(+), 10 deletions(-)