Re: [PATCH v23 22/28] x86/cet/shstk: User-mode shadow stack support
From: Borislav Petkov <bp@alien8.de>
Date: 2021-03-18 12:32:57
Also in:
linux-api, linux-doc, linux-mm, lkml
Subject: Re: [PATCH v23 22/28] x86/cet/shstk: User-mode shadow stack support
^ Add On Tue, Mar 16, 2021 at 08:10:48AM -0700, Yu-cheng Yu wrote:
Introduce basic shadow stack enabling/disabling/allocation routines. A task's shadow stack is allocated from memory with VM_SHSTK flag and has a fixed size of min(RLIMIT_STACK, 4GB). Signed-off-by: Yu-cheng Yu <redacted> Reviewed-by: Kees Cook <redacted> --- arch/x86/include/asm/cet.h | 28 ++++++ arch/x86/include/asm/processor.h | 5 ++ arch/x86/kernel/Makefile | 2 + arch/x86/kernel/cet.c | 147 +++++++++++++++++++++++++++++++
Yeah, since Peter wants stuff split, let's call that shstk.c and the IBT stuff goes into a separate ibt.c please.
quoted hunk ↗ jump to hunk
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index dc6d149bf851..3fce5062261b 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h@@ -27,6 +27,7 @@ struct vm86; #include <asm/unwind_hints.h> #include <asm/vmxfeatures.h> #include <asm/vdso/processor.h> +#include <asm/cet.h> #include <linux/personality.h> #include <linux/cache.h>@@ -535,6 +536,10 @@ struct thread_struct { unsigned int sig_on_uaccess_err:1; +#ifdef CONFIG_X86_CET + struct cet_status cet;
struct shstk_desc shstk; or so.
quoted hunk ↗ jump to hunk
+int cet_setup_shstk(void) +{ + unsigned long addr, size; + struct cet_status *cet = ¤t->thread.cet; + + if (!static_cpu_has(X86_FEATURE_SHSTK))
cpu_feature_enabled
quoted hunk ↗ jump to hunk
+ return -EOPNOTSUPP; + + size = round_up(min(rlimit(RLIMIT_STACK), 1UL << 32), PAGE_SIZE);
^ SZ_4G
+ addr = alloc_shstk(size, 0); +
^ Superfluous newline.
quoted hunk ↗ jump to hunk
+ if (IS_ERR_VALUE(addr)) + return PTR_ERR((void *)addr); + + cet->shstk_base = addr; + cet->shstk_size = size; + + start_update_msrs(); + wrmsrl(MSR_IA32_PL3_SSP, addr + size); + wrmsrl(MSR_IA32_U_CET, CET_SHSTK_EN); + end_update_msrs(); + return 0; +} + +void cet_disable_shstk(void) +{ + struct cet_status *cet = ¤t->thread.cet; + u64 msr_val; + + if (!static_cpu_has(X86_FEATURE_SHSTK) ||
cpu_feature_enabled And put the || on the end of each line: if (!cpu_feature_enabled() || !cet->shstk_size || ... )
quoted hunk ↗ jump to hunk
+ !cet->shstk_size || !cet->shstk_base) + return; + + start_update_msrs(); + rdmsrl(MSR_IA32_U_CET, msr_val); + wrmsrl(MSR_IA32_U_CET, msr_val & ~CET_SHSTK_EN); + wrmsrl(MSR_IA32_PL3_SSP, 0); + end_update_msrs(); + + cet_free_shstk(current); +}
Put that function under cet_free_shstk().
quoted hunk ↗ jump to hunk
+void cet_free_shstk(struct task_struct *tsk) +{ + struct cet_status *cet = &tsk->thread.cet; + + if (!static_cpu_has(X86_FEATURE_SHSTK) ||
cpu_feature_enabled and as above.
quoted hunk ↗ jump to hunk
+ !cet->shstk_size || !cet->shstk_base) + return; + + if (!tsk->mm || tsk->mm != current->mm) + return;
You're operating on current here merrily but what's protecting all those paths operating on current from getting current changed underneath them due to scheduling? IOW, is preemption safely disabled in all those paths ending up here?
+
+ while (1) {Uuh, an endless loop. What guarantees we'll exit it relatively timely...
quoted hunk ↗ jump to hunk
+ int r; + + r = vm_munmap(cet->shstk_base, cet->shstk_size); + + /* + * Retry if mmap_lock is not available. + */ + if (r == -EINTR) { + cond_resched();
... that thing?
+ continue; + } + + WARN_ON_ONCE(r); + break; + } + + cet->shstk_base = 0; + cet->shstk_size = 0; +} -- 2.21.0
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette