--- v1
+++ v8
@@ -1,185 +1,120 @@
-For the current shadow stack implementation, shadow stacks contents cannot
-be arbitrarily provisioned with data. This property helps apps protect
-themselves better, but also restricts any potential apps that may want to
-do exotic things at the expense of a little security.
+For the current shadow stack implementation, shadow stacks contents can't
+easily be provisioned with arbitrary data. This property helps apps
+protect themselves better, but also restricts any potential apps that may
+want to do exotic things at the expense of a little security.
-The x86 shadow stack feature introduces a new instruction, wrss, which
-can be enabled to write directly to shadow stack permissioned memory from
-userspace. Allow it to get enabled via the prctl interface.
+The x86 shadow stack feature introduces a new instruction, WRSS, which
+can be enabled to write directly to shadow stack memory from userspace.
+Allow it to get enabled via the prctl interface.
-Only enable the userspace wrss instruction, which allows writes to
+Only enable the userspace WRSS instruction, which allows writes to
userspace shadow stacks from userspace. Do not allow it to be enabled
independently of shadow stack, as HW does not support using WRSS when
shadow stack is disabled.
-Prevent shadow stack's from becoming executable to assist apps who want
-W^X enforced. Add an arch_validate_flags() implementation to handle the
-check. Rename the uapi/asm/mman.h header guard to be able to use it for
-arch/x86/include/asm/mman.h where the arch_validate_flags() will be.
-
From a fault handler perspective, WRSS will behave very similar to WRUSS,
-which is treated like a user access from a PF err code perspective.
+which is treated like a user access from a #PF err code perspective.
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
+Reviewed-by: Kees Cook <keescook@chromium.org>
+Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
+Tested-by: Pengfei Xu <pengfei.xu@intel.com>
+Tested-by: John Allen <john.allen@amd.com>
+Tested-by: Kees Cook <keescook@chromium.org>
---
+v8:
+ - Update commit log verbiage (Boris)
+ - Drop set_clr_bits_msrl() (Boris)
+ - Fix comments wrss->WRSS (Boris)
-v1:
- - New patch.
+v6:
+ - Make set_clr_bits_msrl() avoid side effects in 'msr'
- arch/x86/include/asm/cet.h | 3 +++
- arch/x86/include/asm/mman.h | 5 ++++-
- arch/x86/include/uapi/asm/prctl.h | 2 +-
- arch/x86/kernel/elf_feature_prctl.c | 6 +++++
- arch/x86/kernel/shstk.c | 35 ++++++++++++++++++++++++++++-
- 5 files changed, 48 insertions(+), 3 deletions(-)
+v5:
+ - Switch to EOPNOTSUPP
+ - Move set_clr_bits_msrl() to patch where it is first used
+ - Commit log formatting
-diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
-index cbc7cfcba5dc..c8ff0bd5f5bc 100644
---- a/arch/x86/include/asm/cet.h
-+++ b/arch/x86/include/asm/cet.h
-@@ -10,6 +10,7 @@ struct task_struct;
- struct thread_shstk {
- u64 base;
- u64 size;
-+ bool wrss;
- };
-
- #ifdef CONFIG_X86_SHADOW_STACK
-@@ -19,6 +20,7 @@ int shstk_alloc_thread_stack(struct task_struct *p, unsigned long clone_flags,
- void shstk_free(struct task_struct *p);
- int shstk_disable(void);
- void reset_thread_shstk(void);
-+int wrss_control(bool enable);
- int shstk_setup_rstor_token(bool proc32, unsigned long restorer,
- unsigned long *new_ssp);
- int shstk_check_rstor_token(bool proc32, unsigned long *new_ssp);
-@@ -32,6 +34,7 @@ static inline int shstk_alloc_thread_stack(struct task_struct *p,
- static inline void shstk_free(struct task_struct *p) {}
- static inline void shstk_disable(void) {}
- static inline void reset_thread_shstk(void) {}
-+static inline void wrss_control(bool enable) {}
- static inline int shstk_setup_rstor_token(bool proc32, unsigned long restorer,
- unsigned long *new_ssp) { return 0; }
- static inline int shstk_check_rstor_token(bool proc32,
-diff --git a/arch/x86/include/asm/mman.h b/arch/x86/include/asm/mman.h
-index b44fe31deb3a..c05951a36d93 100644
---- a/arch/x86/include/asm/mman.h
-+++ b/arch/x86/include/asm/mman.h
-@@ -8,7 +8,10 @@
- #ifdef CONFIG_X86_SHADOW_STACK
- static inline bool arch_validate_flags(unsigned long vm_flags)
- {
-- if ((vm_flags & VM_SHADOW_STACK) && (vm_flags & VM_WRITE))
-+ /*
-+ * Shadow stack must not be executable, to help with W^X due to wrss.
-+ */
-+ if ((vm_flags & VM_SHADOW_STACK) && (vm_flags & (VM_WRITE | VM_EXEC)))
- return false;
-
- return true;
+v3:
+ - Make wrss_control() static
+ - Fix verbiage in commit log (Kees)
+---
+ arch/x86/include/uapi/asm/prctl.h | 1 +
+ arch/x86/kernel/shstk.c | 43 ++++++++++++++++++++++++++++++-
+ 2 files changed, 43 insertions(+), 1 deletion(-)
+
diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
-index aa294c7bcf41..210976925325 100644
+index 7dfd9dc00509..e31495668056 100644
--- a/arch/x86/include/uapi/asm/prctl.h
+++ b/arch/x86/include/uapi/asm/prctl.h
-@@ -28,6 +28,6 @@
- /* x86 feature bits to be used with ARCH_X86_FEATURE arch_prctl()s */
- #define LINUX_X86_FEATURE_IBT 0x00000001
- #define LINUX_X86_FEATURE_SHSTK 0x00000002
--
-+#define LINUX_X86_FEATURE_WRSS 0x00000010
+@@ -28,5 +28,6 @@
+
+ /* ARCH_SHSTK_ features bits */
+ #define ARCH_SHSTK_SHSTK (1ULL << 0)
++#define ARCH_SHSTK_WRSS (1ULL << 1)
#endif /* _ASM_X86_PRCTL_H */
-diff --git a/arch/x86/kernel/elf_feature_prctl.c b/arch/x86/kernel/elf_feature_prctl.c
-index 47de201db3f7..ecad6ebeb4dd 100644
---- a/arch/x86/kernel/elf_feature_prctl.c
-+++ b/arch/x86/kernel/elf_feature_prctl.c
-@@ -21,6 +21,8 @@ static int elf_feat_copy_status_to_user(struct thread_shstk *shstk, u64 __user *
- buf[1] = shstk->base;
- buf[2] = shstk->size;
- }
-+ if (shstk->wrss)
-+ buf[0] |= LINUX_X86_FEATURE_WRSS;
-
- return copy_to_user(ubuf, buf, sizeof(buf));
- }
-@@ -40,6 +42,8 @@ int prctl_elf_feature(int option, u64 arg2)
- if (arg2 & thread->feat_prctl_locked)
- return -EPERM;
-
-+ if (arg2 & LINUX_X86_FEATURE_WRSS && !wrss_control(false))
-+ feat_succ |= LINUX_X86_FEATURE_WRSS;
- if (arg2 & LINUX_X86_FEATURE_SHSTK && !shstk_disable())
- feat_succ |= LINUX_X86_FEATURE_SHSTK;
-
-@@ -52,6 +56,8 @@ int prctl_elf_feature(int option, u64 arg2)
-
- if (arg2 & LINUX_X86_FEATURE_SHSTK && !shstk_setup())
- feat_succ |= LINUX_X86_FEATURE_SHSTK;
-+ if (arg2 & LINUX_X86_FEATURE_WRSS && !wrss_control(true))
-+ feat_succ |= LINUX_X86_FEATURE_WRSS;
-
- if (feat_succ != arg2)
- return -ECANCELED;
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
-index 53be5d5539d4..92612236b4ef 100644
+index 6d2531ce661c..01b45666f1b6 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
-@@ -230,6 +230,36 @@ void shstk_free(struct task_struct *tsk)
- shstk->size = 0;
+@@ -360,6 +360,47 @@ void shstk_free(struct task_struct *tsk)
+ unmap_shadow_stack(shstk->base, shstk->size);
}
-+int wrss_control(bool enable)
++static int wrss_control(bool enable)
+{
-+ struct thread_shstk *shstk = ¤t->thread.shstk;
-+ void *xstate;
-+ int err;
++ u64 msrval;
+
-+ if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
-+ return 1;
++ if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK))
++ return -EOPNOTSUPP;
++
+ /*
-+ * Only enable wrss if shadow stack is enabled. If shadow stack is not
-+ * enabled, wrss will already be disabled, so don't bother clearing it
++ * Only enable WRSS if shadow stack is enabled. If shadow stack is not
++ * enabled, WRSS will already be disabled, so don't bother clearing it
+ * when disabling.
+ */
-+ if (!shstk->size || shstk->wrss == enable)
-+ return 1;
++ if (!features_enabled(ARCH_SHSTK_SHSTK))
++ return -EPERM;
+
-+ xstate = start_update_xsave_msrs(XFEATURE_CET_USER);
-+ if (enable)
-+ err = xsave_set_clear_bits_msrl(xstate, MSR_IA32_U_CET, CET_WRSS_EN, 0);
-+ else
-+ err = xsave_set_clear_bits_msrl(xstate, MSR_IA32_U_CET, 0, CET_WRSS_EN);
-+ end_update_xsave_msrs();
++ /* Already enabled/disabled? */
++ if (features_enabled(ARCH_SHSTK_WRSS) == enable)
++ return 0;
+
-+ if (err)
-+ return 1;
++ fpregs_lock_and_load();
++ rdmsrl(MSR_IA32_U_CET, msrval);
+
-+ shstk->wrss = enable;
++ if (enable) {
++ features_set(ARCH_SHSTK_WRSS);
++ msrval |= CET_WRSS_EN;
++ } else {
++ features_clr(ARCH_SHSTK_WRSS);
++ if (!(msrval & CET_WRSS_EN))
++ goto unlock;
++
++ msrval &= ~CET_WRSS_EN;
++ }
++
++ wrmsrl(MSR_IA32_U_CET, msrval);
++
++unlock:
++ fpregs_unlock();
++
+ return 0;
+}
+
- int shstk_disable(void)
+ static int shstk_disable(void)
{
- struct thread_shstk *shstk = ¤t->thread.shstk;
-@@ -242,7 +272,9 @@ int shstk_disable(void)
- return 1;
-
- xstate = start_update_xsave_msrs(XFEATURE_CET_USER);
-- err = xsave_set_clear_bits_msrl(xstate, MSR_IA32_U_CET, 0, CET_SHSTK_EN);
-+ /* Disable WRSS too when disabling shadow stack */
-+ err = xsave_set_clear_bits_msrl(xstate, MSR_IA32_U_CET, 0,
-+ CET_SHSTK_EN | CET_WRSS_EN);
- if (!err)
- err = xsave_wrmsrl(xstate, MSR_IA32_PL3_SSP, 0);
- end_update_xsave_msrs();
-@@ -251,6 +283,7 @@ int shstk_disable(void)
- return 1;
+ if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK))
+@@ -376,7 +417,7 @@ static int shstk_disable(void)
+ fpregs_unlock();
shstk_free(current);
-+ shstk->wrss = 0;
+- features_clr(ARCH_SHSTK_SHSTK);
++ features_clr(ARCH_SHSTK_SHSTK | ARCH_SHSTK_WRSS);
+
return 0;
}
-
--
2.17.1