Re: [PATCH v7 24/26] x86: Enable User-Mode Instruction Prevention
On Fri, May 05, 2017 at 11:17:22AM -0700, Ricardo Neri wrote: > User_mode Instruction Prevention (UMIP) is enabled by setting/clearing a > bit in %cr4. > > It makes sense to enable UMIP at some point while booting, before user > spaces come up. Like SMAP and SMEP, is not critical to have it enabled > very early during boot. This is because UMIP is relevant only when there is > a userspace to be protected from. Given the similarities in relevance, it > makes sense to enable UMIP along with SMAP and SMEP. > > UMIP is enabled by default. It can be disabled by adding clearcpuid=514 > to the kernel parameters. > > Cc: Andy Lutomirski > Cc: Andrew Morton > Cc: H. Peter Anvin > Cc: Borislav Petkov > Cc: Brian Gerst > Cc: Chen Yucong > Cc: Chris Metcalf > Cc: Dave Hansen > Cc: Fenghua Yu > Cc: Huang Rui > Cc: Jiri Slaby > Cc: Jonathan Corbet > Cc: Michael S. Tsirkin > Cc: Paul Gortmaker > Cc: Peter Zijlstra > Cc: Ravi V. Shankar > Cc: Shuah Khan > Cc: Vlastimil Babka > Cc: Tony Luck > Cc: Paolo Bonzini > Cc: Liang Z. Li > Cc: Alexandre Julliard > Cc: Stas Sergeev > Cc: x...@kernel.org > Cc: linux-msdos@vger.kernel.org > Signed-off-by: Ricardo Neri > --- > arch/x86/Kconfig | 10 ++ > arch/x86/kernel/cpu/common.c | 16 +++- > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 702002b..1b1bbeb 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -1745,6 +1745,16 @@ config X86_SMAP > > If unsure, say Y. > > +config X86_INTEL_UMIP > + def_bool y That's a bit too much. It makes sense on distro kernels but how many machines out there actually have UMIP? > + depends on CPU_SUP_INTEL > + prompt "Intel User Mode Instruction Prevention" if EXPERT > + ---help--- > + The User Mode Instruction Prevention (UMIP) is a security > + feature in newer Intel processors. If enabled, a general > + protection fault is issued if the instructions SGDT, SLDT, > + SIDT, SMSW and STR are executed in user mode. > + > config X86_INTEL_MPX > prompt "Intel MPX (Memory Protection Extensions)" > def_bool n > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > index 8ee3211..66ebded 100644 > --- a/arch/x86/kernel/cpu/common.c > +++ b/arch/x86/kernel/cpu/common.c > @@ -311,6 +311,19 @@ static __always_inline void setup_smap(struct > cpuinfo_x86 *c) > } > } > > +static __always_inline void setup_umip(struct cpuinfo_x86 *c) > +{ > + if (cpu_feature_enabled(X86_FEATURE_UMIP) && > + cpu_has(c, X86_FEATURE_UMIP)) Hmm, so if UMIP is not build-time disabled, the cpu_feature_enabled() will call static_cpu_has(). Looks like you want to call cpu_has() too because alternatives haven't run yet and static_cpu_has() will reply wrong. Please state that in a comment. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- -- To unsubscribe from this list: send the line "unsubscribe linux-msdos" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 23/26] x86/traps: Fixup general protection faults caused by UMIP
On Fri, May 05, 2017 at 11:17:21AM -0700, Ricardo Neri wrote: > If the User-Mode Instruction Prevention CPU feature is available and > enabled, a general protection fault will be issued if the instructions > sgdt, sldt, sidt, str or smsw are executed from user-mode context > (CPL > 0). If the fault was caused by any of the instructions protected > by UMIP, fixup_umip_exception will emulate dummy results for these Please end function names with parentheses. > instructions. If emulation is successful, the result is passed to the > user space program and no SIGSEGV signal is emitted. > > Please note that fixup_umip_exception also caters for the case when > the fault originated while running in virtual-8086 mode. > > Cc: Andy Lutomirski > Cc: Andrew Morton > Cc: H. Peter Anvin > Cc: Borislav Petkov > Cc: Brian Gerst > Cc: Chen Yucong > Cc: Chris Metcalf > Cc: Dave Hansen > Cc: Fenghua Yu > Cc: Huang Rui > Cc: Jiri Slaby > Cc: Jonathan Corbet > Cc: Michael S. Tsirkin > Cc: Paul Gortmaker > Cc: Peter Zijlstra > Cc: Ravi V. Shankar > Cc: Shuah Khan > Cc: Vlastimil Babka > Cc: Tony Luck > Cc: Paolo Bonzini > Cc: Liang Z. Li > Cc: Alexandre Julliard > Cc: Stas Sergeev > Cc: x...@kernel.org > Cc: linux-msdos@vger.kernel.org > Reviewed-by: Andy Lutomirski > Signed-off-by: Ricardo Neri > --- > arch/x86/kernel/traps.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index 3995d3a..cec548d 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -65,6 +65,7 @@ > #include > #include > #include > +#include > > #ifdef CONFIG_X86_64 > #include > @@ -526,6 +527,9 @@ do_general_protection(struct pt_regs *regs, long > error_code) > RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU"); > cond_local_irq_enable(regs); > Almost definitely: if (static_cpu_has(X86_FEATURE_UMIP)) { if (... > + if (user_mode(regs) && fixup_umip_exception(regs)) > + return; We don't want to punish !UMIP machines. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- -- To unsubscribe from this list: send the line "unsubscribe linux-msdos" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 22/26] x86/umip: Force a page fault when unable to copy emulated result to user
On Fri, May 05, 2017 at 11:17:20AM -0700, Ricardo Neri wrote: > fixup_umip_exception() will be called from do_general_protection. If the ^ | Please end function names with parentheses.---+ > former returns false, the latter will issue a SIGSEGV with SEND_SIG_PRIV. > However, when emulation is successful but the emulated result cannot be > copied to user space memory, it is more accurate to issue a SIGSEGV with > SEGV_MAPERR with the offending address. > A new function is inspired in That reads funny. > force_sig_info_fault is introduced to model the page fault. > > Cc: Andy Lutomirski > Cc: Andrew Morton > Cc: H. Peter Anvin > Cc: Borislav Petkov > Cc: Brian Gerst > Cc: Chen Yucong > Cc: Chris Metcalf > Cc: Dave Hansen > Cc: Fenghua Yu > Cc: Huang Rui > Cc: Jiri Slaby > Cc: Jonathan Corbet > Cc: Michael S. Tsirkin > Cc: Paul Gortmaker > Cc: Peter Zijlstra > Cc: Ravi V. Shankar > Cc: Shuah Khan > Cc: Vlastimil Babka > Cc: Tony Luck > Cc: Paolo Bonzini > Cc: Liang Z. Li > Cc: Alexandre Julliard > Cc: Stas Sergeev > Cc: x...@kernel.org > Cc: linux-msdos@vger.kernel.org > Signed-off-by: Ricardo Neri > --- > arch/x86/kernel/umip.c | 45 +++-- > 1 file changed, 43 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c > index c7c5795..ff7366a 100644 > --- a/arch/x86/kernel/umip.c > +++ b/arch/x86/kernel/umip.c > @@ -148,6 +148,41 @@ static int __emulate_umip_insn(struct insn *insn, enum > umip_insn umip_inst, > } > > /** > + * __force_sig_info_umip_fault() - Force a SIGSEGV with SEGV_MAPERR > + * @address: Address that caused the signal > + * @regs:Register set containing the instruction pointer > + * > + * Force a SIGSEGV signal with SEGV_MAPERR as the error code. This function > is > + * intended to be used to provide a segmentation fault when the result of the > + * UMIP emulation could not be copied to the user space memory. > + * > + * Return: none > + */ > +static void __force_sig_info_umip_fault(void __user *address, > + struct pt_regs *regs) > +{ > + siginfo_t info; > + struct task_struct *tsk = current; > + > + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV)) { Save an indentation level: if (!(show_unhandled_signals && unhandled_signal(tsk, SIGSEGV))) return; printk... > + printk_ratelimited("%s[%d] umip emulation segfault ip:%lx > sp:%lx error:%x in %lx\n", > +tsk->comm, task_pid_nr(tsk), regs->ip, > +regs->sp, X86_PF_USER | X86_PF_WRITE, > +regs->ip); > + } > + > + tsk->thread.cr2 = (unsigned long)address; > + tsk->thread.error_code = X86_PF_USER | X86_PF_WRITE; > + tsk->thread.trap_nr = X86_TRAP_PF; > + > + info.si_signo = SIGSEGV; > + info.si_errno = 0; > + info.si_code= SEGV_MAPERR; > + info.si_addr= address; > + force_sig_info(SIGSEGV, &info, tsk); > +} > + > +/** > * fixup_umip_exception() - Fixup #GP faults caused by UMIP > * @regs:Registers as saved when entering the #GP trap > * > @@ -235,8 +270,14 @@ bool fixup_umip_exception(struct pt_regs *regs) > if ((unsigned long)uaddr == -1L) > return false; > nr_copied = copy_to_user(uaddr, dummy_data, dummy_data_size); > - if (nr_copied > 0) > - return false; > + if (nr_copied > 0) { > + /* > + * If copy fails, send a signal and tell caller that > + * fault was fixed up Pls end sentences in the comments with a fullstop. > + */ > + __force_sig_info_umip_fault(uaddr, regs); > + return true; > + } > } > > /* increase IP to let the program keep going */ > -- > 2.9.3 > -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- -- To unsubscribe from this list: send the line "unsubscribe linux-msdos" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html