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(-)
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);
--
2.11.0
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
© 2016 - 2025 Red Hat, Inc.