linux-kernel - Re: [PATCH 15/29] x86: Disable IBT around firmware

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: <831051EB-FF09-4B75-98EE-A7C8D0054CFE@chromium.org>
Date:   Mon, 21 Feb 2022 00:27:20 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Peter Zijlstra <peterz@...radead.org>, x86@...nel.org,
        joao@...rdrivepizza.com, hjl.tools@...il.com, jpoimboe@...hat.com,
        andrew.cooper3@...rix.com
CC:     linux-kernel@...r.kernel.org, peterz@...radead.org,
        ndesaulniers@...gle.com, samitolvanen@...gle.com,
        mark.rutland@....com, alyssa.milburn@...el.com
Subject: Re: [PATCH 15/29] x86: Disable IBT around firmware



On February 18, 2022 8:49:17 AM PST, Peter Zijlstra <peterz@...radead.org> wrote:
>Assume firmware isn't IBT clean and disable it across calls.
>
>Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
>---
> arch/x86/include/asm/efi.h   |    9 +++++++--
> arch/x86/include/asm/ibt.h   |   10 ++++++++++
> arch/x86/kernel/apm_32.c     |    7 +++++++
> arch/x86/kernel/cpu/common.c |   28 ++++++++++++++++++++++++++++
> 4 files changed, 52 insertions(+), 2 deletions(-)
>
>--- a/arch/x86/include/asm/efi.h
>+++ b/arch/x86/include/asm/efi.h
>@@ -7,6 +7,7 @@
> #include <asm/tlb.h>
> #include <asm/nospec-branch.h>
> #include <asm/mmu_context.h>
>+#include <asm/ibt.h>
> #include <linux/build_bug.h>
> #include <linux/kernel.h>
> #include <linux/pgtable.h>
>@@ -120,8 +121,12 @@ extern asmlinkage u64 __efi_call(void *f
> 	efi_enter_mm();							\
> })
> 
>-#define arch_efi_call_virt(p, f, args...)				\
>-	efi_call((void *)p->f, args)					\
>+#define arch_efi_call_virt(p, f, args...) ({				\
>+	u64 ret, ibt = ibt_save();					\
>+	ret = efi_call((void *)p->f, args);				\
>+	ibt_restore(ibt);						\
>+	ret;								\
>+})
> 
> #define arch_efi_call_virt_teardown()					\
> ({									\
>--- a/arch/x86/include/asm/ibt.h
>+++ b/arch/x86/include/asm/ibt.h
>@@ -6,6 +6,8 @@
> 
> #ifndef __ASSEMBLY__
> 
>+#include <linux/types.h>
>+
> #ifdef CONFIG_X86_64
> #define ASM_ENDBR	"endbr64\n\t"
> #else
>@@ -25,6 +27,9 @@ static inline bool is_endbr(const void *
> 	return val == ~0xfa1e0ff3;
> }
> 
>+extern u64 ibt_save(void);
>+extern void ibt_restore(u64 save);
>+
> #else /* __ASSEMBLY__ */
> 
> #ifdef CONFIG_X86_64
>@@ -39,10 +44,15 @@ static inline bool is_endbr(const void *
> 
> #ifndef __ASSEMBLY__
> 
>+#include <linux/types.h>
>+
> #define ASM_ENDBR
> 
> #define __noendbr
> 
>+static inline u64 ibt_save(void) { return 0; }
>+static inline void ibt_restore(u64 save) { }
>+
> #else /* __ASSEMBLY__ */
> 
> #define ENDBR
>--- a/arch/x86/kernel/apm_32.c
>+++ b/arch/x86/kernel/apm_32.c
>@@ -232,6 +232,7 @@
> #include <asm/paravirt.h>
> #include <asm/reboot.h>
> #include <asm/nospec-branch.h>
>+#include <asm/ibt.h>
> 
> #if defined(CONFIG_APM_DISPLAY_BLANK) && defined(CONFIG_VT)
> extern int (*console_blank_hook)(int);
>@@ -598,6 +599,7 @@ static long __apm_bios_call(void *_call)
> 	struct desc_struct	save_desc_40;
> 	struct desc_struct	*gdt;
> 	struct apm_bios_call	*call = _call;
>+	u64			ibt;
> 
> 	cpu = get_cpu();
> 	BUG_ON(cpu != 0);
>@@ -607,11 +609,13 @@ static long __apm_bios_call(void *_call)
> 
> 	apm_irq_save(flags);
> 	firmware_restrict_branch_speculation_start();
>+	ibt = ibt_save();
> 	APM_DO_SAVE_SEGS;
> 	apm_bios_call_asm(call->func, call->ebx, call->ecx,
> 			  &call->eax, &call->ebx, &call->ecx, &call->edx,
> 			  &call->esi);
> 	APM_DO_RESTORE_SEGS;
>+	ibt_restore(ibt);
> 	firmware_restrict_branch_speculation_end();
> 	apm_irq_restore(flags);
> 	gdt[0x40 / 8] = save_desc_40;
>@@ -676,6 +680,7 @@ static long __apm_bios_call_simple(void
> 	struct desc_struct	save_desc_40;
> 	struct desc_struct	*gdt;
> 	struct apm_bios_call	*call = _call;
>+	u64			ibt;
> 
> 	cpu = get_cpu();
> 	BUG_ON(cpu != 0);
>@@ -685,10 +690,12 @@ static long __apm_bios_call_simple(void
> 
> 	apm_irq_save(flags);
> 	firmware_restrict_branch_speculation_start();
>+	ibt = ibt_save();
> 	APM_DO_SAVE_SEGS;
> 	error = apm_bios_call_simple_asm(call->func, call->ebx, call->ecx,
> 					 &call->eax);
> 	APM_DO_RESTORE_SEGS;
>+	ibt_restore(ibt);
> 	firmware_restrict_branch_speculation_end();
> 	apm_irq_restore(flags);
> 	gdt[0x40 / 8] = save_desc_40;
>--- a/arch/x86/kernel/cpu/common.c
>+++ b/arch/x86/kernel/cpu/common.c
>@@ -592,6 +592,34 @@ static __init int setup_disable_pku(char
> __setup("nopku", setup_disable_pku);
> #endif /* CONFIG_X86_64 */
> 
>+#ifdef CONFIG_X86_IBT
>+
>+u64 ibt_save(void)
>+{
>+	u64 msr = 0;
>+
>+	if (cpu_feature_enabled(X86_FEATURE_IBT)) {
>+		rdmsrl(MSR_IA32_S_CET, msr);
>+		wrmsrl(MSR_IA32_S_CET, msr & ~CET_ENDBR_EN);
>+	}
>+
>+	return msr;
>+}
>+
>+void ibt_restore(u64 save)

Please make these both __always_inline so there no risk of them ever gaining ENDBRs and being used by ROP to disable IBT...

>+{
>+	u64 msr;
>+
>+	if (cpu_feature_enabled(X86_FEATURE_IBT)) {
>+		rdmsrl(MSR_IA32_S_CET, msr);
>+		msr &= ~CET_ENDBR_EN;
>+		msr |= (save & CET_ENDBR_EN);
>+		wrmsrl(MSR_IA32_S_CET, msr);
>+	}
>+}
>+
>+#endif
>+
> static __always_inline void setup_cet(struct cpuinfo_x86 *c)
> {
> 	u64 msr;
>
>

-- 
Kees Cook