Re: [PATCH 19/29] x86/ibt,xen: Annotate away warnings

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

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