| Message ID | 20230227222957.24501-33-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
On Mon, Feb 27, 2023 at 02:29:48PM -0800, Rick Edgecombe wrote: > From: Yu-cheng Yu <yu-cheng.yu@intel.com> > > When a signal is handled normally the context is pushed to the stack s/normally // > before handling it. For shadow stacks, since the shadow stack only track's "tracks" > return addresses, there isn't any state that needs to be pushed. However, > there are still a few things that need to be done. These things are > userspace visible and which will be kernel ABI for shadow stacks. "visible to userspace" s/which // > One is to make sure the restorer address is written to shadow stack, since > the signal handler (if not changing ucontext) returns to the restorer, and > the restorer calls sigreturn. So add the restorer on the shadow stack > before handling the signal, so there is not a conflict when the signal > handler returns to the restorer. > > The other thing to do is to place some type of checkable token on the > thread's shadow stack before handling the signal and check it during > sigreturn. This is an extra layer of protection to hamper attackers > calling sigreturn manually as in SROP-like attacks. > > For this token we can use the shadow stack data format defined earlier. ^^^ Please use passive voice in your commit message: no "we" or "I", etc. > Have the data pushed be the previous SSP. In the future the sigreturn > might want to return back to a different stack. Storing the SSP (instead > of a restore offset or something) allows for future functionality that > may want to restore to a different stack. > > So, when handling a signal push > - the SSP pointing in the shadow stack data format > - the restorer address below the restore token. > > In sigreturn, verify SSP is stored in the data format and pop the shadow > stack. ... > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c > index 13c02747386f..40f0a55762a9 100644 > --- a/arch/x86/kernel/shstk.c > +++ b/arch/x86/kernel/shstk.c > @@ -232,6 +232,104 @@ static int get_shstk_data(unsigned long *data, unsigned long __user *addr) > return 0; > } > > +static int shstk_push_sigframe(unsigned long *ssp) > +{ > + unsigned long target_ssp = *ssp; > + > + /* Token must be aligned */ > + if (!IS_ALIGNED(*ssp, 8)) > + return -EINVAL; > + > + if (!IS_ALIGNED(target_ssp, 8)) > + return -EINVAL; Those two statements are identical AFAICT. > + *ssp -= SS_FRAME_SIZE; > + if (put_shstk_data((void *__user)*ssp, target_ssp)) > + return -EFAULT; > + > + return 0; > +}
On Thu, 2023-03-09 at 18:02 +0100, Borislav Petkov wrote: > On Mon, Feb 27, 2023 at 02:29:48PM -0800, Rick Edgecombe wrote: > > From: Yu-cheng Yu <yu-cheng.yu@intel.com> > > > > When a signal is handled normally the context is pushed to the > > stack > > s/normally // It is trying to say "When a signal is handled without shadow stack, the context is pushed to the stack" > > > before handling it. For shadow stacks, since the shadow stack only > > track's > > "tracks" Right. > > > return addresses, there isn't any state that needs to be pushed. > > However, > > there are still a few things that need to be done. These things are > > userspace visible and which will be kernel ABI for shadow stacks. > > "visible to userspace" Sure. > > s/which // Ok. > > > One is to make sure the restorer address is written to shadow > > stack, since > > the signal handler (if not changing ucontext) returns to the > > restorer, and > > the restorer calls sigreturn. So add the restorer on the shadow > > stack > > before handling the signal, so there is not a conflict when the > > signal > > handler returns to the restorer. > > > > The other thing to do is to place some type of checkable token on > > the > > thread's shadow stack before handling the signal and check it > > during > > sigreturn. This is an extra layer of protection to hamper attackers > > calling sigreturn manually as in SROP-like attacks. > > > > For this token we can use the shadow stack data format defined > > earlier. > > ^^^ > > Please use passive voice in your commit message: no "we" or "I", etc. Argh, right. And it looks like I wrote this one. > > > Have the data pushed be the previous SSP. In the future the > > sigreturn > > might want to return back to a different stack. Storing the SSP > > (instead > > of a restore offset or something) allows for future functionality > > that > > may want to restore to a different stack. > > > > So, when handling a signal push > > - the SSP pointing in the shadow stack data format > > - the restorer address below the restore token. > > > > In sigreturn, verify SSP is stored in the data format and pop the > > shadow > > stack. > > ... > > > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c > > index 13c02747386f..40f0a55762a9 100644 > > --- a/arch/x86/kernel/shstk.c > > +++ b/arch/x86/kernel/shstk.c > > @@ -232,6 +232,104 @@ static int get_shstk_data(unsigned long > > *data, unsigned long __user *addr) > > return 0; > > } > > > > +static int shstk_push_sigframe(unsigned long *ssp) > > +{ > > + unsigned long target_ssp = *ssp; > > + > > + /* Token must be aligned */ > > + if (!IS_ALIGNED(*ssp, 8)) > > + return -EINVAL; > > + > > + if (!IS_ALIGNED(target_ssp, 8)) > > + return -EINVAL; > > Those two statements are identical AFAICT. Uhh, yes they are. Not sure what happened here. > > > + *ssp -= SS_FRAME_SIZE; > > + if (put_shstk_data((void *__user)*ssp, target_ssp)) > > + return -EFAULT; > > + > > + return 0; > > +} > >
On Thu, Mar 09, 2023 at 05:16:42PM +0000, Edgecombe, Rick P wrote: > On Thu, 2023-03-09 at 18:02 +0100, Borislav Petkov wrote: > > On Mon, Feb 27, 2023 at 02:29:48PM -0800, Rick Edgecombe wrote: > > > From: Yu-cheng Yu <yu-cheng.yu@intel.com> > > > > > > When a signal is handled normally the context is pushed to the > > > stack > > > > s/normally // > > It is trying to say "When a signal is handled without shadow stack, the > context is pushed to the stack" Yeah, I see that. But "normally" is implicit in the "normal" case, without shadow stack. So you don't really need to say "normally". :-)
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h index 1399f4df098b..acee68d30a07 100644 --- a/arch/x86/include/asm/shstk.h +++ b/arch/x86/include/asm/shstk.h @@ -6,6 +6,7 @@ #include <linux/types.h> struct task_struct; +struct ksignal; #ifdef CONFIG_X86_USER_SHADOW_STACK struct thread_shstk { @@ -19,6 +20,8 @@ int shstk_alloc_thread_stack(struct task_struct *p, unsigned long clone_flags, unsigned long stack_size, unsigned long *shstk_addr); void shstk_free(struct task_struct *p); +int setup_signal_shadow_stack(struct ksignal *ksig); +int restore_signal_shadow_stack(void); #else static inline long shstk_prctl(struct task_struct *task, int option, unsigned long arg2) { return -EINVAL; } @@ -28,6 +31,8 @@ static inline int shstk_alloc_thread_stack(struct task_struct *p, unsigned long stack_size, unsigned long *shstk_addr) { return 0; } static inline void shstk_free(struct task_struct *p) {} +static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; } +static inline int restore_signal_shadow_stack(void) { return 0; } #endif /* CONFIG_X86_USER_SHADOW_STACK */ #endif /* __ASSEMBLY__ */ diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c index 13c02747386f..40f0a55762a9 100644 --- a/arch/x86/kernel/shstk.c +++ b/arch/x86/kernel/shstk.c @@ -232,6 +232,104 @@ static int get_shstk_data(unsigned long *data, unsigned long __user *addr) return 0; } +static int shstk_push_sigframe(unsigned long *ssp) +{ + unsigned long target_ssp = *ssp; + + /* Token must be aligned */ + if (!IS_ALIGNED(*ssp, 8)) + return -EINVAL; + + if (!IS_ALIGNED(target_ssp, 8)) + return -EINVAL; + + *ssp -= SS_FRAME_SIZE; + if (put_shstk_data((void *__user)*ssp, target_ssp)) + return -EFAULT; + + return 0; +} + +static int shstk_pop_sigframe(unsigned long *ssp) +{ + unsigned long token_addr; + int err; + + err = get_shstk_data(&token_addr, (unsigned long __user *)*ssp); + if (unlikely(err)) + return err; + + /* Restore SSP aligned? */ + if (unlikely(!IS_ALIGNED(token_addr, 8))) + return -EINVAL; + + /* SSP in userspace? */ + if (unlikely(token_addr >= TASK_SIZE_MAX)) + return -EINVAL; + + *ssp = token_addr; + + return 0; +} + +int setup_signal_shadow_stack(struct ksignal *ksig) +{ + void __user *restorer = ksig->ka.sa.sa_restorer; + unsigned long ssp; + int err; + + if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK) || + !features_enabled(ARCH_SHSTK_SHSTK)) + return 0; + + if (!restorer) + return -EINVAL; + + ssp = get_user_shstk_addr(); + if (unlikely(!ssp)) + return -EINVAL; + + err = shstk_push_sigframe(&ssp); + if (unlikely(err)) + return err; + + /* Push restorer address */ + ssp -= SS_FRAME_SIZE; + err = write_user_shstk_64((u64 __user *)ssp, (u64)restorer); + if (unlikely(err)) + return -EFAULT; + + fpregs_lock_and_load(); + wrmsrl(MSR_IA32_PL3_SSP, ssp); + fpregs_unlock(); + + return 0; +} + +int restore_signal_shadow_stack(void) +{ + unsigned long ssp; + int err; + + if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK) || + !features_enabled(ARCH_SHSTK_SHSTK)) + return 0; + + ssp = get_user_shstk_addr(); + if (unlikely(!ssp)) + return -EINVAL; + + err = shstk_pop_sigframe(&ssp); + if (unlikely(err)) + return err; + + fpregs_lock_and_load(); + wrmsrl(MSR_IA32_PL3_SSP, ssp); + fpregs_unlock(); + + return 0; +} + void shstk_free(struct task_struct *tsk) { struct thread_shstk *shstk = &tsk->thread.shstk; diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index 004cb30b7419..356253e85ce9 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -40,6 +40,7 @@ #include <asm/syscall.h> #include <asm/sigframe.h> #include <asm/signal.h> +#include <asm/shstk.h> static inline int is_ia32_compat_frame(struct ksignal *ksig) { diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c index 0e808c72bf7e..cacf2ede6217 100644 --- a/arch/x86/kernel/signal_64.c +++ b/arch/x86/kernel/signal_64.c @@ -175,6 +175,9 @@ int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs) frame = get_sigframe(ksig, regs, sizeof(struct rt_sigframe), &fp); uc_flags = frame_uc_flags(regs); + if (setup_signal_shadow_stack(ksig)) + return -EFAULT; + if (!user_access_begin(frame, sizeof(*frame))) return -EFAULT; @@ -260,6 +263,9 @@ SYSCALL_DEFINE0(rt_sigreturn) if (!restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags)) goto badframe; + if (restore_signal_shadow_stack()) + goto badframe; + if (restore_altstack(&frame->uc.uc_stack)) goto badframe;