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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<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