Re: [v2 5/7] x86: Add emulation code for UMIP instructions

[Andy Lutomirski]

On Thu, Dec 29, 2016 at 9:23 PM, Ricardo Neri
 wrote:
> On Tue, 2016-12-27 at 16:48 -0800, Andy Lutomirski wrote:
>>
>> >> > +   if (nr_copied  > 0)
>> >> > +   return -EFAULT;
>> >>
>> >> This should be the only EFAULT case.
>> > Should this be EFAULT event if the caller cares only about successful
>> > (return 0) vs failed (return non-0) emulation?
>>
>> In theory this particular error would be a page fault not a general
>> protection fault (in the UMIP off case).  If you were emulating it
>> extra carefully, you could change the signal accordingly.  But, as I
>> said, I really doubt this matters.
>
> If simple enough and for the sake of accuracy, I could try to issue the
> page fault. It seems to me that this entitles calling
> force_sig_info_fault in this particular case as opposed to the
> force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk) that do_general_protection
> calls.

Sure.  You could even do it by sending the signal in the emulation
code and returning true.

--Andy
--
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: [v2 5/7] x86: Add emulation code for UMIP instructions

[Ricardo Neri]

On Tue, 2016-12-27 at 16:48 -0800, Andy Lutomirski wrote:
> On Tue, Dec 27, 2016 at 4:39 PM, Ricardo Neri
>  wrote:
> > On Fri, 2016-12-23 at 18:11 -0800, Andy Lutomirski wrote:
> >> On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri
> >>  wrote:
> >> > The feature User-Mode Instruction Prevention present in recent Intel
> >> > processor prevents a group of instructions from being executed with
> >> > CPL > 0. Otherwise, a general protection fault is issued.
> >> >
> >> > Rather than relaying this fault to the user space (in the form of a 
> >> > SIGSEGV
> >> > signal), the instructions protected by UMIP can be emulated to provide
> >> > dummy results. This allows to conserve the current kernel behavior and 
> >> > not
> >> > reveal the system resources that UMIP intends to protect (the global
> >> > descriptor and interrupt descriptor tables, the segment selectors of the
> >> > local descriptor table and the task state and the machine status word).
> >> >
> >> > This emulation is needed because certain applications (e.g., WineHQ) rely
> >> > on this subset of instructions to function.
> >> >
> >> > The instructions protected by UMIP can be split in two groups. Those who
> >> > return a kernel memory address (sgdt and sidt) and those who return a
> >> > value (sldt, str and smsw).
> >> >
> >> > For the instructions that return a kernel memory address, the result is
> >> > emulated as the location of a dummy variable in the kernel memory space.
> >> > This is needed as applications such as WineHQ rely on the result being
> >> > located in the kernel memory space function. The limit for the GDT and 
> >> > the
> >> > IDT are set to zero.
> >>
> >> Nak.  This is a trivial KASLR bypass.  Just give them hardcoded
> >> values.  For x86_64, I would suggest 0xfffe and
> >> 0x.
> >
> > I see. I assume you are suggesting these values for x86_64 because they
> > lie in an unused hole. That makes sense to me.
> >
> > For the case of x86_32, I have trouble finding a suitable place as there
> > are not many available holes. It could be put before VMALLOC_START or
> > after VMALLOC_END but this would reveal the position of the vmalloc
> > area. Although, to my knowledge, randomized memory is not available for
> > x86_32. Without randomization, does it hurt to make sidt/sgdt return the
> > address of a kernel static variable?
> 
> I would just use the same addresses, truncated.  There's no reason
> that the address needs to be truly not present -- it just needs to be
> inaccessible to user code.  Anything near the top of the address space
> should work.

Right. Then I will reuse the same addresses.
> 
> >
> >>
> >> >
> >> > The instructions sldt and str return a segment selector relative to the
> >> > base address of the global descriptor table. Since the actual address of
> >> > such table is not revealed, it makes sense to emulate the result as zero.
> >>
> >> Hmm, now I wonder if anything uses SLDT to see if there is an LDT.  If
> >> so, we could emulate it better, but I doubt this matters.
> >
> > So you are saying that the emulated sldt should return a different value
> > based on the presence/absence of a LDT? This could reveal this very
> > fact.
> 
> User code knows whether the LDT exists because an LDT only exists if
> the program called modify_ldt().  But I doubt this matters in
> practice.

In such a case sldt would return a non-null segment selector. I will
keep giving the null segment selector in all cases and make a note in
the code.

> 
> >> > +static int __emulate_umip_insn(struct insn *insn, enum umip_insn 
> >> > umip_inst,
> >> > +  unsigned char *data, int *data_size)
> >> > +{
> >> > +   unsigned long const *dummy_base_addr;
> >> > +   unsigned short dummy_limit = 0;
> >> > +   unsigned short dummy_value = 0;
> >> > +
> >> > +   switch (umip_inst) {
> >> > +   /*
> >> > +* These two instructions return the base address and limit of 
> >> > the
> >> > +* global and interrupt descriptor table. The base address can be
> >> > +* 32-bit or 64-bit. Limit is always 16-bit.
> >> > +*/
> >> > +   case UMIP_SGDT:
> >> > +   case UMIP_SIDT:
> >> > +   if (umip_inst == UMIP_SGDT)
> >> > +   dummy_base_addr = _dummy_gdt_base;
> >> > +   else
> >> > +   dummy_base_addr = _dummy_idt_base;
> >> > +   if (X86_MODRM_MOD(insn->modrm.value) == 3) {
> >> > +   WARN_ONCE(1, "SGDT cannot take register as 
> >> > argument!\n");
> >>
> >> No warnings please.
> >
> > I'll. Remove it.
> 
> Thanks.  In general, WARN_ONCE, etc are supposed to indicate kernel
> bugs, not user bugs.

Agreed. Your statement makes it very clear. I didn't have it that clear
in my mind.

> 
> >> > +   int not_copied, nr_copied, reg_offset, dummy_data_size;
> >> > +

Re: [v2 5/7] x86: Add emulation code for UMIP instructions

[Ricardo Neri]

On Mon, 2016-12-26 at 00:49 +0900, Masami Hiramatsu wrote:
> On Fri, 23 Dec 2016 17:37:43 -0800
> Ricardo Neri  wrote:
> 
> > +static int __identify_insn(struct insn *insn)
> > +{
> > +   /* by getting modrm we also get the opcode */
> > +   insn_get_modrm(insn);
> > +   if (insn->opcode.bytes[0] != 0xf)
> > +   return -EINVAL;
> > +
> > +   if (insn->opcode.bytes[1] == 0x1) {
> > +   switch (X86_MODRM_REG(insn->modrm.value)) {
> > +   case 0:
> > +   return UMIP_SGDT;
> > +   case 1:
> > +   return UMIP_SIDT;
> > +   case 4:
> > +   return UMIP_SMSW;
> > +   default:
> > +   return -EINVAL;
> > +   }
> > +   } else if (insn->opcode.bytes[1] == 0x0) {
> > +   if (X86_MODRM_REG(insn->modrm.value) == 0)
> > +   return UMIP_SLDT;
> > +   else if (X86_MODRM_REG(insn->modrm.value) == 1)
> > +   return UMIP_STR;
> > +   else
> > +   return -EINVAL;
> > +   }
> 
> gcc detected an error here, you may need return "-EINVAL".

I will make this change. I removed this EINVAL at the last minute as it
didn't look right. It was indeed right.

Thanks and BR,
Ricardo
> 
> Thanks,
> 
> 
> 


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

[v2 5/7] x86: Add emulation code for UMIP instructions

[Ricardo Neri]

The feature User-Mode Instruction Prevention present in recent Intel
processor prevents a group of instructions from being executed with
CPL > 0. Otherwise, a general protection fault is issued.

Rather than relaying this fault to the user space (in the form of a SIGSEGV
signal), the instructions protected by UMIP can be emulated to provide
dummy results. This allows to conserve the current kernel behavior and not
reveal the system resources that UMIP intends to protect (the global
descriptor and interrupt descriptor tables, the segment selectors of the
local descriptor table and the task state and the machine status word).

This emulation is needed because certain applications (e.g., WineHQ) rely
on this subset of instructions to function.

The instructions protected by UMIP can be split in two groups. Those who
return a kernel memory address (sgdt and sidt) and those who return a
value (sldt, str and smsw).

For the instructions that return a kernel memory address, the result is
emulated as the location of a dummy variable in the kernel memory space.
This is needed as applications such as WineHQ rely on the result being
located in the kernel memory space function. The limit for the GDT and the
IDT are set to zero.

The instructions sldt and str return a segment selector relative to the
base address of the global descriptor table. Since the actual address of
such table is not revealed, it makes sense to emulate the result as zero.

The instruction smsw is emulated to return zero.

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/include/asm/umip.h |  16 +
 arch/x86/kernel/Makefile|   1 +
 arch/x86/kernel/umip.c  | 170 
 3 files changed, 187 insertions(+)
 create mode 100644 arch/x86/include/asm/umip.h
 create mode 100644 arch/x86/kernel/umip.c

diff --git a/arch/x86/include/asm/umip.h b/arch/x86/include/asm/umip.h
new file mode 100644
index 000..7bcaca6
--- /dev/null
+++ b/arch/x86/include/asm/umip.h
@@ -0,0 +1,16 @@
+#ifndef _ASM_X86_UMIP_H
+#define _ASM_X86_UMIP_H
+
+#include 
+#include 
+#include 
+
+#ifdef CONFIG_X86_INTEL_UMIP
+int fixup_umip_exception(struct pt_regs *regs);
+#else
+static inline int fixup_umip_exception(struct pt_regs *regs)
+{
+   return -EINVAL;
+}
+#endif  /* CONFIG_X86_INTEL_UMIP */
+#endif  /* _ASM_X86_UMIP_H */
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 581386c..c4aec02 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_EFI)   += sysfb_efi.o
 obj-$(CONFIG_PERF_EVENTS)  += perf_regs.o
 obj-$(CONFIG_TRACING)  += tracepoint.o
 obj-$(CONFIG_SCHED_MC_PRIO)+= itmt.o
+obj-$(CONFIG_X86_INTEL_UMIP)   += umip.o
 
 ifdef CONFIG_FRAME_POINTER
 obj-y  += unwind_frame.o
diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
new file mode 100644
index 000..a104aea
--- /dev/null
+++ b/arch/x86/kernel/umip.c
@@ -0,0 +1,170 @@
+/*
+ * umip.c Emulation for instruction protected by the Intel User-Mode
+ * Instruction Prevention. The instructions are:
+ *sgdt
+ *sldt
+ *sidt
+ *str
+ *smsw
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ * Ricardo Neri 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * The address of this dummy values need to be readable by
+ * the user space
+ */
+
+static const long umip_dummy_gdt_base;
+static const long umip_dummy_idt_base;
+
+enum umip_insn {
+   UMIP_SGDT = 0,  /* opcode 0f 01 ModR/M reg 0 */
+   UMIP_SIDT,  /* opcode 0f 01 ModR/M reg 1 */
+   UMIP_SLDT,  /* opcode 0f 00 ModR/M reg 0 */
+   UMIP_SMSW,  /* opcode 0f 01 ModR/M reg 4 */
+   UMIP_STR,   /* opcode 0f 00 ModR/M reg 1 */
+};
+
+static int __identify_insn(struct insn *insn)
+{
+   /* by getting modrm we also get the opcode */
+