[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e5b93f0a-a28c-8c63-0193-4b4d0db509ab@citrix.com>
Date: Fri, 18 Feb 2022 20:24:41 +0000
From: Andrew Cooper <Andrew.Cooper3@...rix.com>
To: Peter Zijlstra <peterz@...radead.org>,
"x86@...nel.org" <x86@...nel.org>,
"joao@...rdrivepizza.com" <joao@...rdrivepizza.com>,
"hjl.tools@...il.com" <hjl.tools@...il.com>,
"jpoimboe@...hat.com" <jpoimboe@...hat.com>,
"Juergen Gross" <jgross@...e.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"ndesaulniers@...gle.com" <ndesaulniers@...gle.com>,
"keescook@...omium.org" <keescook@...omium.org>,
"samitolvanen@...gle.com" <samitolvanen@...gle.com>,
"mark.rutland@....com" <mark.rutland@....com>,
"alyssa.milburn@...el.com" <alyssa.milburn@...el.com>,
Andrew Cooper <Andrew.Cooper3@...rix.com>
Subject: Re: [PATCH 19/29] x86/ibt,xen: Annotate away warnings
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
Powered by blists - more mailing lists