[RESEND PATCH v2 0/4] x86: Tweaks for UMIP

[RESEND PATCH v2 0/4] x86: Tweaks for UMIP

[Ricardo Neri]

[To tip maintainers: This is a resend to copy the Linux kernel mailing
list. No changes in the patches since my original v2 submission.]

Now that the support for UMIP [1], [2] has been merged in the tip tree,
this series add a couple of tweaks.

Ingo asked for two small additions to select UMIP by default when building
and inform of this feature being enabled [3].

Also, Linus suggested to issue a rate-limited warning whenever the any of
the instructions that UMIP protects are used by user space programs [4].
This is useful to give programs a hint on the reason for which they start
seeing an unexpected SIGSEGV signal. Also, it helps to encourage updates
to those programs and avoid using these instructions if possible.

Thanks and BR,
Ricardo

[1]. https://lkml.org/lkml/2017/10/27/699
[2]. https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1523438.html
[3]. https://lkml.org/lkml/2017/11/8/238
[4]. https://lkml.org/lkml/2017/11/8/593

Changes since V1:
* Capitalize all the instructions' mnemonics in both code and patch
  descriptions.
* Correct documentation of umip_pr_warn() to correctly reflect the function
  name.
* Update description of patch #4 to describe the update to the existing
  rate-limited pr_err().

Ricardo Neri (4):
  x86/umip: Select X86_INTEL_UMIP by default
  x86/umip: Inform that UMIP has been enabled
  x86/umip: Identify the str and sldt instructions
  x86/umip: Warn if UMIP-protected instructions are used

 arch/x86/Kconfig             | 10 ++++-
 arch/x86/kernel/cpu/common.c |  2 +
 arch/x86/kernel/umip.c       | 88 +++++++++++++++++++++++++++++++++++++-------
 3 files changed, 85 insertions(+), 15 deletions(-)

-- 
2.7.4

[RESEND PATCH v2 1/4] x86/umip: Select X86_INTEL_UMIP by default

[Ricardo Neri]

UMIP does not incur in a significant performance penalty. Furthermore, it
is triggered only when a small group of instructions are used from user
space programs.

While here, provide more details on the benefits UMIP provides and the
behavior that can expect the few applications that use the instructions
protected by UMIP.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: x86@kernel.org
Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/Kconfig | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f08977d..a524a7a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1805,14 +1805,20 @@ config X86_SMAP
 	  If unsure, say Y.
 
 config X86_INTEL_UMIP
-	def_bool n
+	def_bool y
 	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.
+	  SIDT, SMSW and STR are executed in user mode. These instructions
+	  unnecessarily expose information about the hardware state.
+
+	  The vast majority of applications do not use these instructions.
+	  For the very few that do, software emulation is provided in
+	  specific cases in protected and virtual-8086 modes. Emulated
+	  results are dummy.
 
 config X86_INTEL_MPX
 	prompt "Intel MPX (Memory Protection Extensions)"
-- 
2.7.4

[RESEND PATCH v2 2/4] x86/umip: Inform that UMIP has been enabled

[Ricardo Neri]

Let us have an indication that this feature has been enabled.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: x86@kernel.org
Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/kernel/cpu/common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 13ae9e5..fa998ca 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -341,6 +341,8 @@ static __always_inline void setup_umip(struct cpuinfo_x86 *c)
 
 	cr4_set_bits(X86_CR4_UMIP);
 
+	pr_info("x86/cpu: Activated the Intel User Mode Instruction Prevention (UMIP) CPU feature\n");
+
 	return;
 
 out:
-- 
2.7.4

[RESEND PATCH v2 3/4] x86/umip: Identify the str and sldt instructions

[Ricardo Neri]

The instructions STR and SLDT are not emulated in any case. Thus, it made
sense to not implement functionality to identify them. However, a
subsequent commit will introduce functionality to warn about the use of
all the instructions that UMIP protect, not only those that are emulated.
A first step for that is the ability to identify them.

Plus, now that STR and SLDT are identified, we need to explicitly avoid
their emulation (i.e., not rely on unsuccessful identification). Group
togehter all the cases that we do not want to emulate: STR, SLDT and user
long mode processes.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: x86@kernel.org
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
This patch also corrects the #define of SMSW. This change does not have a
functional impact as it is only used as an identifier.
---
 arch/x86/kernel/umip.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 6ba82be..2e09b5b 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -78,7 +78,9 @@
 
 #define	UMIP_INST_SGDT	0	/* 0F 01 /0 */
 #define	UMIP_INST_SIDT	1	/* 0F 01 /1 */
-#define	UMIP_INST_SMSW	3	/* 0F 01 /4 */
+#define	UMIP_INST_SMSW	2	/* 0F 01 /4 */
+#define	UMIP_INST_SLDT  3       /* 0F 00 /0 */
+#define	UMIP_INST_STR   4       /* 0F 00 /1 */
 
 /**
  * identify_insn() - Identify a UMIP-protected instruction
@@ -118,10 +120,16 @@ static int identify_insn(struct insn *insn)
 		default:
 			return -EINVAL;
 		}
+	} else if (insn->opcode.bytes[1] == 0x0) {
+		if (X86_MODRM_REG(insn->modrm.value) == 0)
+			return UMIP_INST_SLDT;
+		else if (X86_MODRM_REG(insn->modrm.value) == 1)
+			return UMIP_INST_STR;
+		else
+			return -EINVAL;
+	} else {
+		return -EINVAL;
 	}
-
-	/* SLDT AND STR are not emulated */
-	return -EINVAL;
 }
 
 /**
@@ -267,10 +275,6 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (!regs)
 		return false;
 
-	/* Do not emulate 64-bit processes. */
-	if (user_64bit_mode(regs))
-		return false;
-
 	/*
 	 * If not in user-space long mode, a custom code segment could be in
 	 * use. This is true in protected mode (if the process defined a local
@@ -322,6 +326,11 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (umip_inst < 0)
 		return false;
 
+	/* Do not emulate SLDT, STR or user long mode processes. */
+	if (umip_inst == UMIP_INST_STR || umip_inst == UMIP_INST_SLDT ||
+	    user_64bit_mode(regs))
+		return false;
+
 	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size))
 		return false;
 
-- 
2.7.4

[RESEND PATCH v2 4/4] x86/umip: Warn if UMIP-protected instructions are used

[Ricardo Neri]

Issue a rate-limited warning whenever any of the instructions that UMIP
protects (i.e., SGDT, SIDT, SLDT, STR and SMSW) are used by user space
programs.

This is useful because, with UMIP enabled, the few programs that use such
instructions will start receiving a SIGSEGV signal. In the specific cases
for which emulation is provided (instructions SGDT, SIDT and SMSW in
protected and virtual-8086 modes), a warning is also helpful to encourage
updates in such programs to avoid the use of such instructions.

An existing rate-limited pr_err() is converted to use the new function
umip_pr_warn() in order to have it printing at the same rate and log
level.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: x86@kernel.org
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/kernel/umip.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 2e09b5b..50f4b11 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -82,6 +82,54 @@
 #define	UMIP_INST_SLDT  3       /* 0F 00 /0 */
 #define	UMIP_INST_STR   4       /* 0F 00 /1 */
 
+const char * const umip_insns[5] = {
+	[UMIP_INST_SGDT] = "sgdt",
+	[UMIP_INST_SIDT] = "sidt",
+	[UMIP_INST_SMSW] = "smsw",
+	[UMIP_INST_SLDT] = "sldt",
+	[UMIP_INST_STR] = "str",
+};
+
+/*
+ * If you change these strings, ensure that buffers using them are sufficiently
+ * large.
+ */
+static const char umip_warn_use[] = "cannot be used by applications.";
+static const char umip_warn_emu[] = "For now, expensive software emulation returns result.";
+
+/**
+ * umip_pr_warn() - Print a rate-limited warning
+ * @regs:	Register set with the context in which the warning is printed
+ * @msg:	Pointer to a string with the warning message
+ * @error:	Error code to print along with the warning
+ *
+ * Print the message contained in @msg along with the task name, ID number and
+ * instruction and stack pointers of the associated process. Optionally, an
+ * error code is printed if @error is not zero. These warning messages are
+ * limited to a burst of 5 messages every two minutes.
+ *
+ * Returns:
+ *
+ * None.
+ */
+static void umip_pr_warn(struct pt_regs *regs, char *msg, long error)
+{
+	struct task_struct *tsk = current;
+	char err_str[8 + BITS_PER_LONG / 4] = "";
+
+	/* Bursts of 5 messages every two minutes */
+	static DEFINE_RATELIMIT_STATE(ratelimit, 2 * 60 * HZ, 5);
+
+	if (!__ratelimit(&ratelimit))
+		return;
+
+	if (error)
+		snprintf(err_str, sizeof(err_str), " error:%lx", error);
+
+	pr_warn("%s[%d] %s ip:%lx sp:%lx%s\n", tsk->comm, task_pid_nr(tsk), msg,
+		regs->ip, regs->sp, err_str);
+}
+
 /**
  * identify_insn() - Identify a UMIP-protected instruction
  * @insn:	Instruction structure with opcode and ModRM byte.
@@ -236,10 +284,7 @@ static void force_sig_info_umip_fault(void __user *addr, struct pt_regs *regs)
 	if (!(show_unhandled_signals && unhandled_signal(tsk, SIGSEGV)))
 		return;
 
-	pr_err_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);
+	umip_pr_warn(regs, "segfault at", X86_PF_USER | X86_PF_WRITE);
 }
 
 /**
@@ -264,10 +309,10 @@ static void force_sig_info_umip_fault(void __user *addr, struct pt_regs *regs)
 bool fixup_umip_exception(struct pt_regs *regs)
 {
 	int not_copied, nr_copied, reg_offset, dummy_data_size, umip_inst;
+	unsigned char buf[MAX_INSN_SIZE], warn[128];
 	unsigned long seg_base = 0, *reg_addr;
 	/* 10 bytes is the maximum size of the result of UMIP instructions */
 	unsigned char dummy_data[10] = { 0 };
-	unsigned char buf[MAX_INSN_SIZE];
 	void __user *uaddr;
 	struct insn insn;
 	char seg_defs;
@@ -326,10 +371,18 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (umip_inst < 0)
 		return false;
 
+	snprintf(warn, sizeof(warn), "%s %s", umip_insns[umip_inst],
+		 umip_warn_use);
+
 	/* Do not emulate SLDT, STR or user long mode processes. */
 	if (umip_inst == UMIP_INST_STR || umip_inst == UMIP_INST_SLDT ||
-	    user_64bit_mode(regs))
+	    user_64bit_mode(regs)) {
+		umip_pr_warn(regs, warn, 0);
 		return false;
+	}
+
+	snprintf(warn, sizeof(warn), "%s %s", warn, umip_warn_emu);
+	umip_pr_warn(regs, warn, 0);
 
 	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size))
 		return false;
-- 
2.7.4

Re: [RESEND PATCH v2 3/4] x86/umip: Identify the str and sldt instructions

[Ingo Molnar]

* Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote:

> The instructions STR and SLDT are not emulated in any case. Thus, it made
> sense to not implement functionality to identify them. However, a
> subsequent commit will introduce functionality to warn about the use of
> all the instructions that UMIP protect, not only those that are emulated.
> A first step for that is the ability to identify them.
> 
> Plus, now that STR and SLDT are identified, we need to explicitly avoid
> their emulation (i.e., not rely on unsuccessful identification). Group
> togehter all the cases that we do not want to emulate: STR, SLDT and user
> long mode processes.
> 
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
> Cc: x86@kernel.org
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Sigh, the _title_ still refers to 'str'...

I'll fix it up, no need to resend, but this lack of attention to details is 
seriously sad.

Thanks,

	Ingo

Re: [RESEND PATCH v2 4/4] x86/umip: Warn if UMIP-protected instructions are used

[Ingo Molnar]

* Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote:

> +const char * const umip_insns[5] = {
> +	[UMIP_INST_SGDT] = "sgdt",
> +	[UMIP_INST_SIDT] = "sidt",
> +	[UMIP_INST_SMSW] = "smsw",
> +	[UMIP_INST_SLDT] = "sldt",
> +	[UMIP_INST_STR] = "str",
> +};

Sigh ...

> +/*
> + * If you change these strings, ensure that buffers using them are sufficiently
> + * large.
> + */
> +static const char umip_warn_use[] = "cannot be used by applications.";
> +static const char umip_warn_emu[] = "For now, expensive software emulation returns result.";

Please use the string literals directly, don't add an extra obfuscation layer.

Plus:

> +	unsigned char buf[MAX_INSN_SIZE], warn[128];

> +	snprintf(warn, sizeof(warn), "%s %s", umip_insns[umip_inst],
> +		 umip_warn_use);

This is incredibly fragile against future buffer overflows, and warning about it 
in comments does not make it less fragile!

Thanks,

	Ingo

[tip:x86/urgent] x86/umip: Select X86_INTEL_UMIP by default

[tip-bot for Ricardo Neri]

Commit-ID:  796ebc81b9931bfa293b4ca38ae28c21a363f4d0
Gitweb:     https://git.kernel.org/tip/796ebc81b9931bfa293b4ca38ae28c21a363f4d0
Author:     Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
AuthorDate: Mon, 13 Nov 2017 22:29:42 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 14 Nov 2017 08:38:08 +0100

x86/umip: Select X86_INTEL_UMIP by default

UMIP does cause any performance penalty to the vast majority of x86 code
that does not use the legacy instructions affected by UMIP.

Also describe UMIP more accurately and explain the behavior that can be
expected by the (few) applications that use the affected instructions.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: ricardo.neri@intel.com
Link: http://lkml.kernel.org/r/1510640985-18412-2-git-send-email-ricardo.neri-calderon@linux.intel.com
[ Spelling fixes, rewrote the changelog. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/Kconfig | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f08977d..a0623f0 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1805,14 +1805,20 @@ config X86_SMAP
 	  If unsure, say Y.
 
 config X86_INTEL_UMIP
-	def_bool n
+	def_bool y
 	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.
+	  protection fault is issued if the SGDT, SLDT, SIDT, SMSW
+	  or STR instructions are executed in user mode. These instructions
+	  unnecessarily expose information about the hardware state.
+
+	  The vast majority of applications do not use these instructions.
+	  For the very few that do, software emulation is provided in
+	  specific cases in protected and virtual-8086 modes. Emulated
+	  results are dummy.
 
 config X86_INTEL_MPX
 	prompt "Intel MPX (Memory Protection Extensions)"

[tip:x86/urgent] x86/umip: Print a line in the boot log that UMIP has been enabled

[tip-bot for Ricardo Neri]

Commit-ID:  770c77557757873808a474016a3cae4b37690cb2
Gitweb:     https://git.kernel.org/tip/770c77557757873808a474016a3cae4b37690cb2
Author:     Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
AuthorDate: Mon, 13 Nov 2017 22:29:43 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 14 Nov 2017 08:38:09 +0100

x86/umip: Print a line in the boot log that UMIP has been enabled

Indicate that this feature has been enabled.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: ricardo.neri@intel.com
Link: http://lkml.kernel.org/r/1510640985-18412-3-git-send-email-ricardo.neri-calderon@linux.intel.com
[ Changelog tweaks. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 13ae9e5..fa998ca 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -341,6 +341,8 @@ static __always_inline void setup_umip(struct cpuinfo_x86 *c)
 
 	cr4_set_bits(X86_CR4_UMIP);
 
+	pr_info("x86/cpu: Activated the Intel User Mode Instruction Prevention (UMIP) CPU feature\n");
+
 	return;
 
 out:

[tip:x86/urgent] x86/umip: Identify the STR and SLDT instructions

[tip-bot for Ricardo Neri]

Commit-ID:  6e2a3064d6a86094fecc20cd430fd96aaa801687
Gitweb:     https://git.kernel.org/tip/6e2a3064d6a86094fecc20cd430fd96aaa801687
Author:     Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
AuthorDate: Mon, 13 Nov 2017 22:29:44 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 14 Nov 2017 08:38:09 +0100

x86/umip: Identify the STR and SLDT instructions

The STR and SLDT instructions are not emulated by the UMIP code, thus
there's no functionality in the decoder to identify them.

However, a subsequent commit will introduce a warning about the use
of all the instructions that UMIP protect/changes, not only those that
are emulated.

A first step for that is to add the ability to decode/identify them.

Plus, now that STR and SLDT are identified, we need to explicitly avoid
their emulation (i.e., not rely on successful identification). Group
together all the cases that we do not want to emulate: STR, SLDT and user
long mode processes.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: ricardo.neri@intel.com
Link: http://lkml.kernel.org/r/1510640985-18412-4-git-send-email-ricardo.neri-calderon@linux.intel.com
[ Rewrote the changelog, fixed ugly col80 artifact. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/umip.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 6ba82be..1f1f2d5 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -78,7 +78,9 @@
 
 #define	UMIP_INST_SGDT	0	/* 0F 01 /0 */
 #define	UMIP_INST_SIDT	1	/* 0F 01 /1 */
-#define	UMIP_INST_SMSW	3	/* 0F 01 /4 */
+#define	UMIP_INST_SMSW	2	/* 0F 01 /4 */
+#define	UMIP_INST_SLDT  3       /* 0F 00 /0 */
+#define	UMIP_INST_STR   4       /* 0F 00 /1 */
 
 /**
  * identify_insn() - Identify a UMIP-protected instruction
@@ -118,10 +120,16 @@ static int identify_insn(struct insn *insn)
 		default:
 			return -EINVAL;
 		}
+	} else if (insn->opcode.bytes[1] == 0x0) {
+		if (X86_MODRM_REG(insn->modrm.value) == 0)
+			return UMIP_INST_SLDT;
+		else if (X86_MODRM_REG(insn->modrm.value) == 1)
+			return UMIP_INST_STR;
+		else
+			return -EINVAL;
+	} else {
+		return -EINVAL;
 	}
-
-	/* SLDT AND STR are not emulated */
-	return -EINVAL;
 }
 
 /**
@@ -267,10 +275,6 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (!regs)
 		return false;
 
-	/* Do not emulate 64-bit processes. */
-	if (user_64bit_mode(regs))
-		return false;
-
 	/*
 	 * If not in user-space long mode, a custom code segment could be in
 	 * use. This is true in protected mode (if the process defined a local
@@ -322,6 +326,10 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (umip_inst < 0)
 		return false;
 
+	/* Do not emulate SLDT, STR or user long mode processes. */
+	if (umip_inst == UMIP_INST_STR || umip_inst == UMIP_INST_SLDT || user_64bit_mode(regs))
+		return false;
+
 	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size))
 		return false;
 

Re: [RESEND PATCH v2 3/4] x86/umip: Identify the str and sldt instructions

[Ricardo Neri]

On Tue, Nov 14, 2017 at 08:21:57AM +0100, Ingo Molnar wrote:
> 
> * Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote:
> 
> > The instructions STR and SLDT are not emulated in any case. Thus, it made
> > sense to not implement functionality to identify them. However, a
> > subsequent commit will introduce functionality to warn about the use of
> > all the instructions that UMIP protect, not only those that are emulated.
> > A first step for that is the ability to identify them.
> > 
> > Plus, now that STR and SLDT are identified, we need to explicitly avoid
> > their emulation (i.e., not rely on unsuccessful identification). Group
> > togehter all the cases that we do not want to emulate: STR, SLDT and user
> > long mode processes.
> > 
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Ravi V. Shankar <ravi.v.shankar@intel.com>
> > Cc: x86@kernel.org
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> 
> Sigh, the _title_ still refers to 'str'...
> 
> I'll fix it up, no need to resend, but this lack of attention to details is 
> seriously sad.

I apologize for the oversight. I focused on the code and changelog. Thanks for
fixing it!

Thanks and BR,
Ricardo

Re: [RESEND PATCH v2 4/4] x86/umip: Warn if UMIP-protected instructions are used

[Ricardo Neri]

On Tue, Nov 14, 2017 at 08:34:08AM +0100, Ingo Molnar wrote:
> 
> * Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote:
> 
> > +const char * const umip_insns[5] = {
> > +	[UMIP_INST_SGDT] = "sgdt",
> > +	[UMIP_INST_SIDT] = "sidt",
> > +	[UMIP_INST_SMSW] = "smsw",
> > +	[UMIP_INST_SLDT] = "sldt",
> > +	[UMIP_INST_STR] = "str",
> > +};
> 
> Sigh ...

It made sense to me to capitalize code and changelogs but keep the printed warnings
in lower case. Thus, the format would look like:

    umip: smsw instruction cannot be used by applications.

I do see that this could be confusing with STR. I will capitalize these as well.
> 
> > +/*
> > + * If you change these strings, ensure that buffers using them are sufficiently
> > + * large.
> > + */
> > +static const char umip_warn_use[] = "cannot be used by applications.";
> > +static const char umip_warn_emu[] = "For now, expensive software emulation returns result.";
> 
> Please use the string literals directly, don't add an extra obfuscation layer.
> 
> Plus:
> 
> > +	unsigned char buf[MAX_INSN_SIZE], warn[128];
> 
> > +	snprintf(warn, sizeof(warn), "%s %s", umip_insns[umip_inst],
> > +		 umip_warn_use);
> 
> This is incredibly fragile against future buffer overflows, and warning about it 
> in comments does not make it less fragile!

I need to concatenate the instruction mnemonic with the a string. Does something like
this is more acceptable?

	unsigned char warn[50];

	...

	strcpy(warn, umip_insns[umip_inst]);
	strcat(warn, " instruction cannot be used by applications.");
	umip_pr_warn(regs, warn, 0);

In this manner I use the string literal directly but I still have a buffer that might
overflow. Code looks more clear to me. I could #defines for the string lengths or
set a maximum length.

Thanks and BR,
Ricardo

Re: [RESEND PATCH v2 4/4] x86/umip: Warn if UMIP-protected instructions are used

[Ingo Molnar]

* Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote:

> > > +	snprintf(warn, sizeof(warn), "%s %s", umip_insns[umip_inst],
> > > +		 umip_warn_use);
> > 
> > This is incredibly fragile against future buffer overflows, and warning about it 
> > in comments does not make it less fragile!
> 
> I need to concatenate the instruction mnemonic with the a string. Does something like
> this is more acceptable?
> 
> 	unsigned char warn[50];
> 
> 	...
> 
> 	strcpy(warn, umip_insns[umip_inst]);
> 	strcat(warn, " instruction cannot be used by applications.");
> 	umip_pr_warn(regs, warn, 0);
> 
> In this manner I use the string literal directly but I still have a buffer that might
> overflow. Code looks more clear to me. I could #defines for the string lengths or
> set a maximum length.

This is still very fragile.

The right solution would be to make umip_pr_warn() a varargs helper function, so 
that you can just use it to print things the usual way. I'd also use a 
__attribute__((format(printf))) specification to get good build-time warnings.

Thanks,

	Ingo