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

2017-07-25 Thread Ricardo Neri 
On Fri, 2017-06-09 at 18:10 +0200, Borislav Petkov wrote:
> 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?

So would this become a y when more machines 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.

Why would static_cpu_has() reply wrong if alternatives are not in place?
Because it uses the boot CPU data? When it calls _static_cpu_has() it
would do something equivalent to

   testb test_bit, boot_cpu_data.x86_capability[bit].

I am calling cpu_has because cpu_feature_enabled(), via
static_cpu_has(), will use the boot CPU data while cpu_has would use the
local CPU data. Is this what you meant?

I can definitely add a comment with this explanation, if it makes sense.

Thanks and BR,
Ricardo

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

2017-07-25 Thread Ricardo Neri 
I am sorry Boris, I also missed this feedback.

On Fri, 2017-06-09 at 15:02 +0200, Borislav Petkov wrote:
> 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.

I have audited my commit messages to remove all instances of this error.
> 
> > 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 (...

I will make this update.

> 
> > +   if (user_mode(regs) && fixup_umip_exception(regs))
> > +   return;
> 
> We don't want to punish !UMIP machines.

I will add this check.

Thanks and BR,
Ricardo

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

2017-07-25 Thread Ricardo Neri 
On Fri, 2017-06-09 at 13:02 +0200, Borislav Petkov wrote:
> 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.

I will correct this.
> 
> > 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...
> 
I will implement like this.
> 
> 
> > +   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.

I will correct this.

Thanks and BR,
Ricardo

--
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 16/26] x86/insn-eval: Support both signed 32-bit and 64-bit effective addresses

2017-07-25 Thread Ricardo Neri 
I am sorry Boris, while working on this series I missed a few of your
feedback comments.

On Wed, 2017-06-07 at 17:48 +0200, Borislav Petkov wrote:
> On Fri, May 05, 2017 at 11:17:14AM -0700, Ricardo Neri wrote:
> > The 32-bit and 64-bit address encodings are identical. This means that we
> > can use the same function in both cases. In order to reuse the function
> > for 32-bit address encodings, we must sign-extend our 32-bit signed
> > operands to 64-bit signed variables (only for 64-bit builds). To decide on
> > whether sign extension is needed, we rely on the address size as given by
> > the instruction structure.
> > 
> > Once the effective address has been computed, a special verification is
> > needed for 32-bit processes. If running on a 64-bit kernel, such processes
> > can address up to 4GB of memory. Hence, for instance, an effective
> > address of 0x1234 would be misinterpreted as 0x1234 due to
> > the sign extension mentioned above. For this reason, the 4 must be
> 
> Which 4?

I meant to say the 4 most significant bytes. In this case, the
64-address 0x1234 would lie in the kernel memory while
0x1234 would correctly be in the user space memory.
> 
> > truncated to obtain the true effective address.
> > 
> > Lastly, before computing the linear address, we verify that the effective
> > address is within the limits of the segment. The check is kept for long
> > mode because in such a case the limit is set to -1L. This is the largest
> > unsigned number possible. This is equivalent to a limit-less segment.
> > 
> > Cc: Dave Hansen 
> > Cc: Adam Buchbinder 
> > Cc: Colin Ian King 
> > Cc: Lorenzo Stoakes 
> > Cc: Qiaowei Ren 
> > Cc: Arnaldo Carvalho de Melo 
> > Cc: Masami Hiramatsu 
> > Cc: Adrian Hunter 
> > Cc: Kees Cook 
> > Cc: Thomas Garnier 
> > Cc: Peter Zijlstra 
> > Cc: Borislav Petkov 
> > Cc: Dmitry Vyukov 
> > Cc: Ravi V. Shankar 
> > Cc: x...@kernel.org
> > Signed-off-by: Ricardo Neri 
> > ---
> >  arch/x86/lib/insn-eval.c | 99 
> > ++--
> >  1 file changed, 88 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> > index 1a5f5a6..c7c1239 100644
> > --- a/arch/x86/lib/insn-eval.c
> > +++ b/arch/x86/lib/insn-eval.c
> > @@ -688,6 +688,62 @@ int insn_get_modrm_rm_off(struct insn *insn, struct 
> > pt_regs *regs)
> > return get_reg_offset(insn, regs, REG_TYPE_RM);
> >  }
> >  
> > +/**
> > + * _to_signed_long() - Cast an unsigned long into signed long
> > + * @valA 32-bit or 64-bit unsigned long
> > + * @long_bytes The number of bytes used to represent a long number
> > + * @outThe casted signed long
> > + *
> > + * Return: A signed long of either 32 or 64 bits, as per the build 
> > configuration
> > + * of the kernel.
> > + */
> > +static int _to_signed_long(unsigned long val, int long_bytes, long *out)
> > +{
> > +   if (!out)
> > +   return -EINVAL;
> > +
> > +#ifdef CONFIG_X86_64
> > +   if (long_bytes == 4) {
> > +   /* higher bytes should all be zero */
> > +   if (val & ~0x)
> > +   return -EINVAL;
> > +
> > +   /* sign-extend to a 64-bit long */
> 
> So this is a 32-bit userspace on a 64-bit kernel, right?

Yes.
> 
> If so, how can a memory offset be > 32-bits and we have to extend it to
> a 64-bit long?!?

Yes, perhaps the check above is not needed. I included that check as
part of my argument validation. In a 64-bit kernel, this function could
be called with val with non-zero most significant bytes.
> 
> I *think* you want to say that you want to convert it to long so that
> you can do the calculation in longs.

That is exactly what I meant. More specifically, I want to convert my
32-bit variables into 64-bit signed longs; this is the reason I need the
sign extension.
> 
> However!
> 
> If you're a 64-bit kernel running a 32-bit userspace, you need to do
> the calculation in 32-bits only so that it overflows, as it would do
> on 32-bit hardware. IOW, the clamping to 32-bits at the end is not
> something you wanna do but actually let it wrap if it overflows.

I have looked into this closely and as far as I can see, the 4 least
significant bytes will wrap around when using 64-bit signed numbers as
they would when using 32-bit signed numbers. For instance, for two
positive numbers we have:

7fff: + 7000: = efff:.

The addition above overflows. When sign-extended to 64-bit numbers we
would have:

::7fff: + ::7000: = ::efff:.

The addition above does not overflow. However,