Messages in this thread | | | Date | Mon, 21 Feb 2022 00:27:20 -0800 | From | Kees Cook <> | Subject | Re: [PATCH 15/29] x86: Disable IBT around firmware |
| |
On February 18, 2022 8:49:17 AM PST, Peter Zijlstra <peterz@infradead.org> wrote: >Assume firmware isn't IBT clean and disable it across calls. > >Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.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
| |