| Message ID | 20180830143904.3168-20-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control Flow Enforcement: Shadow Stack | expand |
On Thu, Aug 30, 2018 at 4:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > WRUSS is a new kernel-mode instruction but writes directly > to user shadow stack memory. This is used to construct > a return address on the shadow stack for the signal > handler. > > This instruction can fault if the user shadow stack is > invalid shadow stack memory. In that case, the kernel does > fixup. > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> [...] > +static inline int write_user_shstk_64(unsigned long addr, unsigned long val) > +{ > + int err = 0; > + > + asm volatile("1: wrussq %1, (%0)\n" > + "2:\n" > + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wruss) > + : > + : "r" (addr), "r" (val)); > + > + return err; > +} What's up with "err"? You set it to zero, and then you return it, but nothing can ever set it to non-zero, right? > +__visible bool ex_handler_wruss(const struct exception_table_entry *fixup, > + struct pt_regs *regs, int trapnr) > +{ > + regs->ip = ex_fixup_addr(fixup); > + regs->ax = -1; > + return true; > +} And here you just write into regs->ax, but your "asm volatile" doesn't reserve that register. This looks wrong to me. I think you probably want to add something like an explicit `"+&a"(err)` output to the asm statements. > @@ -1305,6 +1305,15 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code, > error_code |= X86_PF_USER; > flags |= FAULT_FLAG_USER; > } else { > + /* > + * WRUSS is a kernel instrcution and but writes Nits: typo ("instrcution"), weird grammar ("and but writes")
On Thu, Aug 30, 2018 at 8:39 AM, Jann Horn <jannh@google.com> wrote: > On Thu, Aug 30, 2018 at 4:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >> >> WRUSS is a new kernel-mode instruction but writes directly >> to user shadow stack memory. This is used to construct >> a return address on the shadow stack for the signal >> handler. >> >> This instruction can fault if the user shadow stack is >> invalid shadow stack memory. In that case, the kernel does >> fixup. >> >> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > [...] >> +static inline int write_user_shstk_64(unsigned long addr, unsigned long val) >> +{ >> + int err = 0; >> + >> + asm volatile("1: wrussq %1, (%0)\n" >> + "2:\n" >> + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wruss) >> + : >> + : "r" (addr), "r" (val)); >> + >> + return err; >> +} > > What's up with "err"? You set it to zero, and then you return it, but > nothing can ever set it to non-zero, right? > >> +__visible bool ex_handler_wruss(const struct exception_table_entry *fixup, >> + struct pt_regs *regs, int trapnr) >> +{ >> + regs->ip = ex_fixup_addr(fixup); >> + regs->ax = -1; >> + return true; >> +} > > And here you just write into regs->ax, but your "asm volatile" doesn't > reserve that register. This looks wrong to me. > > I think you probably want to add something like an explicit > `"+&a"(err)` output to the asm statements. We require asm goto support these days. How about using that? You won't even need a special exception handler. Also, please change the BUG to WARN in the you-did-it-wrong 32-bit case. And return -EFAULT. --Andy
On Thu, 2018-08-30 at 08:55 -0700, Andy Lutomirski wrote: > On Thu, Aug 30, 2018 at 8:39 AM, Jann Horn <jannh@google.com> wrote: > > > > On Thu, Aug 30, 2018 at 4:44 PM Yu-cheng Yu <yu-cheng.yu@intel.com > > > wrote: > > > > > > > > > WRUSS is a new kernel-mode instruction but writes directly > > > to user shadow stack memory. This is used to construct > > > a return address on the shadow stack for the signal > > > handler. > > > > > > This instruction can fault if the user shadow stack is > > > invalid shadow stack memory. In that case, the kernel does > > > fixup. > > > > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > > [...] > > > > > > +static inline int write_user_shstk_64(unsigned long addr, > > > unsigned long val) > > > +{ > > > + int err = 0; > > > + > > > + asm volatile("1: wrussq %1, (%0)\n" > > > + "2:\n" > > > + _ASM_EXTABLE_HANDLE(1b, 2b, > > > ex_handler_wruss) > > > + : > > > + : "r" (addr), "r" (val)); > > > + > > > + return err; > > > +} > > What's up with "err"? You set it to zero, and then you return it, > > but > > nothing can ever set it to non-zero, right? > > > > > > > > +__visible bool ex_handler_wruss(const struct > > > exception_table_entry *fixup, > > > + struct pt_regs *regs, int > > > trapnr) > > > +{ > > > + regs->ip = ex_fixup_addr(fixup); > > > + regs->ax = -1; > > > + return true; > > > +} > > And here you just write into regs->ax, but your "asm volatile" > > doesn't > > reserve that register. This looks wrong to me. > > > > I think you probably want to add something like an explicit > > `"+&a"(err)` output to the asm statements. > We require asm goto support these days. How about using that? You > won't even need a special exception handler. > > Also, please change the BUG to WARN in the you-did-it-wrong 32-bit > case. And return -EFAULT. > > --Andy I will look into that. Yu-cheng
On Thu, 2018-08-30 at 09:22 -0700, Yu-cheng Yu wrote: > On Thu, 2018-08-30 at 08:55 -0700, Andy Lutomirski wrote: > > > > On Thu, Aug 30, 2018 at 8:39 AM, Jann Horn <jannh@google.com> > > wrote: > > > > > > > > > On Thu, Aug 30, 2018 at 4:44 PM Yu-cheng Yu <yu-cheng.yu@intel.c > > > om > > > > > > > > wrote: > > > > > > > > > > > > WRUSS is a new kernel-mode instruction but writes directly > > > > to user shadow stack memory. This is used to construct > > > > a return address on the shadow stack for the signal > > > > handler. > > > > > > > > This instruction can fault if the user shadow stack is > > > > invalid shadow stack memory. In that case, the kernel does > > > > fixup. > > > > > > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > > > [...] > > > > > > > > > > > > +static inline int write_user_shstk_64(unsigned long addr, > > > > unsigned long val) > > > > +{ > > > > + int err = 0; > > > > + > > > > + asm volatile("1: wrussq %1, (%0)\n" > > > > + "2:\n" > > > > + _ASM_EXTABLE_HANDLE(1b, 2b, > > > > ex_handler_wruss) > > > > + : > > > > + : "r" (addr), "r" (val)); > > > > + > > > > + return err; > > > > +} > > > What's up with "err"? You set it to zero, and then you return > > > it, > > > but > > > nothing can ever set it to non-zero, right? > > > > > > > > > > > > > > > +__visible bool ex_handler_wruss(const struct > > > > exception_table_entry *fixup, > > > > + struct pt_regs *regs, int > > > > trapnr) > > > > +{ > > > > + regs->ip = ex_fixup_addr(fixup); > > > > + regs->ax = -1; > > > > + return true; > > > > +} > > > And here you just write into regs->ax, but your "asm volatile" > > > doesn't > > > reserve that register. This looks wrong to me. > > > > > > I think you probably want to add something like an explicit > > > `"+&a"(err)` output to the asm statements. > > We require asm goto support these days. How about using > > that? You > > won't even need a special exception handler. Maybe something like this? It looks simple now. static inline int write_user_shstk_64(unsigned long addr, unsigned long val) { asm_volatile_goto("wrussq %1, (%0)\n" "jmp %l[ok]\n" ".section .fixup,\"ax\"n" "jmp %l[fail]\n" ".previous\n" :: "r" (addr), "r" (val) :: ok, fail); ok: return 0; fail: return -1; }
On Fri, Aug 31, 2018 at 2:49 PM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > On Thu, 2018-08-30 at 09:22 -0700, Yu-cheng Yu wrote: >> On Thu, 2018-08-30 at 08:55 -0700, Andy Lutomirski wrote: >> > >> > On Thu, Aug 30, 2018 at 8:39 AM, Jann Horn <jannh@google.com> >> > wrote: >> > > >> > > >> > > On Thu, Aug 30, 2018 at 4:44 PM Yu-cheng Yu <yu-cheng.yu@intel.c >> > > om >> > > > >> > > > wrote: >> > > > >> > > > >> > > > WRUSS is a new kernel-mode instruction but writes directly >> > > > to user shadow stack memory. This is used to construct >> > > > a return address on the shadow stack for the signal >> > > > handler. >> > > > >> > > > This instruction can fault if the user shadow stack is >> > > > invalid shadow stack memory. In that case, the kernel does >> > > > fixup. >> > > > >> > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> >> > > [...] >> > > > >> > > > >> > > > +static inline int write_user_shstk_64(unsigned long addr, >> > > > unsigned long val) >> > > > +{ >> > > > + int err = 0; >> > > > + >> > > > + asm volatile("1: wrussq %1, (%0)\n" >> > > > + "2:\n" >> > > > + _ASM_EXTABLE_HANDLE(1b, 2b, >> > > > ex_handler_wruss) >> > > > + : >> > > > + : "r" (addr), "r" (val)); >> > > > + >> > > > + return err; >> > > > +} >> > > What's up with "err"? You set it to zero, and then you return >> > > it, >> > > but >> > > nothing can ever set it to non-zero, right? >> > > >> > > > >> > > > >> > > > +__visible bool ex_handler_wruss(const struct >> > > > exception_table_entry *fixup, >> > > > + struct pt_regs *regs, int >> > > > trapnr) >> > > > +{ >> > > > + regs->ip = ex_fixup_addr(fixup); >> > > > + regs->ax = -1; >> > > > + return true; >> > > > +} >> > > And here you just write into regs->ax, but your "asm volatile" >> > > doesn't >> > > reserve that register. This looks wrong to me. >> > > >> > > I think you probably want to add something like an explicit >> > > `"+&a"(err)` output to the asm statements. >> > We require asm goto support these days. How about using >> > that? You >> > won't even need a special exception handler. > > Maybe something like this? It looks simple now. > > static inline int write_user_shstk_64(unsigned long addr, unsigned > long val) > { > asm_volatile_goto("wrussq %1, (%0)\n" > "jmp %l[ok]\n" > ".section .fixup,\"ax\"n" > "jmp %l[fail]\n" > ".previous\n" > :: "r" (addr), "r" (val) > :: ok, fail); > ok: > return 0; > fail: > return -1; > } > I think you can get rid of 'jmp %l[ok]' and the ok label and just fall through. And you don't need an explicit jmp to fail -- just set the _EX_HANDLER entry to land on the fail label.
On Fri, 2018-08-31 at 15:16 -0700, Andy Lutomirski wrote: > On Fri, Aug 31, 2018 at 2:49 PM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > On Thu, 2018-08-30 at 09:22 -0700, Yu-cheng Yu wrote: > > > > > > On Thu, 2018-08-30 at 08:55 -0700, Andy Lutomirski wrote: > > > > > > > > > > > > On Thu, Aug 30, 2018 at 8:39 AM, Jann Horn <jannh@google.com> > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Thu, Aug 30, 2018 at 4:44 PM Yu-cheng Yu <yu-cheng.yu@intel.c > > > > > om > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > WRUSS is a new kernel-mode instruction but writes directly > > > > > > to user shadow stack memory. This is used to construct > > > > > > a return address on the shadow stack for the signal > > > > > > handler. > > > > > > > > > > > > This instruction can fault if the user shadow stack is > > > > > > invalid shadow stack memory. In that case, the kernel does > > > > > > fixup. > > > > > > > > > > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > +static inline int write_user_shstk_64(unsigned long addr, > > > > > > unsigned long val) > > > > > > +{ > > > > > > + int err = 0; > > > > > > + > > > > > > + asm volatile("1: wrussq %1, (%0)\n" > > > > > > + "2:\n" > > > > > > + _ASM_EXTABLE_HANDLE(1b, 2b, > > > > > > ex_handler_wruss) > > > > > > + : > > > > > > + : "r" (addr), "r" (val)); > > > > > > + > > > > > > + return err; > > > > > > +} > > > > > What's up with "err"? You set it to zero, and then you return > > > > > it, > > > > > but > > > > > nothing can ever set it to non-zero, right? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > +__visible bool ex_handler_wruss(const struct > > > > > > exception_table_entry *fixup, > > > > > > + struct pt_regs *regs, int > > > > > > trapnr) > > > > > > +{ > > > > > > + regs->ip = ex_fixup_addr(fixup); > > > > > > + regs->ax = -1; > > > > > > + return true; > > > > > > +} > > > > > And here you just write into regs->ax, but your "asm volatile" > > > > > doesn't > > > > > reserve that register. This looks wrong to me. > > > > > > > > > > I think you probably want to add something like an explicit > > > > > `"+&a"(err)` output to the asm statements. > > > > We require asm goto support these days. How about using > > > > that? You > > > > won't even need a special exception handler. > > Maybe something like this? It looks simple now. > > > > static inline int write_user_shstk_64(unsigned long addr, unsigned > > long val) > > { > > asm_volatile_goto("wrussq %1, (%0)\n" > > "jmp %l[ok]\n" > > ".section .fixup,\"ax\"n" > > "jmp %l[fail]\n" > > ".previous\n" > > :: "r" (addr), "r" (val) > > :: ok, fail); > > ok: > > return 0; > > fail: > > return -1; > > } > > > I think you can get rid of 'jmp %l[ok]' and the ok label and just fall > through. And you don't need an explicit jmp to fail -- just set the > _EX_HANDLER entry to land on the fail label. Thanks! This now looks simple and much better. Yu-cheng +static inline int write_user_shstk_64(unsigned long addr, unsigned long val) +{ + asm_volatile_goto("1: wrussq %1, (%0)\n" + _ASM_EXTABLE(1b, %l[fail]) + :: "r" (addr), "r" (val) + :: fail); + return 0; +fail: + return -1; +}
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h index 317fc59b512c..9f609e802c5c 100644 --- a/arch/x86/include/asm/special_insns.h +++ b/arch/x86/include/asm/special_insns.h @@ -237,6 +237,43 @@ static inline void clwb(volatile void *__p) : [pax] "a" (p)); } +#ifdef CONFIG_X86_INTEL_CET + +#if defined(CONFIG_IA32_EMULATION) || defined(CONFIG_X86_X32) +static inline int write_user_shstk_32(unsigned long addr, unsigned int val) +{ + int err = 0; + + asm volatile("1: wrussd %1, (%0)\n" + "2:\n" + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wruss) + : + : "r" (addr), "r" (val)); + + return err; +} +#else +static inline int write_user_shstk_32(unsigned long addr, unsigned int val) +{ + BUG(); + return 0; +} +#endif + +static inline int write_user_shstk_64(unsigned long addr, unsigned long val) +{ + int err = 0; + + asm volatile("1: wrussq %1, (%0)\n" + "2:\n" + _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wruss) + : + : "r" (addr), "r" (val)); + + return err; +} +#endif /* CONFIG_X86_INTEL_CET */ + #define nop() asm volatile ("nop") diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c index 45f5d6cf65ae..e06ff851b671 100644 --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -157,6 +157,17 @@ __visible bool ex_handler_clear_fs(const struct exception_table_entry *fixup, } EXPORT_SYMBOL(ex_handler_clear_fs); +#ifdef CONFIG_X86_INTEL_CET +__visible bool ex_handler_wruss(const struct exception_table_entry *fixup, + struct pt_regs *regs, int trapnr) +{ + regs->ip = ex_fixup_addr(fixup); + regs->ax = -1; + return true; +} +EXPORT_SYMBOL(ex_handler_wruss); +#endif + __visible bool ex_has_fault_handler(unsigned long ip) { const struct exception_table_entry *e; diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c index 3842353fb4a3..10dbb5c9aaef 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c @@ -1305,6 +1305,15 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code, error_code |= X86_PF_USER; flags |= FAULT_FLAG_USER; } else { + /* + * WRUSS is a kernel instrcution and but writes + * to user shadow stack. When a fault occurs, + * both X86_PF_USER and X86_PF_SHSTK are set. + * Clear X86_PF_USER here. + */ + if ((error_code & (X86_PF_USER | X86_PF_SHSTK)) == + (X86_PF_USER | X86_PF_SHSTK)) + error_code &= ~X86_PF_USER; if (regs->flags & X86_EFLAGS_IF) local_irq_enable(); }
WRUSS is a new kernel-mode instruction but writes directly to user shadow stack memory. This is used to construct a return address on the shadow stack for the signal handler. This instruction can fault if the user shadow stack is invalid shadow stack memory. In that case, the kernel does fixup. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> --- arch/x86/include/asm/special_insns.h | 37 ++++++++++++++++++++++++++++ arch/x86/mm/extable.c | 11 +++++++++ arch/x86/mm/fault.c | 9 +++++++ 3 files changed, 57 insertions(+)