[PATCH 0/4] x86: Tweaks for UMIP

[PATCH 0/4] x86: Tweaks for UMIP

[Ricardo Neri]

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

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

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

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

[PATCH 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..6a6fede 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

[PATCH 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.

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 6a6fede..39e1992 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_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: [PATCH 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.

Did you notice how in all your previous patches (both in the code and in the 
changelogs) I have manually fixed up the capitalization of these instruction 
mnenonics?

The capitalized form is much more readable, especially with seriously overloaded 
acronyms such as 'str' ...

You now repeat the same bad pattern, in fact you regress existing code:

> -	/* SLDT AND STR are not emulated */

> +	/* Do not emulate sldt, str or user long mode processes. */

Please be more careful with such details, and please fix & resend this series.

Thanks,

	Ingo

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

[Ricardo Neri]

On Mon, Nov 13, 2017 at 09:12:03AM +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.
> 
> Did you notice how in all your previous patches (both in the code and in the 
> changelogs) I have manually fixed up the capitalization of these instruction 
> mnenonics?

I am sorry, I tried to see where you made these changes but I could not find
any. I did a git diff of arch/x86/kernel/umip.c between the branch rneri/umip_v11
of my repository [1] and the master branch of the tip tree and I did not find
any differences.

Also, looking at the log of the master branch of the tip tree I see, for
instance:

commit 1e5db223696afa55e6a038fac638f759e1fdcc01
Author: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Date:   Sun Nov 5 18:27:52 2017 -0800

    x86/umip: Add emulation code for UMIP instructions
    
    The feature User-Mode Instruction Prevention present in recent Intel
    processor prevents a group of instructions (sgdt, sidt, sldt, smsw, and
    str) from being executed with CPL > 0. Otherwise, a general protection
    fault is issued.
    ...

The instruction mnemonics were not capitalized. Is the master branch the one where
I can look at your fixes?

> 
> The capitalized form is much more readable, especially with seriously overloaded 
> acronyms such as 'str' ...

I see.
> 
> You now repeat the same bad pattern, in fact you regress existing code:
> 
> > -	/* SLDT AND STR are not emulated */
> 
> > +	/* Do not emulate sldt, str or user long mode processes. */
> 
> Please be more careful with such details, and please fix & resend this series.

Sure, I will submit a v2 with capitalized mnemonics in both the code and the
patch descriptions. I will be more careful in the future.

Thanks and BR,
Ricardo

[1]. https://github.com/ricardon/tip/commits/rneri/umip_v11

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

[Ingo Molnar]

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

> On Mon, Nov 13, 2017 at 09:12:03AM +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.
> > 
> > Did you notice how in all your previous patches (both in the code and in the 
> > changelogs) I have manually fixed up the capitalization of these instruction 
> > mnenonics?
> 
> I am sorry, I tried to see where you made these changes but I could not find
> any. I did a git diff of arch/x86/kernel/umip.c between the branch rneri/umip_v11
> of my repository [1] and the master branch of the tip tree and I did not find
> any differences.

For example, I turned:

  [PATCH v11 12/12] selftests/x86: Add tests for instruction str and sldt

  The instructions str and sldt are not valid when running on virtual-8086
  mode and generate an invalid operand exception.
  ...

into:

  a9e017d5619e: selftests/x86: Add tests for the STR and SLDT instructions

  The STR and SLDT instructions are not valid when running on virtual-8086
  mode and generate an invalid operand exception.
  ...

I did not catch every case though.

Thanks,

	Ingo