[PATCH 0/4] x86: enable User-Mode Instruction Prevention

[PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Ricardo Neri]

User-Mode Instruction Prevention (UMIP) is a security feature present in
new Intel Processors. If enabled, it prevents the execution of certain
instructions if the Current Privilege Level (CPL) is greater than 0. If
these instructions were executed while in CPL > 0, user space applications
could have access to system-wide settings such as the global and local
descriptor tables, the task register and the interrupt descriptor table.

These are the instructions covered by UMIP:
* SGDT - Store Global Descriptor Table
* SIDT - Store Interrupt Descriptor Table
* SLDT - Store Local Descriptor Table
* SMSW - Store Machine Status Word
* STR - Store Task Register

If any of these instructions is executed with CPL > 0, a general protection
exception is issued when UMIP is enbled.

There is a caveat, however. Certain applications running in virtual-8086
mode, such as DOSEMU[1] and Wine[2], want to utilize the SGDT, SIDT and
SLDT instructions for legitimate reasons. In order to keep such
applications working, UMIP must be disabled/enabled when entering/exiting
virtual-8086 mode. We also disable/enable UMIP in context switch if we
detect that there is a valid virtual-8086 state structure. However,
unconditionally disabling UMIP for virtual-8086 tasks could be exploited
by malicious applications. Hence, disabling UMIP for such kind of tasks is
allowed only if the kernel parameter 'umip=novm86' is used.

Rather than using the more modern clearcpuid=1234 format for the
kernel parameters, we use umip={no|novm86}. This is because the former does
cannot cover the three configuration states of UMIP.

The virtual-8086 mode selftests are updated to ensure that the
aforementioned instructions can be executed without issue in such mode.

Thanks and BR,
Ricardo

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Chen Yucong <slaoub@gmail.com>
Cc: Chris Metcalf <cmetcalf@mellanox.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Huang Rui <ray.huang@amd.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Shuah Khan <shuah@kernel.org>


[1]. http://www.dosemu.org/
[2]. https://wiki.winehq.org/Main_Page

Ricardo Neri (4):
  x86/cpufeature: Add User-Mode Instruction Prevention definitions
  x86: Prepare vm86 tasks to handle User-Mode Instruction Prevention
  x86: Enable User-Mode Instruction Prevention
  selftests/x86: Add tests for User-Mode Instruction Prevention

 Documentation/kernel-parameters.txt           |  5 +++
 arch/x86/Kconfig                              | 10 ++++++
 arch/x86/include/asm/cpufeatures.h            |  1 +
 arch/x86/include/asm/disabled-features.h      |  8 ++++-
 arch/x86/include/asm/vm86.h                   |  3 ++
 arch/x86/include/uapi/asm/processor-flags.h   |  2 ++
 arch/x86/kernel/cpu/common.c                  | 50 ++++++++++++++++++++++++++-
 arch/x86/kernel/process.c                     | 10 ++++++
 arch/x86/kernel/vm86_32.c                     | 20 +++++++++++
 tools/testing/selftests/x86/entry_from_vm86.c | 10 +++++-
 10 files changed, 116 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Ricardo Neri]

User-Mode Instruction Prevention (UMIP) is a security feature present in
new Intel Processors. If enabled, it prevents the execution of certain
instructions if the Current Privilege Level (CPL) is greater than 0. If
these instructions were executed while in CPL > 0, user space applications
could have access to system-wide settings such as the global and local
descriptor tables, the task register and the interrupt descriptor table.

These are the instructions covered by UMIP:
* SGDT - Store Global Descriptor Table
* SIDT - Store Interrupt Descriptor Table
* SLDT - Store Local Descriptor Table
* SMSW - Store Machine Status Word
* STR - Store Task Register

If any of these instructions is executed with CPL > 0, a general protection
exception is issued when UMIP is enbled.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Chen Yucong <slaoub@gmail.com>
Cc: Chris Metcalf <cmetcalf@mellanox.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Huang Rui <ray.huang@amd.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/include/asm/cpufeatures.h          | 1 +
 arch/x86/include/asm/disabled-features.h    | 8 +++++++-
 arch/x86/include/uapi/asm/processor-flags.h | 2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 5f0931b..81ef3bbe 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -282,6 +282,7 @@
 #define X86_FEATURE_AVIC	(15*32+13) /* Virtual Interrupt Controller */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:0 (ecx), word 16 */
+#define X86_FEATURE_UMIP	(16*32+ 2) /* User Mode Instruction Protection */
 #define X86_FEATURE_PKU		(16*32+ 3) /* Protection Keys for Userspace */
 #define X86_FEATURE_OSPKE	(16*32+ 4) /* OS Protection Keys Enable */
 
diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h
index 85599ad..4707445 100644
--- a/arch/x86/include/asm/disabled-features.h
+++ b/arch/x86/include/asm/disabled-features.h
@@ -16,6 +16,12 @@
 # define DISABLE_MPX	(1<<(X86_FEATURE_MPX & 31))
 #endif
 
+#ifdef CONFIG_X86_INTEL_UMIP
+# define DISABLE_UMIP	0
+#else
+# define DISABLE_UMIP	(1<<(X86_FEATURE_UMIP & 31))
+#endif
+
 #ifdef CONFIG_X86_64
 # define DISABLE_VME		(1<<(X86_FEATURE_VME & 31))
 # define DISABLE_K6_MTRR	(1<<(X86_FEATURE_K6_MTRR & 31))
@@ -55,7 +61,7 @@
 #define DISABLED_MASK13	0
 #define DISABLED_MASK14	0
 #define DISABLED_MASK15	0
-#define DISABLED_MASK16	(DISABLE_PKU|DISABLE_OSPKE)
+#define DISABLED_MASK16	(DISABLE_PKU|DISABLE_OSPKE|DISABLE_UMIP)
 #define DISABLED_MASK17	0
 #define DISABLED_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 18)
 
diff --git a/arch/x86/include/uapi/asm/processor-flags.h b/arch/x86/include/uapi/asm/processor-flags.h
index 567de50..d2c2af8 100644
--- a/arch/x86/include/uapi/asm/processor-flags.h
+++ b/arch/x86/include/uapi/asm/processor-flags.h
@@ -104,6 +104,8 @@
 #define X86_CR4_OSFXSR		_BITUL(X86_CR4_OSFXSR_BIT)
 #define X86_CR4_OSXMMEXCPT_BIT	10 /* enable unmasked SSE exceptions */
 #define X86_CR4_OSXMMEXCPT	_BITUL(X86_CR4_OSXMMEXCPT_BIT)
+#define X86_CR4_UMIP_BIT	11 /* enable UMIP support */
+#define X86_CR4_UMIP		_BITUL(X86_CR4_UMIP_BIT)
 #define X86_CR4_VMXE_BIT	13 /* enable VMX virtualization */
 #define X86_CR4_VMXE		_BITUL(X86_CR4_VMXE_BIT)
 #define X86_CR4_SMXE_BIT	14 /* enable safer mode (TXT) */
-- 
2.7.4

[PATCH 2/4] x86: Prepare vm86 tasks to handle User-Mode Instruction Prevention

[Ricardo Neri]

User-Mode Instruction Prevention (UMIP) is a security feature in new Intel
processors that causes a general protection exception if certain
instructions are executed in user mode (CPL > 0).

Unfortunately, some of the instructions that are protected by UMIP (i.e.,
SGDT, SIDT and SMSW) are used by certain applications running in virtual-
8086 mode (e.g., DOSEMU and Wine). Thus, UMIP needs to be disabled in
virtual-8086 tasks for such applications to run correctly. However,
unconditionally disabling UMIP for virtual-8086 tasks could be abused by
malicious applcations. Hence, UMIP can only be disabled for this particular
kind of tasks if requested at boot time via vm86_disable_x86_umip.

If disabling UMIP is allowed, it is done in the following two code paths:
1) entering virtual-8086 mode via a system call, and 2) task switch. When
For task-switching a new member is added to struct vm86 to keep track of
the UMIP disabling selection; set in the vm86 system call as per the the
selection made at boot time.

If supported by the CPU, UMIP is re-enabled as soon as we exit virtual-8086
mode via interrupt/exception or task switch. To determine that we switch to
a virtual-8086 mode task, we rely in the fact that virtual-8086 mode tasks
keep a copy of the value of the supervisor mode stack pointer prior to
entering in virtual-8086 mode.

Since the X86_UMIP config option is not defined yet, this code remains
dormant until such option is enabled in a subsequent patch. Such patch will
also introduce code to disable UMIP for virtual-8086 tasks via a kernel
parameter.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Chen Yucong <slaoub@gmail.com>
Cc: Chris Metcalf <cmetcalf@mellanox.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Huang Rui <ray.huang@amd.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/include/asm/vm86.h |  3 +++
 arch/x86/kernel/process.c   | 10 ++++++++++
 arch/x86/kernel/vm86_32.c   | 20 ++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/arch/x86/include/asm/vm86.h b/arch/x86/include/asm/vm86.h
index 1e491f3..bd14cbc 100644
--- a/arch/x86/include/asm/vm86.h
+++ b/arch/x86/include/asm/vm86.h
@@ -40,6 +40,7 @@ struct vm86 {
 	struct revectored_struct int_revectored;
 	struct revectored_struct int21_revectored;
 	struct vm86plus_info_struct vm86plus;
+	bool disable_x86_umip;
 };
 
 #ifdef CONFIG_VM86
@@ -47,6 +48,7 @@ struct vm86 {
 void handle_vm86_fault(struct kernel_vm86_regs *, long);
 int handle_vm86_trap(struct kernel_vm86_regs *, long, int);
 void save_v86_state(struct kernel_vm86_regs *, int);
+void __init vm86_disable_x86_umip(void);
 
 struct task_struct;
 
@@ -76,6 +78,7 @@ void release_vm86_irqs(struct task_struct *);
 
 #define handle_vm86_fault(a, b)
 #define release_vm86_irqs(a)
+#define vm86_disable_x86_umip()
 
 static inline int handle_vm86_trap(struct kernel_vm86_regs *a, long b, int c)
 {
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 0888a87..32b7301 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -233,6 +233,16 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		 */
 		memset(tss->io_bitmap, 0xff, prev->io_bitmap_max);
 	}
+
+#if defined(CONFIG_VM86) && defined(CONFIG_X86_INTEL_UMIP)
+	if (next->vm86 && next->vm86->saved_sp0 && next->vm86->disable_x86_umip)
+		cr4_clear_bits(X86_CR4_UMIP);
+	else {
+		if (static_cpu_has(X86_FEATURE_UMIP))
+			cr4_set_bits(X86_CR4_UMIP);
+	}
+#endif
+
 	propagate_user_return_notify(prev_p, next_p);
 }
 
diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index 01f30e5..7fd22e7 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -90,6 +90,14 @@
 #define SAFE_MASK	(0xDD5)
 #define RETURN_MASK	(0xDFF)
 
+static bool disable_x86_umip;
+
+void __init vm86_disable_x86_umip(void)
+{
+	if (cpu_feature_enabled(X86_FEATURE_UMIP))
+		disable_x86_umip = true;
+}
+
 void save_v86_state(struct kernel_vm86_regs *regs, int retval)
 {
 	struct tss_struct *tss;
@@ -156,6 +164,12 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
 	lazy_load_gs(vm86->regs32.gs);
 
 	regs->pt.ax = retval;
+
+
+	/* Always enable UMIP if supported */
+	if (cpu_feature_enabled(X86_FEATURE_UMIP) &&
+	    static_cpu_has(X86_FEATURE_UMIP))
+		cr4_set_bits(X86_CR4_UMIP);
 }
 
 static void mark_screen_rdonly(struct mm_struct *mm)
@@ -371,6 +385,12 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
 	if (vm86->flags & VM86_SCREEN_BITMAP)
 		mark_screen_rdonly(tsk->mm);
 
+	if (cpu_feature_enabled(X86_FEATURE_UMIP)) {
+		vm86->disable_x86_umip = disable_x86_umip;
+		if (disable_x86_umip)
+			cr4_clear_bits(X86_CR4_UMIP);
+	}
+
 	memcpy((struct kernel_vm86_regs *)regs, &vm86regs, sizeof(vm86regs));
 	force_iret();
 	return regs->ax;
-- 
2.7.4

[PATCH 3/4] x86: Enable User-Mode Instruction Prevention

[Ricardo Neri]

User_mode Instruction Prevention (UMIP) is enabled by setting/clearing a
bit in %cr4.

It make 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.

Also, a __setup function is added to configure the enablement of UMIP with
kernel parameters. UMIP can be disabled completely or only for virtual-8086
tasks. We may want to disable UMIP for virtual-8086 tasks as there are
legitimate applications that utilize instructions disallowed by UMIP.
However, unconditionally disabling UMIP for virtual-8086 could be exploited
by malicious applications. Hence, we let the system owner to allow virtual-
8086 tasks to disable UMIP via a kernel parameter.

Rather than using the clearcpuid=1234 format for our kernel parameters,
this implementations relies on the format umip={no|novm86}. The intention
is to either disable UMIP completely or only for virtual-8086 tasks. The
format clearcpuid=1234 cannot take care of disabling UMIP for virtual-8086
tasks.

UMIP is enabled for all tasks by default; including virtual-8086 tasks.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Chen Yucong <slaoub@gmail.com>
Cc: Chris Metcalf <cmetcalf@mellanox.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Huang Rui <ray.huang@amd.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 Documentation/kernel-parameters.txt |  5 ++++
 arch/x86/Kconfig                    | 10 ++++++++
 arch/x86/kernel/cpu/common.c        | 50 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index fd5c052..95d0917 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -4201,6 +4201,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			Note that genuine overcurrent events won't be
 			reported either.
 
+	umip=		[X86] Configure User-Mode Instruction Prevention
+			no = Disable UMIP even if it is supported by processor
+			novm86 = Disable UMIP only for virtual-8086 tasks; UMIP
+				 remains active for all other tasks.
+
 	unknown_nmi_panic
 			[X86] Cause panic on unknown NMI.
 
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 770fb5f..dad93f9 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1718,6 +1718,16 @@ config X86_SMAP
 
 	  If unsure, say Y.
 
+config X86_INTEL_UMIP
+	def_bool y
+	depends on CPU_SUP_INTEL
+	prompt "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 f3e7ab2..8f0e86c 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -44,6 +44,7 @@
 #include <asm/pat.h>
 #include <asm/microcode.h>
 #include <asm/microcode_intel.h>
+#include <asm/vm86.h>
 
 #ifdef CONFIG_X86_LOCAL_APIC
 #include <asm/uv/uv.h>
@@ -306,6 +307,52 @@ static __always_inline void setup_smap(struct cpuinfo_x86 *c)
 	}
 }
 
+static __init int setup_config_umip(char *arg)
+{
+	char info[] = "x86/umip: Intel User-Mode Execution Prevention (UMIP) disabled";
+	char error[] = "x86/umip: invalid kernel parameter. Valid parameters: umip=no";
+
+	/* do not emit a message if the feature is not present */
+	if (!boot_cpu_has(X86_FEATURE_UMIP))
+		return 1;
+
+	/* do not emit a message if the feature is not enabled */
+	if (!cpu_feature_enabled(X86_FEATURE_UMIP))
+		return 1;
+
+	if (parse_option_str(arg, "no")) {
+		setup_clear_cpu_cap(X86_FEATURE_UMIP);
+		pr_info("%s\n", info);
+		return 1;
+	}
+
+	if (IS_ENABLED(CONFIG_VM86) && parse_option_str(arg, "novm86")) {
+		vm86_disable_x86_umip();
+		pr_info("%s for vm86\n", info);
+		return 1;
+	}
+
+	if (IS_ENABLED(CONFIG_VM86))
+		pr_warn("%s, umip=novm86\n", error);
+	else
+		pr_warn("%s\n", error);
+	return 0;
+}
+__setup("umip=", setup_config_umip);
+
+static __always_inline void setup_umip(struct cpuinfo_x86 *c)
+{
+	if (cpu_feature_enabled(X86_FEATURE_UMIP) &&
+	    cpu_has(c, X86_FEATURE_UMIP))
+		cr4_set_bits(X86_CR4_UMIP);
+	else
+		/*
+		 * Make sure UMIP is disabled in case it was enabled in a
+		 * previous boot (e.g., via kexec).
+		 */
+		cr4_clear_bits(X86_CR4_UMIP);
+}
+
 /*
  * Protection Keys are not available in 32-bit mode.
  */
@@ -1037,9 +1084,10 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 	/* Disable the PN if appropriate */
 	squash_the_stupid_serial_number(c);
 
-	/* Set up SMEP/SMAP */
+	/* Set up SMEP/SMAP/UMIP */
 	setup_smep(c);
 	setup_smap(c);
+	setup_umip(c);
 
 	/*
 	 * The vendor-specific functions might have changed features.
-- 
2.7.4

[PATCH 4/4] selftests/x86: Add tests for User-Mode Instruction Prevention

[Ricardo Neri]

Certain user space programs that run on virtual-8086 mode may utilize
instructions protected by the User-Mode Instruction Prevention (UMIP)
security feature present in new Intel processors: SGDT, SIDT and SMSW. In
such a case, a general protection exception is issued if UMIP is enabled.

For the aforementioned reason, UMIP can be optionally disabled for virtual-
8086 tasks with the umip=novm86 kernel parameter. The purpose of this new
test is to verify whether the impacted instructions can be executed without
causing such #GP. If no #GP exceptions occur, we expect to exit virtual-
8086 mode from INT 0x80; provided umip=novm86 was used in the kernel
parameter.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Chen Yucong <slaoub@gmail.com>
Cc: Chris Metcalf <cmetcalf@mellanox.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Huang Rui <ray.huang@amd.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 tools/testing/selftests/x86/entry_from_vm86.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/entry_from_vm86.c b/tools/testing/selftests/x86/entry_from_vm86.c
index d075ea0..6efa6eb 100644
--- a/tools/testing/selftests/x86/entry_from_vm86.c
+++ b/tools/testing/selftests/x86/entry_from_vm86.c
@@ -95,6 +95,11 @@ asm (
 	"int3\n\t"
 	"vmcode_int80:\n\t"
 	"int $0x80\n\t"
+	"umip:\n\t"
+	"sgdt (2052)\n\t"
+	"sidt (2052)\n\t"
+	"smsw (2052)\n\t"
+	"int $0x80\n\t"
 	".size vmcode, . - vmcode\n\t"
 	"end_vmcode:\n\t"
 	".code32\n\t"
@@ -103,7 +108,7 @@ asm (
 
 extern unsigned char vmcode[], end_vmcode[];
 extern unsigned char vmcode_bound[], vmcode_sysenter[], vmcode_syscall[],
-	vmcode_sti[], vmcode_int3[], vmcode_int80[];
+	vmcode_sti[], vmcode_int3[], vmcode_int80[], umip[];
 
 /* Returns false if the test was skipped. */
 static bool do_test(struct vm86plus_struct *v86, unsigned long eip,
@@ -218,6 +223,9 @@ int main(void)
 	v86.regs.eax = (unsigned int)-1;
 	do_test(&v86, vmcode_int80 - vmcode, VM86_INTx, 0x80, "int80");
 
+	/* UMIP -- should exit with INTx 0x80 unless UMIP was not disabled */
+	do_test(&v86, umip - vmcode, VM86_INTx, 0x80, "UMIP tests");
+
 	/* Execute a null pointer */
 	v86.regs.cs = 0;
 	v86.regs.ss = 0;
-- 
2.7.4

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Peter Zijlstra]

On Mon, Nov 07, 2016 at 10:12:09PM -0800, Ricardo Neri wrote:
> There is a caveat, however. Certain applications running in virtual-8086
> mode, such as DOSEMU[1] and Wine[2], want to utilize the SGDT, SIDT and
> SLDT instructions for legitimate reasons. In order to keep such
> applications working, UMIP must be disabled/enabled when entering/exiting
> virtual-8086 mode. 

Would it not be better to emulate these instructions for them? What way
we can verify they're not malicious.

Re: [PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Andy Lutomirski]

On Mon, Nov 7, 2016 at 10:12 PM, Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
> User-Mode Instruction Prevention (UMIP) is a security feature present in
> new Intel Processors. If enabled, it prevents the execution of certain
> instructions if the Current Privilege Level (CPL) is greater than 0. If
> these instructions were executed while in CPL > 0, user space applications
> could have access to system-wide settings such as the global and local
> descriptor tables, the task register and the interrupt descriptor table.
>
> These are the instructions covered by UMIP:
> * SGDT - Store Global Descriptor Table
> * SIDT - Store Interrupt Descriptor Table
> * SLDT - Store Local Descriptor Table
> * SMSW - Store Machine Status Word
> * STR - Store Task Register
>
> If any of these instructions is executed with CPL > 0, a general protection
> exception is issued when UMIP is enbled.
>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Brian Gerst <brgerst@gmail.com>
> Cc: Chen Yucong <slaoub@gmail.com>
> Cc: Chris Metcalf <cmetcalf@mellanox.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Huang Rui <ray.huang@amd.com>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
>  arch/x86/include/asm/cpufeatures.h          | 1 +
>  arch/x86/include/asm/disabled-features.h    | 8 +++++++-
>  arch/x86/include/uapi/asm/processor-flags.h | 2 ++
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 5f0931b..81ef3bbe 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -282,6 +282,7 @@
>  #define X86_FEATURE_AVIC       (15*32+13) /* Virtual Interrupt Controller */
>
>  /* Intel-defined CPU features, CPUID level 0x00000007:0 (ecx), word 16 */
> +#define X86_FEATURE_UMIP       (16*32+ 2) /* User Mode Instruction Protection */
>  #define X86_FEATURE_PKU                (16*32+ 3) /* Protection Keys for Userspace */
>  #define X86_FEATURE_OSPKE      (16*32+ 4) /* OS Protection Keys Enable */
>
> diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h
> index 85599ad..4707445 100644
> --- a/arch/x86/include/asm/disabled-features.h
> +++ b/arch/x86/include/asm/disabled-features.h
> @@ -16,6 +16,12 @@
>  # define DISABLE_MPX   (1<<(X86_FEATURE_MPX & 31))
>  #endif
>
> +#ifdef CONFIG_X86_INTEL_UMIP

^^^^^

What's this?

Let's try to do this with a minimum of configuration.

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Andy Lutomirski]

On Tue, Nov 8, 2016 at 5:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Nov 07, 2016 at 10:12:09PM -0800, Ricardo Neri wrote:
>> There is a caveat, however. Certain applications running in virtual-8086
>> mode, such as DOSEMU[1] and Wine[2], want to utilize the SGDT, SIDT and
>> SLDT instructions for legitimate reasons. In order to keep such
>> applications working, UMIP must be disabled/enabled when entering/exiting
>> virtual-8086 mode.
>
> Would it not be better to emulate these instructions for them? What way
> we can verify they're not malicious.

Forget malice -- if they are really needed for some silly vm86-using
program, let's trap them and emulate them so they return dummy values.

Also, keep in mind that vm86 is already effectively gated behind a
sysctl for non-root.  I think the default should be that, if root has
enabled vm86, it should work.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 2/4] x86: Prepare vm86 tasks to handle User-Mode Instruction Prevention

[Andy Lutomirski]

On Mon, Nov 7, 2016 at 10:12 PM, Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
> User-Mode Instruction Prevention (UMIP) is a security feature in new Intel
> processors that causes a general protection exception if certain
> instructions are executed in user mode (CPL > 0).
>
> Unfortunately, some of the instructions that are protected by UMIP (i.e.,
> SGDT, SIDT and SMSW) are used by certain applications running in virtual-
> 8086 mode (e.g., DOSEMU and Wine). Thus, UMIP needs to be disabled in
> virtual-8086 tasks for such applications to run correctly. However,
> unconditionally disabling UMIP for virtual-8086 tasks could be abused by
> malicious applcations. Hence, UMIP can only be disabled for this particular
> kind of tasks if requested at boot time via vm86_disable_x86_umip.
>
> If disabling UMIP is allowed, it is done in the following two code paths:
> 1) entering virtual-8086 mode via a system call, and 2) task switch. When
> For task-switching a new member is added to struct vm86 to keep track of
> the UMIP disabling selection; set in the vm86 system call as per the the
> selection made at boot time.
>
> If supported by the CPU, UMIP is re-enabled as soon as we exit virtual-8086
> mode via interrupt/exception or task switch. To determine that we switch to
> a virtual-8086 mode task, we rely in the fact that virtual-8086 mode tasks
> keep a copy of the value of the supervisor mode stack pointer prior to
> entering in virtual-8086 mode.
>
> Since the X86_UMIP config option is not defined yet, this code remains
> dormant until such option is enabled in a subsequent patch. Such patch will
> also introduce code to disable UMIP for virtual-8086 tasks via a kernel
> parameter.
>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Brian Gerst <brgerst@gmail.com>
> Cc: Chen Yucong <slaoub@gmail.com>
> Cc: Chris Metcalf <cmetcalf@mellanox.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Huang Rui <ray.huang@amd.com>
> Cc: Jiri Slaby <jslaby@suse.cz>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
>  arch/x86/include/asm/vm86.h |  3 +++
>  arch/x86/kernel/process.c   | 10 ++++++++++
>  arch/x86/kernel/vm86_32.c   | 20 ++++++++++++++++++++
>  3 files changed, 33 insertions(+)
>
> diff --git a/arch/x86/include/asm/vm86.h b/arch/x86/include/asm/vm86.h
> index 1e491f3..bd14cbc 100644
> --- a/arch/x86/include/asm/vm86.h
> +++ b/arch/x86/include/asm/vm86.h
> @@ -40,6 +40,7 @@ struct vm86 {
>         struct revectored_struct int_revectored;
>         struct revectored_struct int21_revectored;
>         struct vm86plus_info_struct vm86plus;
> +       bool disable_x86_umip;
>  };
>
>  #ifdef CONFIG_VM86
> @@ -47,6 +48,7 @@ struct vm86 {
>  void handle_vm86_fault(struct kernel_vm86_regs *, long);
>  int handle_vm86_trap(struct kernel_vm86_regs *, long, int);
>  void save_v86_state(struct kernel_vm86_regs *, int);
> +void __init vm86_disable_x86_umip(void);
>
>  struct task_struct;
>
> @@ -76,6 +78,7 @@ void release_vm86_irqs(struct task_struct *);
>
>  #define handle_vm86_fault(a, b)
>  #define release_vm86_irqs(a)
> +#define vm86_disable_x86_umip()
>
>  static inline int handle_vm86_trap(struct kernel_vm86_regs *a, long b, int c)
>  {
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 0888a87..32b7301 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -233,6 +233,16 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
>                  */
>                 memset(tss->io_bitmap, 0xff, prev->io_bitmap_max);
>         }
> +
> +#if defined(CONFIG_VM86) && defined(CONFIG_X86_INTEL_UMIP)
> +       if (next->vm86 && next->vm86->saved_sp0 && next->vm86->disable_x86_umip)
> +               cr4_clear_bits(X86_CR4_UMIP);
> +       else {
> +               if (static_cpu_has(X86_FEATURE_UMIP))
> +                       cr4_set_bits(X86_CR4_UMIP);
> +       }
> +#endif
> +

NAK.  If this code is going to exist, it needs to be deeply buried in
some unlikely if statement that already exists.  There's no good
reason to penalize all context switches to support some nonsensical
vm86 use case.

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Thomas Gleixner]

On Tue, 8 Nov 2016, Andy Lutomirski wrote:
> On Tue, Nov 8, 2016 at 5:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, Nov 07, 2016 at 10:12:09PM -0800, Ricardo Neri wrote:
> >> There is a caveat, however. Certain applications running in virtual-8086
> >> mode, such as DOSEMU[1] and Wine[2], want to utilize the SGDT, SIDT and
> >> SLDT instructions for legitimate reasons. In order to keep such
> >> applications working, UMIP must be disabled/enabled when entering/exiting
> >> virtual-8086 mode.
> >
> > Would it not be better to emulate these instructions for them? What way
> > we can verify they're not malicious.
> 
> Forget malice -- if they are really needed for some silly vm86-using
> program, let's trap them and emulate them so they return dummy values.

handle_vm86_fault() already does instruction emulation, so adding the few
bits there is the right thing to do. Then we just can enable UMIP
unconditionally and be done with it.

Thanks,

	tglx

Re: [PATCH 2/4] x86: Prepare vm86 tasks to handle User-Mode Instruction Prevention

[Peter Zijlstra]

On Tue, Nov 08, 2016 at 08:01:39AM -0800, Andy Lutomirski wrote:
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index 0888a87..32b7301 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -233,6 +233,16 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
> >                  */
> >                 memset(tss->io_bitmap, 0xff, prev->io_bitmap_max);
> >         }
> > +
> > +#if defined(CONFIG_VM86) && defined(CONFIG_X86_INTEL_UMIP)
> > +       if (next->vm86 && next->vm86->saved_sp0 && next->vm86->disable_x86_umip)
> > +               cr4_clear_bits(X86_CR4_UMIP);
> > +       else {
> > +               if (static_cpu_has(X86_FEATURE_UMIP))
> > +                       cr4_set_bits(X86_CR4_UMIP);
> > +       }
> > +#endif
> > +
> 
> NAK.  If this code is going to exist, it needs to be deeply buried in
> some unlikely if statement that already exists.  There's no good
> reason to penalize all context switches to support some nonsensical
> vm86 use case.

Agreed, now if instead vm86 get to emulate these instructions, this all
magically goes away..

Re: [PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Ricardo Neri]

On Tue, 2016-11-08 at 07:32 -0800, Andy Lutomirski wrote:
> > diff --git a/arch/x86/include/asm/disabled-features.h
> b/arch/x86/include/asm/disabled-features.h
> > index 85599ad..4707445 100644
> > --- a/arch/x86/include/asm/disabled-features.h
> > +++ b/arch/x86/include/asm/disabled-features.h
> > @@ -16,6 +16,12 @@
> >  # define DISABLE_MPX   (1<<(X86_FEATURE_MPX & 31))
> >  #endif
> >
> > +#ifdef CONFIG_X86_INTEL_UMIP
> 
> ^^^^^
> 
> What's this?
> 
> Let's try to do this with a minimum of configuration.

My intention here is put in this file all the #if build configurations
so that I don't have to put them other files by using functions such as
cpu_feature_enable. Isn't this the intention of this file?

Thanks and BR,
Ricardo

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Ricardo Neri]

On Tue, 2016-11-08 at 17:52 +0100, Thomas Gleixner wrote:
> On Tue, 8 Nov 2016, Andy Lutomirski wrote:
> > On Tue, Nov 8, 2016 at 5:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > On Mon, Nov 07, 2016 at 10:12:09PM -0800, Ricardo Neri wrote:
> > >> There is a caveat, however. Certain applications running in virtual-8086
> > >> mode, such as DOSEMU[1] and Wine[2], want to utilize the SGDT, SIDT and
> > >> SLDT instructions for legitimate reasons. In order to keep such
> > >> applications working, UMIP must be disabled/enabled when entering/exiting
> > >> virtual-8086 mode.
> > >
> > > Would it not be better to emulate these instructions for them? What way
> > > we can verify they're not malicious.
> > 
> > Forget malice -- if they are really needed for some silly vm86-using
> > program, let's trap them and emulate them so they return dummy values.
> 
> handle_vm86_fault() already does instruction emulation, so adding the few
> bits there is the right thing to do. Then we just can enable UMIP
> unconditionally and be done with it.

Ah. I didn't think about that. It make sense to me. I will rework this
series with this approach.
> 
> Thanks,
> 
> 	tglx

Re: [PATCH 2/4] x86: Prepare vm86 tasks to handle User-Mode Instruction Prevention

[Ricardo Neri]

On Tue, 2016-11-08 at 18:00 +0100, Peter Zijlstra wrote:
> > > +       }
> > > +#endif
> > > +
> > 
> > NAK.  If this code is going to exist, it needs to be deeply buried
> in
> > some unlikely if statement that already exists.  There's no good
> > reason to penalize all context switches to support some nonsensical
> > vm86 use case.
> 
> Agreed, now if instead vm86 get to emulate these instructions, this
> all
> magically goes away..

Yes. I agree. I will rework the series and this should not be needed.

Thanks and BR,
Ricardo

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Ricardo Neri]

On Tue, 2016-11-08 at 07:34 -0800, Andy Lutomirski wrote:
> > Would it not be better to emulate these instructions for them? What
> way
> > we can verify they're not malicious.
> 
> Forget malice -- if they are really needed for some silly vm86-using
> program, let's trap them and emulate them so they return dummy values.
> 
> Also, keep in mind that vm86 is already effectively gated behind a
> sysctl for non-root.  I think the default should be that, if root has
> enabled vm86, it should work.

Then should I keep UMIP enabled by default and still provide an option
to disable it via a kernel parameter?

Also, a third option, umip=novm86 would "disable" UMIP in vm86 tasks.
Under the new approach (of emulating the impacted instructions), this
option, a #GP fault would still be generated but the actual values of
GDT/LDT/IDT/MSW would be passed to user space. Does this make sense?

Thanks and BR,
Ricardo

Re: [PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Andy Lutomirski]

On Tue, Nov 8, 2016 at 8:25 PM, Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
> On Tue, 2016-11-08 at 07:32 -0800, Andy Lutomirski wrote:
>> > diff --git a/arch/x86/include/asm/disabled-features.h
>> b/arch/x86/include/asm/disabled-features.h
>> > index 85599ad..4707445 100644
>> > --- a/arch/x86/include/asm/disabled-features.h
>> > +++ b/arch/x86/include/asm/disabled-features.h
>> > @@ -16,6 +16,12 @@
>> >  # define DISABLE_MPX   (1<<(X86_FEATURE_MPX & 31))
>> >  #endif
>> >
>> > +#ifdef CONFIG_X86_INTEL_UMIP
>>
>> ^^^^^
>>
>> What's this?
>>
>> Let's try to do this with a minimum of configuration.
>
> My intention here is put in this file all the #if build configurations
> so that I don't have to put them other files by using functions such as
> cpu_feature_enable. Isn't this the intention of this file?

What I mean is: why does this need a config option at all?

--Andy

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Andy Lutomirski]

On Tue, Nov 8, 2016 at 8:31 PM, Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
> On Tue, 2016-11-08 at 07:34 -0800, Andy Lutomirski wrote:
>> > Would it not be better to emulate these instructions for them? What
>> way
>> > we can verify they're not malicious.
>>
>> Forget malice -- if they are really needed for some silly vm86-using
>> program, let's trap them and emulate them so they return dummy values.
>>
>> Also, keep in mind that vm86 is already effectively gated behind a
>> sysctl for non-root.  I think the default should be that, if root has
>> enabled vm86, it should work.
>
> Then should I keep UMIP enabled by default and still provide an option
> to disable it via a kernel parameter?

Probably, but clearcpuid might be good enough.  There might be some
unexpected breakage.

>
> Also, a third option, umip=novm86 would "disable" UMIP in vm86 tasks.
> Under the new approach (of emulating the impacted instructions), this
> option, a #GP fault would still be generated but the actual values of
> GDT/LDT/IDT/MSW would be passed to user space. Does this make sense?

I don't think so.  As far as I know, there is no legitimate reason for
a vm86-using program to care about what these instructions spit out.
Heck, in real mode and vm86 mode, there aren't segment descriptors at
all, so the GDT is really quite useless even if it were readable.

I would suggest having all of these instructions return compile-time
constants in vm86 mode.

Re: [PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Ricardo Neri]

On Wed, 2016-11-09 at 03:02 -0800, Andy Lutomirski wrote:
> On Tue, Nov 8, 2016 at 8:25 PM, Ricardo Neri
> <ricardo.neri-calderon@linux.intel.com> wrote:
> > On Tue, 2016-11-08 at 07:32 -0800, Andy Lutomirski wrote:
> >> > diff --git a/arch/x86/include/asm/disabled-features.h
> >> b/arch/x86/include/asm/disabled-features.h
> >> > index 85599ad..4707445 100644
> >> > --- a/arch/x86/include/asm/disabled-features.h
> >> > +++ b/arch/x86/include/asm/disabled-features.h
> >> > @@ -16,6 +16,12 @@
> >> >  # define DISABLE_MPX   (1<<(X86_FEATURE_MPX & 31))
> >> >  #endif
> >> >
> >> > +#ifdef CONFIG_X86_INTEL_UMIP
> >>
> >> ^^^^^
> >>
> >> What's this?
> >>
> >> Let's try to do this with a minimum of configuration.
> >
> > My intention here is put in this file all the #if build configurations
> > so that I don't have to put them other files by using functions such as
> > cpu_feature_enable. Isn't this the intention of this file?
> 
> What I mean is: why does this need a config option at all?

I intended this feature to be configurable at build time in case someone
wants to build a kernel without it; similar to other features such as
SMAP. Is this not needed? Should Linux be built with this feature always
enabled?

This feature could always be disabled via a kernel parameter, though;
even if Linux is built with it.

Thanks and BR,
Ricardo
> 
> --Andy

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Ricardo Neri]

On Wed, 2016-11-09 at 03:05 -0800, Andy Lutomirski wrote:
> On Tue, Nov 8, 2016 at 8:31 PM, Ricardo Neri
> <ricardo.neri-calderon@linux.intel.com> wrote:
> > On Tue, 2016-11-08 at 07:34 -0800, Andy Lutomirski wrote:
> >> > Would it not be better to emulate these instructions for them? What
> >> way
> >> > we can verify they're not malicious.
> >>
> >> Forget malice -- if they are really needed for some silly vm86-using
> >> program, let's trap them and emulate them so they return dummy values.
> >>
> >> Also, keep in mind that vm86 is already effectively gated behind a
> >> sysctl for non-root.  I think the default should be that, if root has
> >> enabled vm86, it should work.
> >
> > Then should I keep UMIP enabled by default and still provide an option
> > to disable it via a kernel parameter?
> 
> Probably, but clearcpuid might be good enough.  There might be some
> unexpected breakage.
> 
> >
> > Also, a third option, umip=novm86 would "disable" UMIP in vm86 tasks.
> > Under the new approach (of emulating the impacted instructions), this
> > option, a #GP fault would still be generated but the actual values of
> > GDT/LDT/IDT/MSW would be passed to user space. Does this make sense?
> 
> I don't think so.  As far as I know, there is no legitimate reason for
> a vm86-using program to care about what these instructions spit out.
> Heck, in real mode and vm86 mode, there aren't segment descriptors at
> all, so the GDT is really quite useless even if it were readable.

I took a closer look at the dosemu code. It appears that it does not
purposely utilize SGDT to obtain the descriptor table while in vm86. It
does use SGDT (in protected mode) to emulate certain functionality such
as the Virtual xxx Driver. In such a case, UMIP needs to be disabled.
However, this code seems to be disabled [1]. dosemu includes an i386
emulator that in some cases uses the actual instructions of the host
system. In such cases, UMIP might be needed to be disabled.

So, yes, I agree now that UMIP does not need to be disabled specifically
for vm86 tasks but via clearcpuid.

Thanks and BR,
Ricardo
[1].
https://sourceforge.net/p/dosemu/code/ci/master/tree/src/dosext/dpmi/vxd.c#l731 
> 
> I would suggest having all of these instructions return compile-time
> constants in vm86 mode.

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Stas Sergeev]

Hi!

I don't know the context of that discussion, so I'll only
comment on the dosemu part.

10.11.2016 09:46, Ricardo Neri пишет:
> I took a closer look at the dosemu code. It appears that it does not
> purposely utilize SGDT to obtain the descriptor table while in vm86. It
> does use SGDT (in protected mode) to emulate certain functionality such
> as the Virtual xxx Driver. In such a case, UMIP needs to be disabled.
> However, this code seems to be disabled [1].
Indeed.
The code you've found, was copied from wine, because
dosemu supports windows-3.1. But sgdt is in win32s part
that is disabled in dosemu. It is however enabled in wine, or
at least it was when I ported the VxD code from there. So you
may want to ask wine devs if they still use sgdt and vm86.
In dosemu, if we ever enable win32s support, we won't rely
on sgdt. In fact, when some prot mode program under dosemu
uses GDT selectors, in a fault handler we replace them with
LDT selectors.

>   dosemu includes an i386
> emulator that in some cases uses the actual instructions of the host
> system.
In dosemu2 code, the places you've found, now contain this:
error("SGDT not implemented\n");
If we ever support SGDT, we'll use some emulation/fake values.

So overall, dosemu is not going to willingly use sgdt in any
near future. But the programs running under vm86 or in prot mode
may do so. This is very uncommon though, especially under dosemu,
because it supports only a "polite" programs - those that work
under win95's dos prompt. No one would get sufficiently hurt if
sgdt under vm86 will somehow change from its current behaviour.

You can ask wine people for their sgdt use in win32s subsystem.

Re: [PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Borislav Petkov]

On Wed, Nov 09, 2016 at 07:24:43PM -0800, Ricardo Neri wrote:
> I intended this feature to be configurable at build time in case someone
> wants to build a kernel without it; similar to other features such as
> SMAP. Is this not needed? Should Linux be built with this feature always
> enabled?
> 
> This feature could always be disabled via a kernel parameter, though;
> even if Linux is built with it.

It probably is a good idea to have it build-time configurable for the
embedded folks. But you can do a before and after build and look at
the vmlinux size and see how much it has grown. If it is only a couple
of KBs I guess we can drop the config option even but I know there are
people who still care about KBs too...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Dave Hansen]

On 11/09/2016 07:24 PM, Ricardo Neri wrote:
> On Wed, 2016-11-09 at 03:02 -0800, Andy Lutomirski wrote:
...
>> > What I mean is: why does this need a config option at all?
> I intended this feature to be configurable at build time in case someone
> wants to build a kernel without it; similar to other features such as
> SMAP. Is this not needed? Should Linux be built with this feature always
> enabled?

I think marking these features with their own CONFIG's is a really good
idea.  It helps the tinification effort.  It's also nice for folks that
might want to turn all the Intel features off because they're running on
AMD or something.

We don't necessarily need prompts for *everything*, but I can't imagine
just slapping the code in without #ifdefs of any kind.

Re: [PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Ricardo Neri]

On Thu, 2016-11-10 at 09:58 +0100, Borislav Petkov wrote:
> On Wed, Nov 09, 2016 at 07:24:43PM -0800, Ricardo Neri wrote:
> > I intended this feature to be configurable at build time in case someone
> > wants to build a kernel without it; similar to other features such as
> > SMAP. Is this not needed? Should Linux be built with this feature always
> > enabled?
> > 
> > This feature could always be disabled via a kernel parameter, though;
> > even if Linux is built with it.
> 
> It probably is a good idea to have it build-time configurable for the
> embedded folks. But you can do a before and after build and look at
> the vmlinux size and see how much it has grown. If it is only a couple
> of KBs I guess we can drop the config option even but I know there are
> people who still care about KBs too...

Thanks for the suggestions. Perhaps I can include these metrics in my
V2. On th other hand, Dave Hansen gave a good argument on potential
conflicts when, of instance running on an AMD CPU. UMIP is enabled by
setting a bit in CR4. If that bit is not supposed to be set, that could
cause a #GP fault.

Thanks and BR,
Ricardo
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Ricardo Neri]

On Thu, 2016-11-10 at 11:52 +0300, Stas Sergeev wrote:
> Hi!
> 
> I don't know the context of that discussion, so I'll only
> comment on the dosemu part.

I'm sorry! I will cc you and the linux-msdos list in my v2.
> 
> 10.11.2016 09:46, Ricardo Neri пишет:
> > I took a closer look at the dosemu code. It appears that it does not
> > purposely utilize SGDT to obtain the descriptor table while in vm86. It
> > does use SGDT (in protected mode) to emulate certain functionality such
> > as the Virtual xxx Driver. In such a case, UMIP needs to be disabled.
> > However, this code seems to be disabled [1].
> Indeed.
> The code you've found, was copied from wine, because
> dosemu supports windows-3.1. But sgdt is in win32s part
> that is disabled in dosemu. It is however enabled in wine, or
> at least it was when I ported the VxD code from there. So you
> may want to ask wine devs if they still use sgdt and vm86.
> In dosemu, if we ever enable win32s support, we won't rely
> on sgdt. In fact, when some prot mode program under dosemu
> uses GDT selectors, in a fault handler we replace them with
> LDT selectors.

Actually, the SLDT instruction is also impacted by this feature. This
feature, will cause a GP fault if the instructions SGDT, SLDT, SIDT,
SMSW or STR are executed with CPL > 0. Would this be a problem for
dosemu? The proposal now is to trap this GPU fault and give fake value
for these tables.
> 
> >   dosemu includes an i386
> > emulator that in some cases uses the actual instructions of the host
> > system.
> In dosemu2 code, the places you've found, now contain this:
> error("SGDT not implemented\n");
> If we ever support SGDT, we'll use some emulation/fake values.
> 
> So overall, dosemu is not going to willingly use sgdt in any
> near future. But the programs running under vm86 or in prot mode
> may do so. This is very uncommon though, especially under dosemu,
> because it supports only a "polite" programs - those that work
> under win95's dos prompt. No one would get sufficiently hurt if
> sgdt under vm86 will somehow change from its current behaviour.

This is good news. This means that we could go ahead and give a fake
pointer to the GDT and the other impacted tables?
> 
> You can ask wine people for their sgdt use in win32s subsystem.

Will do.

Thanks and BR,
Ricardo

Re: [PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Borislav Petkov]

On Thu, Nov 10, 2016 at 08:08:07PM -0800, Ricardo Neri wrote:
> UMIP is enabled by setting a bit in CR4. If that bit is not supposed
> to be set, that could cause a #GP fault.

Yeah, you do check CPUID first, AFAICT, so you should be ok...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Dave Hansen]

On 11/10/2016 08:08 PM, Ricardo Neri wrote:
> Thanks for the suggestions. Perhaps I can include these metrics in my
> V2. On th other hand, Dave Hansen gave a good argument on potential
> conflicts when, of instance running on an AMD CPU. UMIP is enabled by
> setting a bit in CR4. If that bit is not supposed to be set, that could
> cause a #GP fault.

I just meant that some folks probably appreciate being able to build out
all the Intel-specific features.  Not that it causes a functional problem.

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Stas Sergeev]

11.11.2016 07:14, Ricardo Neri пишет:
>> 10.11.2016 09:46, Ricardo Neri пишет:
>>> I took a closer look at the dosemu code. It appears that it does not
>>> purposely utilize SGDT to obtain the descriptor table while in vm86. It
>>> does use SGDT (in protected mode) to emulate certain functionality such
>>> as the Virtual xxx Driver. In such a case, UMIP needs to be disabled.
>>> However, this code seems to be disabled [1].
>> Indeed.
>> The code you've found, was copied from wine, because
>> dosemu supports windows-3.1. But sgdt is in win32s part
>> that is disabled in dosemu. It is however enabled in wine, or
>> at least it was when I ported the VxD code from there. So you
>> may want to ask wine devs if they still use sgdt and vm86.
>> In dosemu, if we ever enable win32s support, we won't rely
>> on sgdt. In fact, when some prot mode program under dosemu
>> uses GDT selectors, in a fault handler we replace them with
>> LDT selectors.
> Actually, the SLDT instruction is also impacted by this feature. This
We do not support programs that do SLDT.
The "polite" programs use special DPMI API extension to get
the selector that covers LDT. That allows us to manage an "ldt
alias" - memory buffer where we emulate LDT by write-protecting it.
If we ever support SLDT, we would very much like to trap it
and provide the pointer to our alias. Some very old dos extenders
for 286 may start to work with such change, that are currently
unsupported.

> feature, will cause a GP fault if the instructions SGDT, SLDT, SIDT,
> SMSW or STR are executed with CPL > 0. Would this be a problem for
> dosemu?
I am only a bit unsure about SMSW; the rest should be safe.
Maybe some odd prog would use SMSW to check for FPU?
Or to check for v86 mode by checking the PE bit?
I am sure this is very uncommon, and if we find such prog, we
can add an emulation of that instruction. I am pretty sure no one
would get sufficiently hurt, but there will likely be 1-2 bug reports
in our tracker, because if something is possible, then some DOS
prog did that. :)

>   The proposal now is to trap this GPU fault and give fake value
> for these tables.
If this fake value will be cooked up by the kernel without delivering
the signal to dosemu process, then I don't see any problem at all.
Of course you can provide the sane value for smsw.
If that will go up to dosemu, then some coding may be needed
on the user-space side.

> This is good news. This means that we could go ahead and give a fake
> pointer to the GDT and the other impacted tables?
Definitely.
What these fake tables will look like, btw?
Will they somehow resemble the real ones?
Visible to user-space?

Re: [PATCH 1/4] x86/cpufeature: Add User-Mode Instruction Prevention definitions

[Ricardo Neri]

On Fri, 2016-11-11 at 11:22 +0100, Borislav Petkov wrote:
> On Thu, Nov 10, 2016 at 08:08:07PM -0800, Ricardo Neri wrote:
> > UMIP is enabled by setting a bit in CR4. If that bit is not supposed
> > to be set, that could cause a #GP fault.
> 
> Yeah, you do check CPUID first, AFAICT, so you should be ok...

Right. I missed this detail. Yes, there should not be a problem.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Ricardo Neri]

On Fri, 2016-11-11 at 23:51 +0300, Stas Sergeev wrote:
> 11.11.2016 07:14, Ricardo Neri пишет:
> >> 10.11.2016 09:46, Ricardo Neri пишет:
> >>> I took a closer look at the dosemu code. It appears that it does not
> >>> purposely utilize SGDT to obtain the descriptor table while in vm86. It
> >>> does use SGDT (in protected mode) to emulate certain functionality such
> >>> as the Virtual xxx Driver. In such a case, UMIP needs to be disabled.
> >>> However, this code seems to be disabled [1].
> >> Indeed.
> >> The code you've found, was copied from wine, because
> >> dosemu supports windows-3.1. But sgdt is in win32s part
> >> that is disabled in dosemu. It is however enabled in wine, or
> >> at least it was when I ported the VxD code from there. So you
> >> may want to ask wine devs if they still use sgdt and vm86.
> >> In dosemu, if we ever enable win32s support, we won't rely
> >> on sgdt. In fact, when some prot mode program under dosemu
> >> uses GDT selectors, in a fault handler we replace them with
> >> LDT selectors.
> > Actually, the SLDT instruction is also impacted by this feature. This
> We do not support programs that do SLDT.
> The "polite" programs use special DPMI API extension to get
> the selector that covers LDT. That allows us to manage an "ldt
> alias" - memory buffer where we emulate LDT by write-protecting it.
> If we ever support SLDT, we would very much like to trap it
> and provide the pointer to our alias. Some very old dos extenders
> for 286 may start to work with such change, that are currently
> unsupported.

I see.
> 
> > feature, will cause a GP fault if the instructions SGDT, SLDT, SIDT,
> > SMSW or STR are executed with CPL > 0. Would this be a problem for
> > dosemu?
> I am only a bit unsure about SMSW; the rest should be safe.
> Maybe some odd prog would use SMSW to check for FPU?
> Or to check for v86 mode by checking the PE bit?
> I am sure this is very uncommon, and if we find such prog, we
> can add an emulation of that instruction. I am pretty sure no one
> would get sufficiently hurt, but there will likely be 1-2 bug reports
> in our tracker, because if something is possible, then some DOS
> prog did that. :)

Fair enough.
> 
> >   The proposal now is to trap this GPU fault and give fake value
> > for these tables.
> If this fake value will be cooked up by the kernel without delivering
> the signal to dosemu process, then I don't see any problem at all.

Yes, the GP fault will be trapped in the kernel and not delivered to the
user space. All the user space will see is the fake value given by the
kernel.
> Of course you can provide the sane value for smsw.
> If that will go up to dosemu, then some coding may be needed
> on the user-space side.
> 
> > This is good news. This means that we could go ahead and give a fake
> > pointer to the GDT and the other impacted tables?
> Definitely.
> What these fake tables will look like, btw?
> Will they somehow resemble the real ones?
> Visible to user-space?
Since the intention is to hide these tables from the user space, I was
planning on giving 0x0 to all of it.

Thanks and BR,
Ricardo

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[One Thousand Gnomes]

> I took a closer look at the dosemu code. It appears that it does not

That doesn't tell you want DOS itself will try and do...

> purposely utilize SGDT to obtain the descriptor table while in vm86. It
> does use SGDT (in protected mode) to emulate certain functionality such
> as the Virtual xxx Driver. In such a case, UMIP needs to be disabled.
> However, this code seems to be disabled [1]. dosemu includes an i386
> emulator that in some cases uses the actual instructions of the host
> system. In such cases, UMIP might be needed to be disabled.

Is anyone actually still using DOSemu these days or are people all
using DOSbox ?

Alan

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[Harald Arnesen]

Den 14/11/2016 11:59, skrev One Thousand Gnomes:

> Is anyone actually still using DOSemu these days or are people all
> using DOSbox ?
>
> Alan

One thing lacking from DOSbox is TCP/IP networking.
-- 
Hilsen Harald