Messages in this thread Patch in this message | | | From | Andrew Cooper <> | Subject | Re: [PATCH 19/29] x86/ibt,xen: Annotate away warnings | Date | Fri, 18 Feb 2022 20:24:41 +0000 |
| |
On 18/02/2022 16:49, Peter Zijlstra wrote: > The xen_iret ENDBR is needed for pre-alternative code calling the > pv_ops using indirect calls. > > The rest look like hypervisor entry points which will be IRET like > transfers and as such don't need ENDBR.
That's up for debate. Mechanically, yes - they're IRET or SYSERET.
Logically however, they're entrypoints registered with Xen, so following the spec, Xen ought to force WAIT-FOR-ENDBR.
Or we could argue that said entrypoints are registered in Xen.
The case for ENDBR for the IDT vectors is quite obviously - a stray write into the IDT can modify the entrypoint, and ENDBR limits an attacker's choices.
OTOH, the SYSCALL and SYSENTER entrypoints are latched in MSRs, and if you've got a sufficiently large security hole that the attacker can write these MSRs, you have already lost. I'm not aware of any extra security you get from forcing WAIT-FOR-ENDBR in the SYSCALL/SYSENTER flow, and suspect it was like that just for consistency.
Under Xen PV, all entrypoints are configured by explicit hypercall, not via a shared memory structure, so better match the MSR model for native. I could probably be argued away from having a RMW of MSR_U_CET in the event delivery fastpath.
I'd be tempted to leave the ENDBR's in. It feels like a safer default until we figure out how to paravirt IBT properly.
> The hypercall page comes from the hypervisor, there might or might not > be ENDBR there, not our problem.
Xen will make sure that the hypercall page contains ENDBR's if CET-IBT is available for the guest to use. Perhaps...
> --- a/arch/x86/xen/xen-head.S > +++ b/arch/x86/xen/xen-head.S > @@ -25,8 +25,8 @@ > SYM_CODE_START(hypercall_page) > .rept (PAGE_SIZE / 32) > UNWIND_HINT_FUNC > - .skip 31, 0x90 > - RET > + ANNOTATE_NOENDBR > + .skip 32, 0xcc
// Xen writes the hypercall page, and will sort out ENDBR
?
Also, somewhere in this series needs:
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 5004feb16783..e30f77264ee6 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -624,6 +624,7 @@ static struct trap_array_entry trap_array[] = { TRAP_ENTRY(exc_coprocessor_error, false ), TRAP_ENTRY(exc_alignment_check, false ), TRAP_ENTRY(exc_simd_coprocessor_error, false ), + TRAP_ENTRY(exc_control_protection, false ), }; static bool __ref get_trap_addr(void **addr, unsigned int ist) diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S index 444d824775f6..6f077aedd561 100644 --- a/arch/x86/xen/xen-asm.S +++ b/arch/x86/xen/xen-asm.S @@ -147,6 +147,7 @@ xen_pv_trap asm_exc_page_fault xen_pv_trap asm_exc_spurious_interrupt_bug xen_pv_trap asm_exc_coprocessor_error xen_pv_trap asm_exc_alignment_check +xen_pv_trap asm_exc_control_protection #ifdef CONFIG_X86_MCE xen_pv_trap asm_xenpv_exc_machine_check #endif /* CONFIG_X86_MCE */
at a minimum, and possibly also:
diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S index 444d824775f6..96db5c50a6e7 100644 --- a/arch/x86/xen/xen-asm.S +++ b/arch/x86/xen/xen-asm.S @@ -124,7 +124,7 @@ SYM_CODE_START(xen_\name) UNWIND_HINT_EMPTY pop %rcx pop %r11 - jmp \name + jmp \name + 4 * IS_ENABLED(CONFIG_X86_IBT) SYM_CODE_END(xen_\name) _ASM_NOKPROBE(xen_\name) .endm
(Entirely untested.)
~Andrew
| |