| Message ID | 20220929222936.14584-30-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadowstacks for userspace | expand |
On Thu, Sep 29, 2022 at 03:29:26PM -0700, Rick Edgecombe wrote: > For the current shadow stack implementation, shadow stacks contents easily > be arbitrarily provisioned with data. I can't parse this sentence. > 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. Is anything using this right now? Wouldn't thing be safer without WRSS? (Why can't we skip this patch?)
On 10/3/22 15:28, Kees Cook wrote: > On Thu, Sep 29, 2022 at 03:29:26PM -0700, Rick Edgecombe wrote: >> For the current shadow stack implementation, shadow stacks contents easily >> be arbitrarily provisioned with data. > > I can't parse this sentence. > >> 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. > > Is anything using this right now? Wouldn't thing be safer without WRSS? > (Why can't we skip this patch?) > So that people don't write programs that need either (shstk off) or (shstk on and WRSS on) and crash or otherwise fail on kernels that support shstk but don't support WRSS, perhaps?
On Mon, Oct 03, 2022 at 04:00:36PM -0700, Andy Lutomirski wrote: > On 10/3/22 15:28, Kees Cook wrote: > > On Thu, Sep 29, 2022 at 03:29:26PM -0700, Rick Edgecombe wrote: > > > For the current shadow stack implementation, shadow stacks contents easily > > > be arbitrarily provisioned with data. > > > > I can't parse this sentence. > > > > > 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. > > > > Is anything using this right now? Wouldn't thing be safer without WRSS? > > (Why can't we skip this patch?) > > > > So that people don't write programs that need either (shstk off) or (shstk > on and WRSS on) and crash or otherwise fail on kernels that support shstk > but don't support WRSS, perhaps? Right, yes. I meant more "what programs currently need WRSS to operate under shstk? (And what is it that they are doing that needs it?)" All is see currently is compiler self-tests and emulators using it? https://codesearch.debian.net/search?q=%5Cb%28wrss%7CWRSS%29%5Cb&literal=0&perpkg=1
On Mon, Oct 03, 2022 at 03:28:47PM -0700, Kees Cook wrote: > On Thu, Sep 29, 2022 at 03:29:26PM -0700, Rick Edgecombe wrote: > > For the current shadow stack implementation, shadow stacks contents easily > > be arbitrarily provisioned with data. > > I can't parse this sentence. > > > 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. > > Is anything using this right now? Wouldn't thing be safer without WRSS? > (Why can't we skip this patch?) CRIU uses WRSS to restore the shadow stack contents. > -- > Kees Cook
On Mon, 2022-10-03 at 21:37 -0700, Kees Cook wrote: > On Mon, Oct 03, 2022 at 04:00:36PM -0700, Andy Lutomirski wrote: > > On 10/3/22 15:28, Kees Cook wrote: > > > On Thu, Sep 29, 2022 at 03:29:26PM -0700, Rick Edgecombe wrote: > > > > For the current shadow stack implementation, shadow stacks > > > > contents easily > > > > be arbitrarily provisioned with data. > > > > > > I can't parse this sentence. > > > > > > > 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. > > > > > > Is anything using this right now? Wouldn't thing be safer without > > > WRSS? > > > (Why can't we skip this patch?) > > > > > > > So that people don't write programs that need either (shstk off) or > > (shstk > > on and WRSS on) and crash or otherwise fail on kernels that support > > shstk > > but don't support WRSS, perhaps? > > Right, yes. I meant more "what programs currently need WRSS to > operate > under shstk? (And what is it that they are doing that needs it?)" > > All is see currently is compiler self-tests and emulators using it? > https://codesearch.debian.net/search?q=%5Cb%28wrss%7CWRSS%29%5Cb&literal=0&perpkg=1 Most apps that weren't just automatically compiled haven't had implementation effort yet. (of course glibc has had a bunch) I hope we would see more of that when we finally get it upstream. So I think a better question is, how many apps will need WRSS when they go to enable shadow stack. I'm thinking the answer must be some and it could be nice to catch them when they first investigate enabling it. But yes, except for Mike's CRIU branch, there aren't any programs that use it today, and we could drop it for a first implementation. I don't see it as something that would only make things less safe though. It just lets apps that can't easily work within the stricter shadow stack environment, at least get access to a weaker but still beneficial one. Kees, did you catch that it can be locked off while enabling shadow stack?
On Thu, Oct 06, 2022 at 12:38:06AM +0000, Edgecombe, Rick P wrote: > On Mon, 2022-10-03 at 21:37 -0700, Kees Cook wrote: > > On Mon, Oct 03, 2022 at 04:00:36PM -0700, Andy Lutomirski wrote: > > > On 10/3/22 15:28, Kees Cook wrote: > > > > On Thu, Sep 29, 2022 at 03:29:26PM -0700, Rick Edgecombe wrote: > > > > > For the current shadow stack implementation, shadow stacks > > > > > contents easily > > > > > be arbitrarily provisioned with data. > > > > > > > > I can't parse this sentence. > > > > > > > > > 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. > > > > > > > > Is anything using this right now? Wouldn't thing be safer without > > > > WRSS? > > > > (Why can't we skip this patch?) > > > > > > > > > > So that people don't write programs that need either (shstk off) or > > > (shstk > > > on and WRSS on) and crash or otherwise fail on kernels that support > > > shstk > > > but don't support WRSS, perhaps? > > > > Right, yes. I meant more "what programs currently need WRSS to > > operate > > under shstk? (And what is it that they are doing that needs it?)" > > > > All is see currently is compiler self-tests and emulators using it? > > > https://codesearch.debian.net/search?q=%5Cb%28wrss%7CWRSS%29%5Cb&literal=0&perpkg=1 > > Most apps that weren't just automatically compiled haven't had > implementation effort yet. (of course glibc has had a bunch) I hope we > would see more of that when we finally get it upstream. So I think a > better question is, how many apps will need WRSS when they go to enable > shadow stack. I'm thinking the answer must be some and it could be nice > to catch them when they first investigate enabling it. > > But yes, except for Mike's CRIU branch, there aren't any programs that > use it today, and we could drop it for a first implementation. I don't > see it as something that would only make things less safe though. It > just lets apps that can't easily work within the stricter shadow stack > environment, at least get access to a weaker but still beneficial one. > > Kees, did you catch that it can be locked off while enabling shadow > stack? Yup, saw that! Looks good. Thanks. :)
diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h index 8c6fab9f402a..edf681d4843a 100644 --- a/arch/x86/include/asm/cet.h +++ b/arch/x86/include/asm/cet.h @@ -25,6 +25,7 @@ int shstk_disable(void); void reset_thread_shstk(void); int setup_signal_shadow_stack(struct ksignal *ksig); int restore_signal_shadow_stack(void); +int wrss_control(bool enable); #else static inline long cet_prctl(struct task_struct *task, int option, unsigned long features) { return -EINVAL; } @@ -38,6 +39,7 @@ static inline int shstk_disable(void) { return -EOPNOTSUPP; } static inline void reset_thread_shstk(void) {} static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; } static inline int restore_signal_shadow_stack(void) { return 0; } +static inline int wrss_control(bool enable) { return -EOPNOTSUPP; } #endif /* CONFIG_X86_SHADOW_STACK */ #endif /* __ASSEMBLY__ */ diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h index 41af3a8c4fa4..d811f0c5fc4f 100644 --- a/arch/x86/include/uapi/asm/prctl.h +++ b/arch/x86/include/uapi/asm/prctl.h @@ -27,5 +27,6 @@ #define ARCH_CET_LOCK 0x4003 #define CET_SHSTK 0x1 +#define CET_WRSS 0x2 #endif /* _ASM_X86_PRCTL_H */ diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c index 873830d63adc..fc64a04366aa 100644 --- a/arch/x86/kernel/shstk.c +++ b/arch/x86/kernel/shstk.c @@ -386,6 +386,36 @@ void shstk_free(struct task_struct *tsk) unmap_shadow_stack(shstk->base, shstk->size); } +int wrss_control(bool enable) +{ + if (!cpu_feature_enabled(X86_FEATURE_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 + * when disabling. + */ + if (!feature_enabled(CET_SHSTK)) + return -EPERM; + + /* Already enabled/disabled? */ + if (feature_enabled(CET_WRSS) == enable) + return 0; + + fpu_lock_and_load(); + if (enable) { + set_clr_bits_msrl(MSR_IA32_U_CET, CET_WRSS_EN, 0); + feature_set(CET_WRSS); + } else { + set_clr_bits_msrl(MSR_IA32_U_CET, 0, CET_WRSS_EN); + feature_clr(CET_WRSS); + } + fpregs_unlock(); + + return 0; +} + int shstk_disable(void) { if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) @@ -397,12 +427,12 @@ int shstk_disable(void) fpu_lock_and_load(); /* Disable WRSS too when disabling shadow stack */ - set_clr_bits_msrl(MSR_IA32_U_CET, 0, CET_SHSTK_EN); + set_clr_bits_msrl(MSR_IA32_U_CET, 0, CET_SHSTK_EN | CET_WRSS_EN); wrmsrl(MSR_IA32_PL3_SSP, 0); fpregs_unlock(); shstk_free(current); - feature_clr(CET_SHSTK); + feature_clr(CET_SHSTK | CET_WRSS); return 0; }
For the current shadow stack implementation, shadow stacks contents easily 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. 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. 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. 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. Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- v2: - Add some commit log verbiage from (Dave Hansen) v1: - New patch. arch/x86/include/asm/cet.h | 2 ++ arch/x86/include/uapi/asm/prctl.h | 1 + arch/x86/kernel/shstk.c | 34 +++++++++++++++++++++++++++++-- 3 files changed, 35 insertions(+), 2 deletions(-)