[PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Masami Hiramatsu]

Hi,

These patches allow x86 instruction decoder to decode
xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit
kprobes to probe on it.

Josh reported that the objtool can not decode such special
prefixed instructions, and I found that we also have to
prohibit kprobes to probe on such instruction.

This series can be applied on -tip master branch which
has merged Josh's objtool/perf sharing common x86 insn
decoder series.


Thank you,

---

Masami Hiramatsu (2):
      x86: xen: insn: Decode XEN_EMULATE_PREFIX correctly
      x86: kprobes: Prohibit probing on instruction which has Xen prefix


 arch/x86/include/asm/insn.h             |    2 +
 arch/x86/include/asm/xen/interface.h    |    7 ++++-
 arch/x86/include/asm/xen/prefix.h       |   10 +++++++
 arch/x86/kernel/kprobes/core.c          |    4 +++
 arch/x86/lib/insn.c                     |   43 +++++++++++++++++++++++++++++++
 tools/arch/x86/include/asm/insn.h       |    2 +
 tools/arch/x86/include/asm/xen/prefix.h |   10 +++++++
 tools/arch/x86/lib/insn.c               |   43 +++++++++++++++++++++++++++++++
 tools/objtool/sync-check.sh             |    3 +-
 9 files changed, 121 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/include/asm/xen/prefix.h
 create mode 100644 tools/arch/x86/include/asm/xen/prefix.h

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

[PATCH -tip 1/2] x86: xen: insn: Decode XEN_EMULATE_PREFIX correctly

[Masami Hiramatsu]

Decode XEN_EMULATE_PREFIX prefix by x86 insn decoder.
This treats a special sequence of instructions of XEN_EMULATE_PREFIX
as a prefix bytes in x86 insn decoder. User can test whether the
instruction has the XEN_EMULATE_PREFIX or not by insn_is_xen_prefixed().
Note that this prefix is treated as just a dummy prefix, so no affect
for decoding opcode and operand.

Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/include/asm/insn.h             |    2 +
 arch/x86/include/asm/xen/interface.h    |    7 ++++-
 arch/x86/include/asm/xen/prefix.h       |   10 +++++++
 arch/x86/lib/insn.c                     |   43 +++++++++++++++++++++++++++++++
 tools/arch/x86/include/asm/insn.h       |    2 +
 tools/arch/x86/include/asm/xen/prefix.h |   10 +++++++
 tools/arch/x86/lib/insn.c               |   43 +++++++++++++++++++++++++++++++
 tools/objtool/sync-check.sh             |    3 +-
 8 files changed, 117 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/include/asm/xen/prefix.h
 create mode 100644 tools/arch/x86/include/asm/xen/prefix.h

diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 154f27be8bfc..ac42efd4295a 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -128,6 +128,8 @@ static inline int insn_is_evex(struct insn *insn)
 	return (insn->vex_prefix.nbytes == 4);
 }
 
+extern int insn_has_xen_prefix(struct insn *insn);
+
 /* Ensure this instruction is decoded completely */
 static inline int insn_complete(struct insn *insn)
 {
diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
index 62ca03ef5c65..fe33a9798708 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -379,12 +379,15 @@ struct xen_pmu_arch {
  * Prefix forces emulation of some non-trapping instructions.
  * Currently only CPUID.
  */
+#include <asm/xen/prefix.h>
+
 #ifdef __ASSEMBLY__
-#define XEN_EMULATE_PREFIX .byte 0x0f,0x0b,0x78,0x65,0x6e ;
+#define XEN_EMULATE_PREFIX .byte __XEN_EMULATE_PREFIX ;
 #define XEN_CPUID          XEN_EMULATE_PREFIX cpuid
 #else
-#define XEN_EMULATE_PREFIX ".byte 0x0f,0x0b,0x78,0x65,0x6e ; "
+#define XEN_EMULATE_PREFIX ".byte " __XEN_EMULATE_PREFIX_STR " ; "
 #define XEN_CPUID          XEN_EMULATE_PREFIX "cpuid"
+
 #endif
 
 #endif /* _ASM_X86_XEN_INTERFACE_H */
diff --git a/arch/x86/include/asm/xen/prefix.h b/arch/x86/include/asm/xen/prefix.h
new file mode 100644
index 000000000000..f901be0d7a95
--- /dev/null
+++ b/arch/x86/include/asm/xen/prefix.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _TOOLS_ASM_X86_XEN_PREFIX_H
+#define _TOOLS_ASM_X86_XEN_PREFIX_H
+
+#include <linux/stringify.h>
+
+#define __XEN_EMULATE_PREFIX  0x0f,0x0b,0x78,0x65,0x6e
+#define __XEN_EMULATE_PREFIX_STR  __stringify(__XEN_EMULATE_PREFIX)
+
+#endif
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 0b5862ba6a75..310a609ee382 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -13,6 +13,9 @@
 #include <asm/inat.h>
 #include <asm/insn.h>
 
+/* For special Xen prefix */
+#include <asm/xen/prefix.h>
+
 /* Verify next sizeof(t) bytes can be on the same instruction */
 #define validate_next(t, insn, n)	\
 	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
@@ -58,6 +61,42 @@ void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64)
 		insn->addr_bytes = 4;
 }
 
+static const insn_byte_t xen_prefix[] = { __XEN_EMULATE_PREFIX };
+
+static int insn_xen_prefix(struct insn *insn, insn_byte_t b)
+{
+	struct insn_field *prefixes = &insn->prefixes;
+	size_t i;
+
+	for (i = 0; i < sizeof(xen_prefix); i++) {
+		b = peek_nbyte_next(insn_byte_t, insn, i);
+		if (b != xen_prefix[i])
+			goto err_out;
+	}
+
+	memcpy(prefixes->bytes, xen_prefix, 3);
+	prefixes->bytes[3] = xen_prefix[sizeof(xen_prefix) - 1];
+	prefixes->nbytes = sizeof(xen_prefix);
+	insn->next_byte += prefixes->nbytes;
+	prefixes->got = 1;
+
+	return 1;
+
+err_out:
+	return 0;
+}
+
+int insn_has_xen_prefix(struct insn *insn)
+{
+	if (unlikely(insn->prefixes.nbytes == sizeof(xen_prefix))) {
+		return !memcmp(insn->prefixes.bytes, xen_prefix, 3) &&
+			insn->prefixes.bytes[3] ==
+				xen_prefix[sizeof(xen_prefix) - 1];
+	}
+
+	return 0;
+}
+
 /**
  * insn_get_prefixes - scan x86 instruction prefix bytes
  * @insn:	&struct insn containing instruction
@@ -79,6 +118,10 @@ void insn_get_prefixes(struct insn *insn)
 	nb = 0;
 	lb = 0;
 	b = peek_next(insn_byte_t, insn);
+
+	if (insn_xen_prefix(insn, b))
+		return;
+
 	attr = inat_get_opcode_attribute(b);
 	while (inat_is_legacy_prefix(attr)) {
 		/* Skip if same prefix */
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index 37a4c390750b..1c723fb5c6ee 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -128,6 +128,8 @@ static inline int insn_is_evex(struct insn *insn)
 	return (insn->vex_prefix.nbytes == 4);
 }
 
+extern int insn_has_xen_prefix(struct insn *insn);
+
 /* Ensure this instruction is decoded completely */
 static inline int insn_complete(struct insn *insn)
 {
diff --git a/tools/arch/x86/include/asm/xen/prefix.h b/tools/arch/x86/include/asm/xen/prefix.h
new file mode 100644
index 000000000000..f901be0d7a95
--- /dev/null
+++ b/tools/arch/x86/include/asm/xen/prefix.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _TOOLS_ASM_X86_XEN_PREFIX_H
+#define _TOOLS_ASM_X86_XEN_PREFIX_H
+
+#include <linux/stringify.h>
+
+#define __XEN_EMULATE_PREFIX  0x0f,0x0b,0x78,0x65,0x6e
+#define __XEN_EMULATE_PREFIX_STR  __stringify(__XEN_EMULATE_PREFIX)
+
+#endif
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index 79e048f1d902..b34cd1cefb1c 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -13,6 +13,9 @@
 #include "../include/asm/inat.h"
 #include "../include/asm/insn.h"
 
+/* For special Xen prefix */
+#include "../include/asm/xen/prefix.h"
+
 /* Verify next sizeof(t) bytes can be on the same instruction */
 #define validate_next(t, insn, n)	\
 	((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
@@ -58,6 +61,42 @@ void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64)
 		insn->addr_bytes = 4;
 }
 
+static const insn_byte_t xen_prefix[] = { __XEN_EMULATE_PREFIX };
+
+static int insn_xen_prefix(struct insn *insn, insn_byte_t b)
+{
+	struct insn_field *prefixes = &insn->prefixes;
+	size_t i;
+
+	for (i = 0; i < sizeof(xen_prefix); i++) {
+		b = peek_nbyte_next(insn_byte_t, insn, i);
+		if (b != xen_prefix[i])
+			goto err_out;
+	}
+
+	memcpy(prefixes->bytes, xen_prefix, 3);
+	prefixes->bytes[3] = xen_prefix[sizeof(xen_prefix) - 1];
+	prefixes->nbytes = sizeof(xen_prefix);
+	insn->next_byte += prefixes->nbytes;
+	prefixes->got = 1;
+
+	return 1;
+
+err_out:
+	return 0;
+}
+
+int insn_has_xen_prefix(struct insn *insn)
+{
+	if (unlikely(insn->prefixes.nbytes == sizeof(xen_prefix))) {
+		return !memcmp(insn->prefixes.bytes, xen_prefix, 3) &&
+			insn->prefixes.bytes[3] ==
+				xen_prefix[sizeof(xen_prefix) - 1];
+	}
+
+	return 0;
+}
+
 /**
  * insn_get_prefixes - scan x86 instruction prefix bytes
  * @insn:	&struct insn containing instruction
@@ -79,6 +118,10 @@ void insn_get_prefixes(struct insn *insn)
 	nb = 0;
 	lb = 0;
 	b = peek_next(insn_byte_t, insn);
+
+	if (insn_xen_prefix(insn, b))
+		return;
+
 	attr = inat_get_opcode_attribute(b);
 	while (inat_is_legacy_prefix(attr)) {
 		/* Skip if same prefix */
diff --git a/tools/objtool/sync-check.sh b/tools/objtool/sync-check.sh
index 0a832e265a50..34143ea3d477 100755
--- a/tools/objtool/sync-check.sh
+++ b/tools/objtool/sync-check.sh
@@ -4,6 +4,7 @@
 FILES='
 arch/x86/include/asm/inat_types.h
 arch/x86/include/asm/orc_types.h
+arch/x86/include/asm/xen/prefix.h
 arch/x86/lib/x86-opcode-map.txt
 arch/x86/tools/gen-insn-attr-x86.awk
 '
@@ -46,6 +47,6 @@ done
 check arch/x86/include/asm/inat.h     '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"'
 check arch/x86/include/asm/insn.h     '-I "^#include [\"<]\(asm/\)*inat.h[\">]"'
 check arch/x86/lib/inat.c             '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"'
-check arch/x86/lib/insn.c             '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]"'
+check arch/x86/lib/insn.c             '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/xen/prefix.h[\">]"'
 
 cd -

[PATCH -tip 2/2] x86: kprobes: Prohibit probing on instruction which has Xen prefix

[Masami Hiramatsu]

Prohibit probing on instruction which has XEN_EMULATE_PREFIX
(it must be cpuid.) Since that prefix is a marker for Xen,
if we modify the marker by kprobe's int3, that doesn't work
as expected.

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

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 43fc13c831af..b1e86af4a985 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -351,6 +351,10 @@ int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn)
 	kernel_insn_init(insn, dest, MAX_INSN_SIZE);
 	insn_get_length(insn);
 
+	/* We can not probe XEN_EMULATE_PREFIX instruction */
+	if (insn_has_xen_prefix(insn))
+		return 0;
+
 	/* Another subsystem puts a breakpoint, failed to recover */
 	if (insn->opcode.bytes[0] == BREAKPOINT_INSTRUCTION)
 		return 0;

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Andrew Cooper]

On 04/09/2019 12:45, Masami Hiramatsu wrote:
> Hi,
>
> These patches allow x86 instruction decoder to decode
> xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit
> kprobes to probe on it.
>
> Josh reported that the objtool can not decode such special
> prefixed instructions, and I found that we also have to
> prohibit kprobes to probe on such instruction.
>
> This series can be applied on -tip master branch which
> has merged Josh's objtool/perf sharing common x86 insn
> decoder series.

The paravirtualised xen-cpuid is were you'll see it most in a regular
kernel, but be aware that it is also used for testing purposes in other
circumstances, and there is an equivalent KVM prefix which is used for
KVM testing.

It might be better to generalise the decode support to "virtualisation
escape prefix" or something slightly more generic.

~Andrew

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Masami Hiramatsu]

On Wed, 4 Sep 2019 12:54:55 +0100
Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> On 04/09/2019 12:45, Masami Hiramatsu wrote:
> > Hi,
> >
> > These patches allow x86 instruction decoder to decode
> > xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit
> > kprobes to probe on it.
> >
> > Josh reported that the objtool can not decode such special
> > prefixed instructions, and I found that we also have to
> > prohibit kprobes to probe on such instruction.
> >
> > This series can be applied on -tip master branch which
> > has merged Josh's objtool/perf sharing common x86 insn
> > decoder series.
> 
> The paravirtualised xen-cpuid is were you'll see it most in a regular
> kernel, but be aware that it is also used for testing purposes in other
> circumstances, and there is an equivalent KVM prefix which is used for
> KVM testing.

Good catch! I didn't notice that. Is that really same sequance or KVM uses
another sequence of instructions for KVM prefix?

> 
> It might be better to generalise the decode support to "virtualisation
> escape prefix" or something slightly more generic.

Agreed, it is easy to expand it, we can switch the prefix template.
Could you tell me where I should look? I will add it.

Thank you,


> 
> ~Andrew


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Andrew Cooper]

On 05/09/2019 02:49, Masami Hiramatsu wrote:
> On Wed, 4 Sep 2019 12:54:55 +0100
> Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
>> On 04/09/2019 12:45, Masami Hiramatsu wrote:
>>> Hi,
>>>
>>> These patches allow x86 instruction decoder to decode
>>> xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit
>>> kprobes to probe on it.
>>>
>>> Josh reported that the objtool can not decode such special
>>> prefixed instructions, and I found that we also have to
>>> prohibit kprobes to probe on such instruction.
>>>
>>> This series can be applied on -tip master branch which
>>> has merged Josh's objtool/perf sharing common x86 insn
>>> decoder series.
>> The paravirtualised xen-cpuid is were you'll see it most in a regular
>> kernel, but be aware that it is also used for testing purposes in other
>> circumstances, and there is an equivalent KVM prefix which is used for
>> KVM testing.
> Good catch! I didn't notice that. Is that really same sequance or KVM uses
> another sequence of instructions for KVM prefix?

I don't know if you've spotted, but the prefix is a ud2a instruction
followed by 'xen' in ascii.

The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2

>
>> It might be better to generalise the decode support to "virtualisation
>> escape prefix" or something slightly more generic.
> Agreed, it is easy to expand it, we can switch the prefix template.
> Could you tell me where I should look? I will add it.

These are the only two I'm aware of.

~Andrew

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Peter Zijlstra]

On Thu, Sep 05, 2019 at 08:54:17AM +0100, Andrew Cooper wrote:

> I don't know if you've spotted, but the prefix is a ud2a instruction
> followed by 'xen' in ascii.
>
> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2

While the Xen one disassebles to valid instructions, that KVM one does
not:

	.text
xen:
	ud2; .ascii "xen"
kvm:
	ud2; .ascii "kvm"

disassembles like:

0000000000000000 <xen>:
   0:   0f 0b                   ud2
   2:   78 65                   js     69 <kvm+0x64>
   4:   6e                      outsb  %ds:(%rsi),(%dx)
0000000000000005 <kvm>:
   5:   0f 0b                   ud2
   7:   6b                      .byte 0x6b
   8:   76 6d                   jbe    77 <kvm+0x72>

Which is a bit unfortunate I suppose. At least they don't appear to
consume further bytes.

I know it is water under the bridge at this point; but you could've used
UD1 with a displacement with some 'unlikely' values. That way it
would've decoded to a single instruction.

Something like:

	ud1    0x6e6578(%rax),%rax

which spells out "xen\0" in the displacement:

	48 0f b9 80 78 65 6e 00

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Andrew Cooper]

On 05/09/2019 09:26, Peter Zijlstra wrote:
> On Thu, Sep 05, 2019 at 08:54:17AM +0100, Andrew Cooper wrote:
>
>> I don't know if you've spotted, but the prefix is a ud2a instruction
>> followed by 'xen' in ascii.
>>
>> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2
> While the Xen one disassebles to valid instructions, that KVM one does
> not:
>
> 	.text
> xen:
> 	ud2; .ascii "xen"
> kvm:
> 	ud2; .ascii "kvm"
>
> disassembles like:
>
> 0000000000000000 <xen>:
>    0:   0f 0b                   ud2
>    2:   78 65                   js     69 <kvm+0x64>
>    4:   6e                      outsb  %ds:(%rsi),(%dx)
> 0000000000000005 <kvm>:
>    5:   0f 0b                   ud2
>    7:   6b                      .byte 0x6b
>    8:   76 6d                   jbe    77 <kvm+0x72>
>
> Which is a bit unfortunate I suppose. At least they don't appear to
> consume further bytes.

It does when you give objdump one extra byte to look at.

0000000000000005 <kvm>:
   5:    0f 0b                    ud2   
   7:    6b 76 6d 00              imul   $0x0,0x6d(%rsi),%esi

I did try to point out that this property should have been checked
before settling on 'kvm' as the string.

As for the Xen prefix, it's introduction pre-dates me substantially, and
I don't know whether the disassembly was considered, or we just got lucky.

IMO, the string 'Xen' would would have been sightly nicer

0000000000000005 <Xen>:
   5:    0f 0b                    ud2   
   7:    58                       pop    %rax
   8:    65 6e                    outsb  %gs:(%rsi),(%dx)

but we're 13 years too late to amend this.

> I know it is water under the bridge at this point; but you could've used
> UD1 with a displacement with some 'unlikely' values. That way it
> would've decoded to a single instruction.
>
> Something like:
>
> 	ud1    0x6e6578(%rax),%rax
>
> which spells out "xen\0" in the displacement:
>
> 	48 0f b9 80 78 65 6e 00

:)

I seem to recall UD0 and UD1 being very late to the documentation party.

~Andrew

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Peter Zijlstra]

On Thu, Sep 05, 2019 at 09:53:32AM +0100, Andrew Cooper wrote:
> On 05/09/2019 09:26, Peter Zijlstra wrote:
> > On Thu, Sep 05, 2019 at 08:54:17AM +0100, Andrew Cooper wrote:
> >
> >> I don't know if you've spotted, but the prefix is a ud2a instruction
> >> followed by 'xen' in ascii.
> >>
> >> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2
> > While the Xen one disassebles to valid instructions, that KVM one does
> > not:
> >
> > 	.text
> > xen:
> > 	ud2; .ascii "xen"
> > kvm:
> > 	ud2; .ascii "kvm"
> >
> > disassembles like:
> >
> > 0000000000000000 <xen>:
> >    0:   0f 0b                   ud2
> >    2:   78 65                   js     69 <kvm+0x64>
> >    4:   6e                      outsb  %ds:(%rsi),(%dx)
> > 0000000000000005 <kvm>:
> >    5:   0f 0b                   ud2
> >    7:   6b                      .byte 0x6b
> >    8:   76 6d                   jbe    77 <kvm+0x72>
> >
> > Which is a bit unfortunate I suppose. At least they don't appear to
> > consume further bytes.
> 
> It does when you give objdump one extra byte to look at.
> 
> 0000000000000005 <kvm>:
>    5:    0f 0b                    ud2   
>    7:    6b 76 6d 00              imul   $0x0,0x6d(%rsi),%esi
> 
> I did try to point out that this property should have been checked
> before settling on 'kvm' as the string.

Bah you're right; when I write:

	ud2; .ascii "kvm"; cpuid

The output is gibberish :/

> but we're 13 years too late to amend this.

Yah, I figured :/

> > I know it is water under the bridge at this point; but you could've used
> > UD1 with a displacement with some 'unlikely' values. That way it
> > would've decoded to a single instruction.
> >
> > Something like:
> >
> > 	ud1    0x6e6578(%rax),%rax
> >
> > which spells out "xen\0" in the displacement:
> >
> > 	48 0f b9 80 78 65 6e 00
> 
> :)
> 
> I seem to recall UD0 and UD1 being very late to the documentation party.

They were; and the documentation for still UD0 differs between vendors :/

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Andrew Cooper]

On 05/09/2019 10:26, Peter Zijlstra wrote:
> On Thu, Sep 05, 2019 at 09:53:32AM +0100, Andrew Cooper wrote:
>> On 05/09/2019 09:26, Peter Zijlstra wrote:
>>> On Thu, Sep 05, 2019 at 08:54:17AM +0100, Andrew Cooper wrote:
>>>
>>>> I don't know if you've spotted, but the prefix is a ud2a instruction
>>>> followed by 'xen' in ascii.
>>>>
>>>> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2
>>> While the Xen one disassebles to valid instructions, that KVM one does
>>> not:
>>>
>>> 	.text
>>> xen:
>>> 	ud2; .ascii "xen"
>>> kvm:
>>> 	ud2; .ascii "kvm"
>>>
>>> disassembles like:
>>>
>>> 0000000000000000 <xen>:
>>>    0:   0f 0b                   ud2
>>>    2:   78 65                   js     69 <kvm+0x64>
>>>    4:   6e                      outsb  %ds:(%rsi),(%dx)
>>> 0000000000000005 <kvm>:
>>>    5:   0f 0b                   ud2
>>>    7:   6b                      .byte 0x6b
>>>    8:   76 6d                   jbe    77 <kvm+0x72>
>>>
>>> Which is a bit unfortunate I suppose. At least they don't appear to
>>> consume further bytes.
>> It does when you give objdump one extra byte to look at.
>>
>> 0000000000000005 <kvm>:
>>    5:    0f 0b                    ud2   
>>    7:    6b 76 6d 00              imul   $0x0,0x6d(%rsi),%esi
>>
>> I did try to point out that this property should have been checked
>> before settling on 'kvm' as the string.
> Bah you're right; when I write:
>
> 	ud2; .ascii "kvm"; cpuid
>
> The output is gibberish :/

KVM could possibly amend this.  It is an off-by-default testing-only
interface.

Xen's is part of the ABI for ring-deprivielged guests, to force CPUID to
be the virtualised view on hardware without CPUID Faulting.

~Andrew

[OT] Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Masami Hiramatsu]

On Thu, 5 Sep 2019 09:53:32 +0100
Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> On 05/09/2019 09:26, Peter Zijlstra wrote:
> > On Thu, Sep 05, 2019 at 08:54:17AM +0100, Andrew Cooper wrote:
> >
> >> I don't know if you've spotted, but the prefix is a ud2a instruction
> >> followed by 'xen' in ascii.
> >>
> >> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2
> > While the Xen one disassebles to valid instructions, that KVM one does
> > not:
> >
> > 	.text
> > xen:
> > 	ud2; .ascii "xen"
> > kvm:
> > 	ud2; .ascii "kvm"
> >
> > disassembles like:
> >
> > 0000000000000000 <xen>:
> >    0:   0f 0b                   ud2
> >    2:   78 65                   js     69 <kvm+0x64>
> >    4:   6e                      outsb  %ds:(%rsi),(%dx)
> > 0000000000000005 <kvm>:
> >    5:   0f 0b                   ud2
> >    7:   6b                      .byte 0x6b
> >    8:   76 6d                   jbe    77 <kvm+0x72>
> >
> > Which is a bit unfortunate I suppose. At least they don't appear to
> > consume further bytes.
> 
> It does when you give objdump one extra byte to look at.
> 
> 0000000000000005 <kvm>:
>    5:    0f 0b                    ud2   
>    7:    6b 76 6d 00              imul   $0x0,0x6d(%rsi),%esi
> 

Hmm, that consumes the first byte of the next instruction.
For example, 

  .text
xen:
  ud2; .ascii "xen"; cpuid
kvm:
  ud2; .ascii "kvm"; cpuid

0000000000000000 <xen>:
   0:	0f 0b                	ud2    
   2:	78 65                	js     69 <kvm+0x62>
   4:	6e                   	outsb  %ds:(%rsi),(%dx)
   5:	0f a2                	cpuid  

0000000000000007 <kvm>:
   7:	0f 0b                	ud2    
   9:	6b 76 6d 0f          	imul   $0xf,0x6d(%rsi),%esi
   d:	a2                   	.byte 0xa2

This will disturbe decoding bytestream. Anyway, with the next version
it will be fixed in x86 insn decoder.

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Masami Hiramatsu]

On Thu, 5 Sep 2019 08:54:17 +0100
Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> On 05/09/2019 02:49, Masami Hiramatsu wrote:
> > On Wed, 4 Sep 2019 12:54:55 +0100
> > Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >
> >> On 04/09/2019 12:45, Masami Hiramatsu wrote:
> >>> Hi,
> >>>
> >>> These patches allow x86 instruction decoder to decode
> >>> xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit
> >>> kprobes to probe on it.
> >>>
> >>> Josh reported that the objtool can not decode such special
> >>> prefixed instructions, and I found that we also have to
> >>> prohibit kprobes to probe on such instruction.
> >>>
> >>> This series can be applied on -tip master branch which
> >>> has merged Josh's objtool/perf sharing common x86 insn
> >>> decoder series.
> >> The paravirtualised xen-cpuid is were you'll see it most in a regular
> >> kernel, but be aware that it is also used for testing purposes in other
> >> circumstances, and there is an equivalent KVM prefix which is used for
> >> KVM testing.
> > Good catch! I didn't notice that. Is that really same sequance or KVM uses
> > another sequence of instructions for KVM prefix?
> 
> I don't know if you've spotted, but the prefix is a ud2a instruction
> followed by 'xen' in ascii.
> 
> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2
> 

Ah, OK. I see it. But it seems that another ud0/ud1 can be used by
other (new) virtualization. So at this moment I will just add a sequence
as a pattern of prefix. Not use a fixed ud2 + sig.

Thank you,

> >
> >> It might be better to generalise the decode support to "virtualisation
> >> escape prefix" or something slightly more generic.
> > Agreed, it is easy to expand it, we can switch the prefix template.
> > Could you tell me where I should look? I will add it.
> 
> These are the only two I'm aware of.



> 
> ~Andrew


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Masami Hiramatsu]

On Thu, 5 Sep 2019 20:32:24 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 5 Sep 2019 08:54:17 +0100
> Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> > On 05/09/2019 02:49, Masami Hiramatsu wrote:
> > > On Wed, 4 Sep 2019 12:54:55 +0100
> > > Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > >
> > >> On 04/09/2019 12:45, Masami Hiramatsu wrote:
> > >>> Hi,
> > >>>
> > >>> These patches allow x86 instruction decoder to decode
> > >>> xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit
> > >>> kprobes to probe on it.
> > >>>
> > >>> Josh reported that the objtool can not decode such special
> > >>> prefixed instructions, and I found that we also have to
> > >>> prohibit kprobes to probe on such instruction.
> > >>>
> > >>> This series can be applied on -tip master branch which
> > >>> has merged Josh's objtool/perf sharing common x86 insn
> > >>> decoder series.
> > >> The paravirtualised xen-cpuid is were you'll see it most in a regular
> > >> kernel, but be aware that it is also used for testing purposes in other
> > >> circumstances, and there is an equivalent KVM prefix which is used for
> > >> KVM testing.
> > > Good catch! I didn't notice that. Is that really same sequance or KVM uses
> > > another sequence of instructions for KVM prefix?
> > 
> > I don't know if you've spotted, but the prefix is a ud2a instruction
> > followed by 'xen' in ascii.
> > 
> > The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2

Hmm, I think I might misunderstand what the "emulate prefix"... that is not
a prefix which replace actual prefix, but just works like an escape sequence.
Thus the next instruction can have any x86 prefix, correct?

If so, this patch doesn't work. I have to add a new field in struct insn
like "insn.emulate_prefix_size" so that we can keep a room for the prefixes
for real instruction.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Andrew Cooper]

On 05/09/2019 14:09, Masami Hiramatsu wrote:
> On Thu, 5 Sep 2019 20:32:24 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
>> On Thu, 5 Sep 2019 08:54:17 +0100
>> Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>>> On 05/09/2019 02:49, Masami Hiramatsu wrote:
>>>> On Wed, 4 Sep 2019 12:54:55 +0100
>>>> Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>>
>>>>> On 04/09/2019 12:45, Masami Hiramatsu wrote:
>>>>>> Hi,
>>>>>>
>>>>>> These patches allow x86 instruction decoder to decode
>>>>>> xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit
>>>>>> kprobes to probe on it.
>>>>>>
>>>>>> Josh reported that the objtool can not decode such special
>>>>>> prefixed instructions, and I found that we also have to
>>>>>> prohibit kprobes to probe on such instruction.
>>>>>>
>>>>>> This series can be applied on -tip master branch which
>>>>>> has merged Josh's objtool/perf sharing common x86 insn
>>>>>> decoder series.
>>>>> The paravirtualised xen-cpuid is were you'll see it most in a regular
>>>>> kernel, but be aware that it is also used for testing purposes in other
>>>>> circumstances, and there is an equivalent KVM prefix which is used for
>>>>> KVM testing.
>>>> Good catch! I didn't notice that. Is that really same sequance or KVM uses
>>>> another sequence of instructions for KVM prefix?
>>> I don't know if you've spotted, but the prefix is a ud2a instruction
>>> followed by 'xen' in ascii.
>>>
>>> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2
> Hmm, I think I might misunderstand what the "emulate prefix"... that is not
> a prefix which replace actual prefix, but just works like an escape sequence.
> Thus the next instruction can have any x86 prefix, correct?

There is a bit of history here :)

Originally, 13 years ago, Xen invented the "Force Emulate Prefix", which
was the sequence:

ud2a; .ascii 'xen'; cpuid

which hit the #UD handler and was recognised as a request for
virtualised CPUID information.  This was for ring-deprivileged
virtualisation, and is needed because the CPUID instruction itself
doesn't trap to the hypervisor.

Following some security issues in our instruction emulator, I reused
this prefix with VT-x/SVM guests for testing purposes.  It behaves in a
similar manner - when enabled, it is recognised in #UD exception
intercept, and causes Xen to add 5 to the instruction pointer, then
emulate the instruction starting there.

Then various folk thought that having the same kind of ability to test
KVM's instruction emulator would be a good idea, so they borrowed the idea.

From a behaviour point of view, it is an opaque 5 bytes which means
"break into the hypervisor, then emulate the following instruction".

The name "prefix" is unfortunate.  It was named thusly because from the
programmers point of view, it was something you put before the CPUID
instruction which wanted to be emulated.  It is not related to x86
instruction concept of a prefix.

~Andrew

Re: [Xen-devel] [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Masami Hiramatsu]

On Thu, 5 Sep 2019 14:31:56 +0100
Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> >>> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2
> > Hmm, I think I might misunderstand what the "emulate prefix"... that is not
> > a prefix which replace actual prefix, but just works like an escape sequence.
> > Thus the next instruction can have any x86 prefix, correct?
> 
> There is a bit of history here :)
> 
> Originally, 13 years ago, Xen invented the "Force Emulate Prefix", which
> was the sequence:
> 
> ud2a; .ascii 'xen'; cpuid
> 
> which hit the #UD handler and was recognised as a request for
> virtualised CPUID information.  This was for ring-deprivileged
> virtualisation, and is needed because the CPUID instruction itself
> doesn't trap to the hypervisor.
> 
> Following some security issues in our instruction emulator, I reused
> this prefix with VT-x/SVM guests for testing purposes.  It behaves in a
> similar manner - when enabled, it is recognised in #UD exception
> intercept, and causes Xen to add 5 to the instruction pointer, then
> emulate the instruction starting there.
> 
> Then various folk thought that having the same kind of ability to test
> KVM's instruction emulator would be a good idea, so they borrowed the idea.
> 
> From a behaviour point of view, it is an opaque 5 bytes which means
> "break into the hypervisor, then emulate the following instruction".
> 
> The name "prefix" is unfortunate.  It was named thusly because from the
> programmers point of view, it was something you put before the CPUID
> instruction which wanted to be emulated.  It is not related to x86
> instruction concept of a prefix.

OK, then we should not use the insn->prefixes for those escape sequences.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip 0/2] x86: Prohibit kprobes on XEN_EMULATE_PREFIX

[Peter Zijlstra]

On Wed, Sep 04, 2019 at 08:45:47PM +0900, Masami Hiramatsu wrote:
> Hi,
> 
> These patches allow x86 instruction decoder to decode
> xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit
> kprobes to probe on it.
> 
> Josh reported that the objtool can not decode such special
> prefixed instructions, and I found that we also have to
> prohibit kprobes to probe on such instruction.
> 
> This series can be applied on -tip master branch which
> has merged Josh's objtool/perf sharing common x86 insn
> decoder series.
> 
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (2):
>       x86: xen: insn: Decode XEN_EMULATE_PREFIX correctly
>       x86: kprobes: Prohibit probing on instruction which has Xen prefix
> 
> 
>  arch/x86/include/asm/insn.h             |    2 +
>  arch/x86/include/asm/xen/interface.h    |    7 ++++-
>  arch/x86/include/asm/xen/prefix.h       |   10 +++++++
>  arch/x86/kernel/kprobes/core.c          |    4 +++
>  arch/x86/lib/insn.c                     |   43 +++++++++++++++++++++++++++++++
>  tools/arch/x86/include/asm/insn.h       |    2 +
>  tools/arch/x86/include/asm/xen/prefix.h |   10 +++++++
>  tools/arch/x86/lib/insn.c               |   43 +++++++++++++++++++++++++++++++
>  tools/objtool/sync-check.sh             |    3 +-
>  9 files changed, 121 insertions(+), 3 deletions(-)
>  create mode 100644 arch/x86/include/asm/xen/prefix.h
>  create mode 100644 tools/arch/x86/include/asm/xen/prefix.h

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>