[PATCH 0/2] uprobes: x86: Reject probing MOV SS

[PATCH 0/2] uprobes: x86: Reject probing MOV SS

[Masami Hiramatsu]

Hi,

I found the CVE-2018-1087 and CVE-2018-8897 should
be related to kprobes and uprobes too, since both
are using #DB for single stepping.

I decided to just reject probes on MOV SS and POP SS
because those are not recommended to use (Intel SDM
recommend to use LSS instead), thus it might be
rare case.

Oleg, could you review the uprobes patch?

Thank you,

---

Masami Hiramatsu (2):
      kprobes: x86: Prohibit probing on exception masking instructions
      uprobes: x86: Prohibit probing on MOV SS instruction


 arch/x86/include/asm/insn.h    |   18 ++++++++++++++++++
 arch/x86/kernel/kprobes/core.c |    4 ++++
 arch/x86/kernel/uprobes.c      |    4 ++++
 3 files changed, 26 insertions(+)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH 1/2] kprobes: x86: Prohibit probing on exception masking instructions

[Masami Hiramatsu]

Since MOV SS and POP SS instructions will delay the exceptions
until the next instruction is executed, we should not do
single-stepping on it by kprobes.

However, kprobes usually executes those instructions directly
on trampoline buffer (a.k.a. kprobe-booster), except for the
kprobes which has post_handler. Thus if kprobe user probes MOV SS
with post_handler, it will do single-stepping on the MOV SS.

This means it is safe that if we use it via ftrace or perf/bpf
since those don't use the post_handler.

Anyway, since the stack switching is a rare case, it is safer
just rejecting kprobes on such instructions.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/include/asm/insn.h    |   18 ++++++++++++++++++
 arch/x86/kernel/kprobes/core.c |    4 ++++
 2 files changed, 22 insertions(+)

diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index b3e32b010ab1..c2c01f84df75 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -208,4 +208,22 @@ static inline int insn_offset_immediate(struct insn *insn)
 	return insn_offset_displacement(insn) + insn->displacement.nbytes;
 }
 
+#define POP_SS_OPCODE 0x1f
+#define MOV_SREG_OPCODE 0x8e
+
+/*
+ * Intel SDM Vol.3A 6.8.3 states;
+ * "Any single-step trap that would be delivered following the MOV to SS
+ * instruction or POP to SS instruction (because EFLAGS.TF is 1) is
+ * suppressed."
+ * This function returns true if @insn is MOV SS or POP SS. On these
+ * instructions, single stepping is suppressed.
+ */
+static inline int insn_masking_exception(struct insn *insn)
+{
+	return insn->opcode.bytes[0] == POP_SS_OPCODE ||
+		(insn->opcode.bytes[0] == MOV_SREG_OPCODE &&
+		 X86_MODRM_REG(insn->modrm.bytes[0]) == 2);
+}
+
 #endif /* _ASM_X86_INSN_H */
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 0715f827607c..6f4d42377fe5 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -370,6 +370,10 @@ int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn)
 	if (insn->opcode.bytes[0] == BREAKPOINT_INSTRUCTION)
 		return 0;
 
+	/* We should not singlestep on the exception masking instructions */
+	if (insn_masking_exception(insn))
+		return 0;
+
 #ifdef CONFIG_X86_64
 	/* Only x86_64 has RIP relative instructions */
 	if (insn_rip_relative(insn)) {

[PATCH 2/2] uprobes: x86: Prohibit probing on MOV SS instruction

[Masami Hiramatsu]

Since MOV SS and POP SS instructions will delay the exceptions
until the next instruction is executed, we should not do
single-stepping on it by uprobes.

uprobe already rejects probing on POP SS (0x1f), but allows
probing on MOV SS (0x8e and reg == 2).
This checks the target instruction and if it is MOV SS
or POP SS, returns -ENOTSUPP to reject probing.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/uprobes.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 85c7ef23d99f..c84bb5396958 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -299,6 +299,10 @@ static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool
 	if (is_prefix_bad(insn))
 		return -ENOTSUPP;
 
+	/* We should not singlestep on the exception masking instructions */
+	if (insn_masking_exception(insn))
+		return -ENOTSUPP;
+
 	if (x86_64)
 		good_insns = good_insns_64;
 	else

[PATCH 0/2] uprobes: kprobes: x86: Reject probing MOV SS/POP SS

[Masami Hiramatsu]

Oops, I missed adding kprobes: and POP SS on subject...
	

On Wed,  9 May 2018 21:57:44 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,
> 
> I found the CVE-2018-1087 and CVE-2018-8897 should
> be related to kprobes and uprobes too, since both
> are using #DB for single stepping.
> 
> I decided to just reject probes on MOV SS and POP SS
> because those are not recommended to use (Intel SDM
> recommend to use LSS instead), thus it might be
> rare case.
>
> Oleg, could you review the uprobes patch?
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (2):
>       kprobes: x86: Prohibit probing on exception masking instructions
>       uprobes: x86: Prohibit probing on MOV SS instruction
> 
> 
>  arch/x86/include/asm/insn.h    |   18 ++++++++++++++++++
>  arch/x86/kernel/kprobes/core.c |    4 ++++
>  arch/x86/kernel/uprobes.c      |    4 ++++
>  3 files changed, 26 insertions(+)
> 
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 0/2] uprobes: x86: Reject probing MOV SS

[Andy Lutomirski]

On Wed, May 9, 2018 at 5:58 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,

> I found the CVE-2018-1087 and CVE-2018-8897 should
> be related to kprobes and uprobes too, since both
> are using #DB for single stepping.


Seems okay to me.  I doubt we have a security problem, but I can easily
imagine that the probes wouldn't work right.

Re: [PATCH 2/2] uprobes: x86: Prohibit probing on MOV SS instruction

[Oleg Nesterov]

On 05/09, Masami Hiramatsu wrote:
>
> Since MOV SS and POP SS instructions will delay the exceptions
> until the next instruction is executed, we should not do
> single-stepping on it by uprobes.
...
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -299,6 +299,10 @@ static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool
>  	if (is_prefix_bad(insn))
>  		return -ENOTSUPP;
>  
> +	/* We should not singlestep on the exception masking instructions */
> +	if (insn_masking_exception(insn))
> +		return -ENOTSUPP;

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH 0/2] uprobes: x86: Reject probing MOV SS

[Masami Hiramatsu]

On Wed, 09 May 2018 14:36:27 +0000
Andy Lutomirski <luto@kernel.org> wrote:

> On Wed, May 9, 2018 at 5:58 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hi,
> 
> > I found the CVE-2018-1087 and CVE-2018-8897 should
> > be related to kprobes and uprobes too, since both
> > are using #DB for single stepping.
> 
> 
> Seems okay to me.  I doubt we have a security problem, but I can easily
> imagine that the probes wouldn't work right.

Sorry for confusion, yes, that will not be a security issue, since kprobes
and uprobes can be used only by root user.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

[tip:x86/urgent] kprobes/x86: Prohibit probing on exception masking instructions

[tip-bot for Masami Hiramatsu]

Commit-ID:  ee6a7354a3629f9b65bc18dbe393503e9440d6f5
Gitweb:     https://git.kernel.org/tip/ee6a7354a3629f9b65bc18dbe393503e9440d6f5
Author:     Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate: Wed, 9 May 2018 21:58:15 +0900
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 13 May 2018 19:52:55 +0200

kprobes/x86: Prohibit probing on exception masking instructions

Since MOV SS and POP SS instructions will delay the exceptions until the
next instruction is executed, single-stepping on it by kprobes must be
prohibited.

However, kprobes usually executes those instructions directly on trampoline
buffer (a.k.a. kprobe-booster), except for the kprobes which has
post_handler. Thus if kprobe user probes MOV SS with post_handler, it will
do single-stepping on the MOV SS.

This means it is safe that if it is used via ftrace or perf/bpf since those
don't use the post_handler.

Anyway, since the stack switching is a rare case, it is safer just
rejecting kprobes on such instructions.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Cc: Francis Deslauriers <francis.deslauriers@efficios.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "David S . Miller" <davem@davemloft.net>
Link: https://lkml.kernel.org/r/152587069574.17316.3311695234863248641.stgit@devbox

---
 arch/x86/include/asm/insn.h    | 18 ++++++++++++++++++
 arch/x86/kernel/kprobes/core.c |  4 ++++
 2 files changed, 22 insertions(+)

diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index b3e32b010ab1..c2c01f84df75 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -208,4 +208,22 @@ static inline int insn_offset_immediate(struct insn *insn)
 	return insn_offset_displacement(insn) + insn->displacement.nbytes;
 }
 
+#define POP_SS_OPCODE 0x1f
+#define MOV_SREG_OPCODE 0x8e
+
+/*
+ * Intel SDM Vol.3A 6.8.3 states;
+ * "Any single-step trap that would be delivered following the MOV to SS
+ * instruction or POP to SS instruction (because EFLAGS.TF is 1) is
+ * suppressed."
+ * This function returns true if @insn is MOV SS or POP SS. On these
+ * instructions, single stepping is suppressed.
+ */
+static inline int insn_masking_exception(struct insn *insn)
+{
+	return insn->opcode.bytes[0] == POP_SS_OPCODE ||
+		(insn->opcode.bytes[0] == MOV_SREG_OPCODE &&
+		 X86_MODRM_REG(insn->modrm.bytes[0]) == 2);
+}
+
 #endif /* _ASM_X86_INSN_H */
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 0715f827607c..6f4d42377fe5 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -370,6 +370,10 @@ int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn)
 	if (insn->opcode.bytes[0] == BREAKPOINT_INSTRUCTION)
 		return 0;
 
+	/* We should not singlestep on the exception masking instructions */
+	if (insn_masking_exception(insn))
+		return 0;
+
 #ifdef CONFIG_X86_64
 	/* Only x86_64 has RIP relative instructions */
 	if (insn_rip_relative(insn)) {

[tip:x86/urgent] uprobes/x86: Prohibit probing on MOV SS instruction

[tip-bot for Masami Hiramatsu]

Commit-ID:  13ebe18c94f5b0665c01ae7fad2717ae959f4212
Gitweb:     https://git.kernel.org/tip/13ebe18c94f5b0665c01ae7fad2717ae959f4212
Author:     Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate: Wed, 9 May 2018 21:58:45 +0900
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 13 May 2018 19:52:56 +0200

uprobes/x86: Prohibit probing on MOV SS instruction

Since MOV SS and POP SS instructions will delay the exceptions until the
next instruction is executed, single-stepping on it by uprobes must be
prohibited.

uprobe already rejects probing on POP SS (0x1f), but allows probing on MOV
SS (0x8e and reg == 2).  This checks the target instruction and if it is
MOV SS or POP SS, returns -ENOTSUPP to reject probing.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Cc: Francis Deslauriers <francis.deslauriers@efficios.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "David S . Miller" <davem@davemloft.net>
Link: https://lkml.kernel.org/r/152587072544.17316.5950935243917346341.stgit@devbox

---
 arch/x86/kernel/uprobes.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 85c7ef23d99f..c84bb5396958 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -299,6 +299,10 @@ static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool
 	if (is_prefix_bad(insn))
 		return -ENOTSUPP;
 
+	/* We should not singlestep on the exception masking instructions */
+	if (insn_masking_exception(insn))
+		return -ENOTSUPP;
+
 	if (x86_64)
 		good_insns = good_insns_64;
 	else