| Message ID | 20180607143807.3611-4-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > Set and restore shadow stack pointer for signals. How does this interact with siglongjmp()? This patch makes me extremely nervous due to the possibility of ABI issues and CRIU breakage. > diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h > index 844d60eb1882..6c8997a0156a 100644 > --- a/arch/x86/include/uapi/asm/sigcontext.h > +++ b/arch/x86/include/uapi/asm/sigcontext.h > @@ -230,6 +230,7 @@ struct sigcontext_32 { > __u32 fpstate; /* Zero when no FPU/extended context */ > __u32 oldmask; > __u32 cr2; > + __u32 ssp; > }; > > /* > @@ -262,6 +263,7 @@ struct sigcontext_64 { > __u64 trapno; > __u64 oldmask; > __u64 cr2; > + __u64 ssp; > > /* > * fpstate is really (struct _fpstate *) or (struct _xstate *) > @@ -320,6 +322,7 @@ struct sigcontext { > struct _fpstate __user *fpstate; > __u32 oldmask; > __u32 cr2; > + __u32 ssp; Is it actually okay to modify these structures like this? They're part of the user ABI, and I don't know whether any user code relies on the size being constant. > +int cet_push_shstk(int ia32, unsigned long ssp, unsigned long val) > +{ > + if (val >= TASK_SIZE) > + return -EINVAL; TASK_SIZE_MAX. But I'm a bit unsure why you need this check at all. > +int cet_restore_signal(unsigned long ssp) > +{ > + if (!current->thread.cet.shstk_enabled) > + return 0; > + return cet_set_shstk_ptr(ssp); > +} This will blow up if the shadow stack enabled state changes in a signal handler. Maybe we don't care.
On 06/07/2018 08:30 PM, Andy Lutomirski wrote: > On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >> >> Set and restore shadow stack pointer for signals. > > How does this interact with siglongjmp()? We plan to use some unused signal mask bits in the jump buffer (we have a lot of those in glibc for some reason) to store the shadow stack pointer. > This patch makes me extremely nervous due to the possibility of ABI > issues and CRIU breakage. > >> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h >> index 844d60eb1882..6c8997a0156a 100644 >> --- a/arch/x86/include/uapi/asm/sigcontext.h >> +++ b/arch/x86/include/uapi/asm/sigcontext.h >> @@ -230,6 +230,7 @@ struct sigcontext_32 { >> __u32 fpstate; /* Zero when no FPU/extended context */ >> __u32 oldmask; >> __u32 cr2; >> + __u32 ssp; >> }; >> >> /* >> @@ -262,6 +263,7 @@ struct sigcontext_64 { >> __u64 trapno; >> __u64 oldmask; >> __u64 cr2; >> + __u64 ssp; >> >> /* >> * fpstate is really (struct _fpstate *) or (struct _xstate *) >> @@ -320,6 +322,7 @@ struct sigcontext { >> struct _fpstate __user *fpstate; >> __u32 oldmask; >> __u32 cr2; >> + __u32 ssp; > > Is it actually okay to modify these structures like this? They're > part of the user ABI, and I don't know whether any user code relies on > the size being constant. Probably not. Historically, these things have been tacked at the end of the floating point state, see struct _xstate: /* New processor state extensions go here: */ However, I'm not sure if this is really ideal because I doubt that everyone who needs the shadow stack pointer also wants to sacrifice space for the AVX-512 save area (which is already a backwards compatibility hazard). Other architectures have variable offsets and some TLV-style setup here. Thanks, Florian
On Thu, 2018-06-07 at 20:58 +0200, Florian Weimer wrote: > On 06/07/2018 08:30 PM, Andy Lutomirski wrote: > > On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > >> > >> Set and restore shadow stack pointer for signals. > > > > How does this interact with siglongjmp()? > > We plan to use some unused signal mask bits in the jump buffer (we have > a lot of those in glibc for some reason) to store the shadow stack pointer. > > > This patch makes me extremely nervous due to the possibility of ABI > > issues and CRIU breakage. > > > >> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h > >> index 844d60eb1882..6c8997a0156a 100644 > >> --- a/arch/x86/include/uapi/asm/sigcontext.h > >> +++ b/arch/x86/include/uapi/asm/sigcontext.h > >> @@ -230,6 +230,7 @@ struct sigcontext_32 { > >> __u32 fpstate; /* Zero when no FPU/extended context */ > >> __u32 oldmask; > >> __u32 cr2; > >> + __u32 ssp; > >> }; > >> > >> /* > >> @@ -262,6 +263,7 @@ struct sigcontext_64 { > >> __u64 trapno; > >> __u64 oldmask; > >> __u64 cr2; > >> + __u64 ssp; > >> > >> /* > >> * fpstate is really (struct _fpstate *) or (struct _xstate *) > >> @@ -320,6 +322,7 @@ struct sigcontext { > >> struct _fpstate __user *fpstate; > >> __u32 oldmask; > >> __u32 cr2; > >> + __u32 ssp; > > > > Is it actually okay to modify these structures like this? They're > > part of the user ABI, and I don't know whether any user code relies on > > the size being constant. > > Probably not. Historically, these things have been tacked at the end of > the floating point state, see struct _xstate: > > /* New processor state extensions go here: */ > > However, I'm not sure if this is really ideal because I doubt that > everyone who needs the shadow stack pointer also wants to sacrifice > space for the AVX-512 save area (which is already a backwards > compatibility hazard). Other architectures have variable offsets and > some TLV-style setup here. > > Thanks, > Florian I will move 'ssp' to _xstate for now, and look for other ways.
On Thu, Jun 07, 2018 at 11:30:34AM -0700, Andy Lutomirski wrote: > On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > Set and restore shadow stack pointer for signals. > > How does this interact with siglongjmp()? > > This patch makes me extremely nervous due to the possibility of ABI > issues and CRIU breakage. > > > diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h > > index 844d60eb1882..6c8997a0156a 100644 > > --- a/arch/x86/include/uapi/asm/sigcontext.h > > +++ b/arch/x86/include/uapi/asm/sigcontext.h > > @@ -230,6 +230,7 @@ struct sigcontext_32 { > > __u32 fpstate; /* Zero when no FPU/extended context */ > > __u32 oldmask; > > __u32 cr2; > > + __u32 ssp; > > }; > > > > /* > > @@ -262,6 +263,7 @@ struct sigcontext_64 { > > __u64 trapno; > > __u64 oldmask; > > __u64 cr2; > > + __u64 ssp; > > > > /* > > * fpstate is really (struct _fpstate *) or (struct _xstate *) > > @@ -320,6 +322,7 @@ struct sigcontext { > > struct _fpstate __user *fpstate; > > __u32 oldmask; > > __u32 cr2; > > + __u32 ssp; > > Is it actually okay to modify these structures like this? They're > part of the user ABI, and I don't know whether any user code relies on > the size being constant. For sure it might cause problems for CRIU since we have similar definitions for this structure inside our code. That said if kernel is about to modify the structures it should keep backward compatibility at least if a user passes previous version of a structure @ssp should be set to something safe by the kernel itself. I didn't read the whole series of patches in details yet, hopefully will be able tomorrow. Thanks Andy for CC'ing!
On Thu, 2018-06-07 at 11:30 -0700, Andy Lutomirski wrote: > On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > Set and restore shadow stack pointer for signals. > > How does this interact with siglongjmp()? > > This patch makes me extremely nervous due to the possibility of ABI > issues and CRIU breakage. Longjmp/Siglongjmp is handled in GLIBC and basically the shadow stack pointer is unwound. There could be some unexpected conditions. However, we run all GLIBC tests. > > > diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h > > index 844d60eb1882..6c8997a0156a 100644 > > --- a/arch/x86/include/uapi/asm/sigcontext.h > > +++ b/arch/x86/include/uapi/asm/sigcontext.h > > @@ -230,6 +230,7 @@ struct sigcontext_32 { > > __u32 fpstate; /* Zero when no FPU/extended context */ > > __u32 oldmask; > > __u32 cr2; > > + __u32 ssp; > > }; > > > > /* > > @@ -262,6 +263,7 @@ struct sigcontext_64 { > > __u64 trapno; > > __u64 oldmask; > > __u64 cr2; > > + __u64 ssp; > > > > /* > > * fpstate is really (struct _fpstate *) or (struct _xstate *) > > @@ -320,6 +322,7 @@ struct sigcontext { > > struct _fpstate __user *fpstate; > > __u32 oldmask; > > __u32 cr2; > > + __u32 ssp; > > Is it actually okay to modify these structures like this? They're > part of the user ABI, and I don't know whether any user code relies on > the size being constant. > > > +int cet_push_shstk(int ia32, unsigned long ssp, unsigned long val) > > +{ > > + if (val >= TASK_SIZE) > > + return -EINVAL; > > TASK_SIZE_MAX. But I'm a bit unsure why you need this check at all. If an invalid address is put on the shadow stack, the task will get a control protection fault. I will change it to TASK_SIZE_MAX. > > > +int cet_restore_signal(unsigned long ssp) > > +{ > > + if (!current->thread.cet.shstk_enabled) > > + return 0; > > + return cet_set_shstk_ptr(ssp); > > +} > > This will blow up if the shadow stack enabled state changes in a > signal handler. Maybe we don't care. Yes, the task will get a control protection fault.
On 06/07/2018 01:12 PM, Yu-cheng Yu wrote: >>> +int cet_restore_signal(unsigned long ssp) >>> +{ >>> + if (!current->thread.cet.shstk_enabled) >>> + return 0; >>> + return cet_set_shstk_ptr(ssp); >>> +} >> This will blow up if the shadow stack enabled state changes in a >> signal handler. Maybe we don't care. > Yes, the task will get a control protection fault. Sounds like something to add to the very long list of things that are unwise to do in a signal handler. Great manpage fodder.
On Thu, Jun 7, 2018 at 1:07 PM Cyrill Gorcunov <gorcunov@gmail.com> wrote: > > On Thu, Jun 07, 2018 at 11:30:34AM -0700, Andy Lutomirski wrote: > > On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > > > Set and restore shadow stack pointer for signals. > > > > How does this interact with siglongjmp()? > > > > This patch makes me extremely nervous due to the possibility of ABI > > issues and CRIU breakage. > > > > > diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h > > > index 844d60eb1882..6c8997a0156a 100644 > > > --- a/arch/x86/include/uapi/asm/sigcontext.h > > > +++ b/arch/x86/include/uapi/asm/sigcontext.h > > > @@ -230,6 +230,7 @@ struct sigcontext_32 { > > > __u32 fpstate; /* Zero when no FPU/extended context */ > > > __u32 oldmask; > > > __u32 cr2; > > > + __u32 ssp; > > > }; > > > > > > /* > > > @@ -262,6 +263,7 @@ struct sigcontext_64 { > > > __u64 trapno; > > > __u64 oldmask; > > > __u64 cr2; > > > + __u64 ssp; > > > > > > /* > > > * fpstate is really (struct _fpstate *) or (struct _xstate *) > > > @@ -320,6 +322,7 @@ struct sigcontext { > > > struct _fpstate __user *fpstate; > > > __u32 oldmask; > > > __u32 cr2; > > > + __u32 ssp; > > > > Is it actually okay to modify these structures like this? They're > > part of the user ABI, and I don't know whether any user code relies on > > the size being constant. > > For sure it might cause problems for CRIU since we have > similar definitions for this structure inside our code. > That said if kernel is about to modify the structures it > should keep backward compatibility at least if a user > passes previous version of a structure @ssp should be > set to something safe by the kernel itself. > > I didn't read the whole series of patches in details > yet, hopefully will be able tomorrow. Thanks Andy for > CC'ing! We have uc_flags. It might be useful to carve out some of the flag space (24 bits?) to indicate something like the *size* of sigcontext and teach the kernel that new sigcontext fields should only be parsed on sigreturn() if the size is large enough.
On Thu, Jun 07, 2018 at 01:57:03PM -0700, Andy Lutomirski wrote: ... > > > > I didn't read the whole series of patches in details > > yet, hopefully will be able tomorrow. Thanks Andy for > > CC'ing! > > We have uc_flags. It might be useful to carve out some of the flag > space (24 bits?) to indicate something like the *size* of sigcontext > and teach the kernel that new sigcontext fields should only be parsed > on sigreturn() if the size is large enough. Yes, this should do the trick.
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c index 86b1341cba9a..26a776baff7c 100644 --- a/arch/x86/ia32/ia32_signal.c +++ b/arch/x86/ia32/ia32_signal.c @@ -34,6 +34,7 @@ #include <asm/sigframe.h> #include <asm/sighandling.h> #include <asm/smap.h> +#include <asm/cet.h> /* * Do a signal return; undo the signal stack. @@ -74,6 +75,7 @@ static int ia32_restore_sigcontext(struct pt_regs *regs, unsigned int tmpflags, err = 0; void __user *buf; u32 tmp; + u32 ssp; /* Always make any pending restarted system calls return -EINTR */ current->restart_block.fn = do_no_restart_syscall; @@ -104,9 +106,11 @@ static int ia32_restore_sigcontext(struct pt_regs *regs, get_user_ex(tmp, &sc->fpstate); buf = compat_ptr(tmp); + get_user_ex(ssp, &sc->ssp); } get_user_catch(err); err |= fpu__restore_sig(buf, 1); + err |= cet_restore_signal((unsigned long)ssp); force_iret(); @@ -194,6 +198,7 @@ static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc, put_user_ex(current->thread.trap_nr, &sc->trapno); put_user_ex(current->thread.error_code, &sc->err); put_user_ex(regs->ip, &sc->ip); + put_user_ex((u32)cet_get_shstk_ptr(), &sc->ssp); put_user_ex(regs->cs, (unsigned int __user *)&sc->cs); put_user_ex(regs->flags, &sc->flags); put_user_ex(regs->sp, &sc->sp_at_signal); diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h index 9d5bc1efc9b7..5507469cb803 100644 --- a/arch/x86/include/asm/cet.h +++ b/arch/x86/include/asm/cet.h @@ -17,14 +17,21 @@ struct cet_stat { #ifdef CONFIG_X86_INTEL_CET unsigned long cet_get_shstk_ptr(void); +int cet_push_shstk(int ia32, unsigned long ssp, unsigned long val); int cet_setup_shstk(void); void cet_disable_shstk(void); void cet_disable_free_shstk(struct task_struct *p); +int cet_restore_signal(unsigned long ssp); +int cet_setup_signal(int ia32, unsigned long addr); #else static inline unsigned long cet_get_shstk_ptr(void) { return 0; } +static inline int cet_push_shstk(int ia32, unsigned long ssp, + unsigned long val) { return 0; } static inline int cet_setup_shstk(void) { return 0; } static inline void cet_disable_shstk(void) {} static inline void cet_disable_free_shstk(struct task_struct *p) {} +static inline int cet_restore_signal(unsigned long ssp) { return 0; } +static inline int cet_setup_signal(int ia32, unsigned long addr) { return 0; } #endif #endif /* __ASSEMBLY__ */ diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h index 844d60eb1882..6c8997a0156a 100644 --- a/arch/x86/include/uapi/asm/sigcontext.h +++ b/arch/x86/include/uapi/asm/sigcontext.h @@ -230,6 +230,7 @@ struct sigcontext_32 { __u32 fpstate; /* Zero when no FPU/extended context */ __u32 oldmask; __u32 cr2; + __u32 ssp; }; /* @@ -262,6 +263,7 @@ struct sigcontext_64 { __u64 trapno; __u64 oldmask; __u64 cr2; + __u64 ssp; /* * fpstate is really (struct _fpstate *) or (struct _xstate *) @@ -320,6 +322,7 @@ struct sigcontext { struct _fpstate __user *fpstate; __u32 oldmask; __u32 cr2; + __u32 ssp; }; # else /* __x86_64__: */ struct sigcontext { @@ -377,6 +380,7 @@ struct sigcontext { __u64 trapno; __u64 oldmask; __u64 cr2; + __u64 ssp; struct _fpstate __user *fpstate; /* Zero when no FPU context */ # ifdef __ILP32__ __u32 __fpstate_pad; diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c index 8abbfd44322a..6f445ce94c83 100644 --- a/arch/x86/kernel/cet.c +++ b/arch/x86/kernel/cet.c @@ -17,6 +17,7 @@ #include <asm/fpu/xstate.h> #include <asm/fpu/types.h> #include <asm/cet.h> +#include <asm/special_insns.h> #define SHSTK_SIZE (0x8000 * (test_thread_flag(TIF_IA32) ? 4 : 8)) @@ -47,6 +48,24 @@ unsigned long cet_get_shstk_ptr(void) return ptr; } +int cet_push_shstk(int ia32, unsigned long ssp, unsigned long val) +{ + if (val >= TASK_SIZE) + return -EINVAL; + + if (IS_ENABLED(CONFIG_IA32_EMULATION) && ia32) { + if (!IS_ALIGNED(ssp, 4)) + return -EINVAL; + cet_set_shstk_ptr(ssp); + return write_user_shstk_32(ssp, (unsigned int)val); + } else { + if (!IS_ALIGNED(ssp, 8)) + return -EINVAL; + cet_set_shstk_ptr(ssp); + return write_user_shstk_64(ssp, val); + } +} + static unsigned long shstk_mmap(unsigned long addr, unsigned long len) { struct mm_struct *mm = current->mm; @@ -121,3 +140,35 @@ void cet_disable_free_shstk(struct task_struct *tsk) tsk->thread.cet.shstk_enabled = 0; } + +int cet_restore_signal(unsigned long ssp) +{ + if (!current->thread.cet.shstk_enabled) + return 0; + return cet_set_shstk_ptr(ssp); +} + +int cet_setup_signal(int ia32, unsigned long rstor_addr) +{ + unsigned long ssp; + struct cet_stat *cet = ¤t->thread.cet; + + if (!current->thread.cet.shstk_enabled) + return 0; + + ssp = cet_get_shstk_ptr(); + + /* + * Put the restorer address on the shstk + */ + if (ia32) + ssp -= sizeof(u32); + else + ssp -= sizeof(rstor_addr); + + if (ssp >= (cet->shstk_base + cet->shstk_size) || + ssp < cet->shstk_base) + return -EINVAL; + + return cet_push_shstk(ia32, ssp, rstor_addr); +} diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index da270b95fe4d..86fb897cae19 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -46,6 +46,7 @@ #include <asm/sigframe.h> #include <asm/signal.h> +#include <asm/cet.h> #define COPY(x) do { \ get_user_ex(regs->x, &sc->x); \ @@ -102,6 +103,7 @@ static int restore_sigcontext(struct pt_regs *regs, void __user *buf; unsigned int tmpflags; unsigned int err = 0; + unsigned long ssp = 0; /* Always make any pending restarted system calls return -EINTR */ current->restart_block.fn = do_no_restart_syscall; @@ -148,9 +150,11 @@ static int restore_sigcontext(struct pt_regs *regs, get_user_ex(buf_val, &sc->fpstate); buf = (void __user *)buf_val; + get_user_ex(ssp, &sc->ssp); } get_user_catch(err); err |= fpu__restore_sig(buf, IS_ENABLED(CONFIG_X86_32)); + err |= cet_restore_signal(ssp); force_iret(); @@ -193,6 +197,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate, put_user_ex(current->thread.trap_nr, &sc->trapno); put_user_ex(current->thread.error_code, &sc->err); put_user_ex(regs->ip, &sc->ip); + put_user_ex(cet_get_shstk_ptr(), &sc->ssp); #ifdef CONFIG_X86_32 put_user_ex(regs->cs, (unsigned int __user *)&sc->cs); put_user_ex(regs->flags, &sc->flags); @@ -742,6 +747,12 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs) user_disable_single_step(current); failed = (setup_rt_frame(ksig, regs) < 0); + if (!failed) { + unsigned long rstor = (unsigned long)ksig->ka.sa.sa_restorer; + int ia32 = is_ia32_frame(ksig); + + failed = cet_setup_signal(ia32, rstor); + } if (!failed) { /* * Clear the direction flag as per the ABI for function entry.
Set and restore shadow stack pointer for signals. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> --- arch/x86/ia32/ia32_signal.c | 5 ++++ arch/x86/include/asm/cet.h | 7 +++++ arch/x86/include/uapi/asm/sigcontext.h | 4 +++ arch/x86/kernel/cet.c | 51 ++++++++++++++++++++++++++++++++++ arch/x86/kernel/signal.c | 11 ++++++++ 5 files changed, 78 insertions(+)