Re: [PATCH v7 24/26] x86: Enable User-Mode Instruction Prevention

[Borislav Petkov]

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

[Borislav Petkov]

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

[Borislav Petkov]

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, , 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