Re: linux-next: Tree for Aug 27 (objtool)

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Thu, Aug 29, 2019 at 10:53:56AM +0900, Masami Hiramatsu wrote:
> Hi Josh,
> 
> On Wed, 28 Aug 2019 11:34:33 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Wed, Aug 28, 2019 at 11:13:31AM -0500, Josh Poimboeuf wrote:
> > > Turns out this patch does break something:
> > > 
> > >   arch/x86/xen/enlighten_pv.o: warning: objtool: xen_cpuid()+0x25: can't find jump dest instruction at .text+0x9c
> > > 
> > > I'll need to figure out a better way to whitelist that
> > > XEN_EMULATE_PREFIX fake instruction thing.  I'll probably just teach
> > > the objtool decoder about it.
> > 
> > Hi Masami,
> > 
> > Is it possible for the kernel x86 decoder to recognize the
> > XEN_EMULATE_PREFIX prefix?
> > 
> >         asm(XEN_EMULATE_PREFIX "cpuid"
> >                 : "=a" (*ax),
> >                   "=b" (*bx),
> >                   "=c" (*cx),
> >                   "=d" (*dx)
> >                 : "0" (*ax), "2" (*cx));
> > 
> > is disassembled to:
> > 
> >       33:       0f 0b                   ud2
> >       35:       78 65                   js     9c <xen_store_tr+0xc>
> >       37:       6e                      outsb  %ds:(%rsi),(%dx)
> >       38:       0f a2                   cpuid
> > 
> > which confuses objtool.  Presumably that would confuse other users of
> > the decoder as well.
> 
> Good catch! It should be problematic, since x86 decoder sanity test is
> based on objtool.

I think you mean the decoder test is based on objdump, not objtool?

Actually I wonder if X86_DECODER_SELFTEST is even still needed these
days, since objtool is enabled on default configs.  Objtool already uses
the decoder to disassemble every instruction in the kernel (except for a
few whitelisted files).

> But I don't want to change the test code itself,
> because this problem is highly depending on Xen.
> 
> > That's a highly unlikely sequence of instructions, maybe the kernel
> > decoder should recognize it as a single instruction.
> 
> OK, it is better to be done in decoder (only for CONFIG_XEN_PVHVM)
> 
> BTW, could you also share what test case would you using?

Enable CONFIG_XEN_PV and CONFIG_STACK_VALIDATION, and remove the
STACK_FRAME_NON_STANDARD(xen_cpuid) line from
arch/x86/xen/enlighten_pv.c.  objtool will complain:

  arch/x86/xen/enlighten_pv.o: warning: objtool: xen_cpuid()+0x25: can't find jump dest instruction at .text+0x9c

Basing it on CONFIG_XEN_PVHVM may be problematic.  The decoder is
duplicated in the tools directory so objtool can use it.  But the tools
don't know about kernel configs.

BTW, I'm not sure if you're aware of this, but both objtool and perf
have identical copies of the decoder.  The makefiles warn if they get
out of sync with the kernel version.

We will always need at least one copy of the decoder in tools, because
the tools subdir is supposed to be standalone from the rest of the
kernel.  Still, I may look at combining the perf and objtool copies into
a single shared copy.

> And what about attached patch? (just compile checked with/without CONFIG_XEN_PVHVM)

I copied the decoder to objtool, removed the CONFIG_XEN_PVHVM ifdef, and
played a bit with the includes, and got it to compile with objtool, but
it still fails:

  $ make arch/x86/xen/enlighten_pv.o
  arch/x86/xen/enlighten_pv.o: warning: objtool: xen_cpuid()+0x25: can't find jump dest instruction at .text+0x9c

Patch below:

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 58f79ab32358..dbba9b2dc408 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -193,7 +193,6 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 
 	*bx &= maskebx;
 }
-STACK_FRAME_NON_STANDARD(xen_cpuid); /* XEN_EMULATE_PREFIX */
 
 static bool __init xen_check_mwait(void)
 {
diff --git a/tools/objtool/arch/x86/include/asm/xen/interface.h b/tools/objtool/arch/x86/include/asm/xen/interface.h
new file mode 100644
index 000000000000..c11286eac21c
--- /dev/null
+++ b/tools/objtool/arch/x86/include/asm/xen/interface.h
@@ -0,0 +1,38 @@
+/******************************************************************************
+ * arch-x86_32.h
+ *
+ * Guest OS interface to x86 Xen.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (c) 2004-2006, K A Fraser
+ */
+#ifndef _ASM_X86_XEN_INTERFACE_H
+#define _ASM_X86_XEN_INTERFACE_H
+
+#include <linux/stringify.h>
+/*
+ * Prefix forces emulation of some non-trapping instructions.
+ * Currently only CPUID.
+ */
+#define __XEN_EMULATE_PREFIX  0x0f,0x0b,0x78,0x65,0x6e
+#define __XEN_EMULATE_PREFIX_STR  __stringify(__XEN_EMULATE_PREFIX)
+#define XEN_EMULATE_PREFIX ".byte " __XEN_EMULATE_PREFIX_STR " ; "
+
+#endif /* _ASM_X86_XEN_INTERFACE_H */
diff --git a/tools/objtool/arch/x86/lib/insn.c b/tools/objtool/arch/x86/lib/insn.c
index 0b5862ba6a75..bc77f607932e 100644
--- a/tools/objtool/arch/x86/lib/insn.c
+++ b/tools/objtool/arch/x86/lib/insn.c
@@ -10,6 +10,8 @@
 #else
 #include <string.h>
 #endif
+#include <linux/kernel.h>
+#include <asm/xen/interface.h>
 #include <asm/inat.h>
 #include <asm/insn.h>
 
@@ -58,6 +60,30 @@ 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;
+	int i = 0;
+
+	while (i < ARRAY_SIZE(xen_prefix) && b == xen_prefix[i])
+		b = peek_nbyte_next(insn_byte_t, insn, ++i);
+
+	if (unlikely(i == ARRAY_SIZE(xen_prefix))) {
+		memcpy(prefixes->bytes, xen_prefix, 3);
+		prefixes->bytes[3] = xen_prefix[ARRAY_SIZE(xen_prefix) - 1];
+		prefixes->nbytes = ARRAY_SIZE(xen_prefix);
+		insn->next_byte += prefixes->nbytes;
+		prefixes->got = 1;
+
+		return 1;
+	}
+
+err_out:
+	return 0;
+}
+
 /**
  * insn_get_prefixes - scan x86 instruction prefix bytes
  * @insn:	&struct insn containing instruction
@@ -79,6 +105,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 */