Re: [PATCH v6 07/10] x86, mpx: decode MPX instruction to get bound violation information

[Dave Hansen <dave.hansen@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

On 06/18/2014 11:53 PM, Ren, Qiaowei wrote:
> On 2014-06-19, Borislav Petkov wrote:
>> On Thu, Jun 19, 2014 at 01:13:48AM +0000, Ren, Qiaowei wrote:
>>> On 2014-06-18, Borislav Petkov wrote:
>>>> On Wed, Jun 18, 2014 at 05:44:13PM +0800, Qiaowei Ren wrote:
>>>>
>>>> This whole insn decoding machinery above looks like adapted from
>>>> arch/x86/lib/insn.c. You should merge it with the generic code in
>>>> insn.c instead of homegrowing it here only for the purposes of MPX.
>>>> And if it doesn't work for your needs, you should should extend
>>>> the generic code to do so.
>>
>>> Petkov, as we discussed on initial version of this patchset, general
>>> insn framework didn't work out well and I have tried to use generic
>>> struct insn, insn_field, etc. for obvious benefits.
>>
>> Let me repeat myself: "And if it doesn't work for your needs, you
>> should extend the generic code to do so."
>>
>> We don't do homegrown almost-copies of generic code.
>> 
> I see. If possible, I will be very happy to use or extend generic
> code. But due to extra overhead caused by MPX, I have to use MPX
> specific decoding to do performance optimization.

Could you please support this position with some data?  I'm a bit
skeptical that instruction decoding is going to be a
performance-critical path.

I also don't see the extra field that you talked about in the previous
thread?  What's the extra field?  I see a 'limit' vs. 'length', but you
don't use 'length' at all, so I think you can use it instead, or at
least union it.

I've taken a quick stab at trying to consolidate things.  I think I may
have screwed up this:

	insn->limit = MAX_MPX_INSN_SIZE - bytes;

Qiaowei, is there anything fundamentally broken with what I've got here?


[-- Attachment #2: consolidate-instruction-decoding.patch --]
[-- Type: text/x-patch, Size: 9071 bytes --]



---

 b/arch/x86/include/asm/insn.h |    1 
 b/arch/x86/include/asm/mpx.h  |   14 ---
 b/arch/x86/kernel/mpx.c       |  183 ++----------------------------------------
 b/arch/x86/lib/insn.c         |   43 +++++++++
 4 files changed, 56 insertions(+), 185 deletions(-)

diff -puN arch/x86/kernel/mpx.c~consolidate-instruction-decoding arch/x86/kernel/mpx.c
--- a/arch/x86/kernel/mpx.c~consolidate-instruction-decoding	2014-06-19 09:53:53.894441788 -0700
+++ b/arch/x86/kernel/mpx.c	2014-06-19 09:53:53.909442460 -0700
@@ -3,6 +3,7 @@
 #include <linux/prctl.h>
 #include <asm/mpx.h>
 #include <asm/i387.h>
+#include <asm/insn.h>
 #include <asm/fpu-internal.h>
 
 /*
@@ -59,7 +60,7 @@ int mpx_unregister(struct task_struct *t
 }
 
 typedef enum {REG_TYPE_RM, REG_TYPE_INDEX, REG_TYPE_BASE} reg_type_t;
-static unsigned long get_reg(struct mpx_insn *insn, struct pt_regs *regs,
+static unsigned long get_reg(struct insn *insn, struct pt_regs *regs,
 			     reg_type_t type)
 {
 	int regno = 0;
@@ -118,7 +119,7 @@ static unsigned long get_reg(struct mpx_
  * for rm=3 returning the content of the rm reg
  * for rm!=3 calculates the address using SIB and Disp
  */
-static unsigned long get_addr_ref(struct mpx_insn *insn, struct pt_regs *regs)
+static unsigned long get_addr_ref(struct insn *insn, struct pt_regs *regs)
 {
 	unsigned long addr;
 	unsigned long base;
@@ -142,182 +143,22 @@ static unsigned long get_addr_ref(struct
 	return addr;
 }
 
-/* Verify next sizeof(t) bytes can be on the same instruction */
-#define validate_next(t, insn, n)	\
-	((insn)->next_byte + sizeof(t) + n - (insn)->kaddr <= (insn)->limit)
-
-#define __get_next(t, insn)		\
-({					\
-	t r = *(t *)insn->next_byte;	\
-	insn->next_byte += sizeof(t);	\
-	r;				\
-})
-
-#define __peek_next(t, insn)		\
-({					\
-	t r = *(t *)insn->next_byte;	\
-	r;				\
-})
-
-#define get_next(t, insn)		\
-({					\
-	if (unlikely(!validate_next(t, insn, 0)))	\
-		goto err_out;		\
-	__get_next(t, insn);		\
-})
-
-#define peek_next(t, insn)		\
-({					\
-	if (unlikely(!validate_next(t, insn, 0)))	\
-		goto err_out;		\
-	__peek_next(t, insn);		\
-})
-
-static void mpx_insn_get_prefixes(struct mpx_insn *insn)
-{
-	unsigned char b;
-
-	/* Decode legacy prefix and REX prefix */
-	b = peek_next(unsigned char, insn);
-	while (b != 0x0f) {
-		/*
-		 * look for a rex prefix
-		 * a REX prefix cannot be followed by a legacy prefix.
-		 */
-		if (insn->x86_64 && ((b&0xf0) == 0x40)) {
-			insn->rex_prefix.value = b;
-			insn->rex_prefix.nbytes = 1;
-			insn->next_byte++;
-			break;
-		}
-
-		/* check the other legacy prefixes */
-		switch (b) {
-		case 0xf2:
-		case 0xf3:
-		case 0xf0:
-		case 0x64:
-		case 0x65:
-		case 0x2e:
-		case 0x3e:
-		case 0x26:
-		case 0x36:
-		case 0x66:
-		case 0x67:
-			insn->next_byte++;
-			break;
-		default: /* everything else is garbage */
-			goto err_out;
-		}
-		b = peek_next(unsigned char, insn);
-	}
-
-err_out:
-	return;
-}
-
-static void mpx_insn_get_modrm(struct mpx_insn *insn)
-{
-	insn->modrm.value = get_next(unsigned char, insn);
-	insn->modrm.nbytes = 1;
-
-err_out:
-	return;
-}
-
-static void mpx_insn_get_sib(struct mpx_insn *insn)
-{
-	unsigned char modrm = (unsigned char)insn->modrm.value;
-
-	if (X86_MODRM_MOD(modrm) != 3 && X86_MODRM_RM(modrm) == 4) {
-		insn->sib.value = get_next(unsigned char, insn);
-		insn->sib.nbytes = 1;
-	}
-
-err_out:
-	return;
-}
-
-static void mpx_insn_get_displacement(struct mpx_insn *insn)
-{
-	unsigned char mod, rm, base;
-
-	/*
-	 * Interpreting the modrm byte:
-	 * mod = 00 - no displacement fields (exceptions below)
-	 * mod = 01 - 1-byte displacement field
-	 * mod = 10 - displacement field is 4 bytes
-	 * mod = 11 - no memory operand
-	 *
-	 * mod != 11, r/m = 100 - SIB byte exists
-	 * mod = 00, SIB base = 101 - displacement field is 4 bytes
-	 * mod = 00, r/m = 101 - rip-relative addressing, displacement
-	 *	field is 4 bytes
-	 */
-	mod = X86_MODRM_MOD(insn->modrm.value);
-	rm = X86_MODRM_RM(insn->modrm.value);
-	base = X86_SIB_BASE(insn->sib.value);
-	if (mod == 3)
-		return;
-	if (mod == 1) {
-		insn->displacement.value = get_next(unsigned char, insn);
-		insn->displacement.nbytes = 1;
-	} else if ((mod == 0 && rm == 5) || mod == 2 ||
-			(mod == 0 && base == 5)) {
-		insn->displacement.value = get_next(int, insn);
-		insn->displacement.nbytes = 4;
-	}
-
-err_out:
-	return;
-}
-
-static void mpx_insn_init(struct mpx_insn *insn, struct pt_regs *regs)
-{
-	unsigned char buf[MAX_MPX_INSN_SIZE];
-	int bytes;
-
-	memset(insn, 0, sizeof(*insn));
-
-	bytes = copy_from_user(buf, (void __user *)regs->ip, MAX_MPX_INSN_SIZE);
-	insn->limit = MAX_MPX_INSN_SIZE - bytes;
-	insn->kaddr = buf;
-	insn->next_byte = buf;
-
-	/*
-	 * In 64-bit Mode, all Intel MPX instructions use 64-bit
-	 * operands for bounds and 64 bit addressing, i.e. REX.W &
-	 * 67H have no effect on data or address size.
-	 *
-	 * In compatibility and legacy modes (including 16-bit code
-	 * segments, real and virtual 8086 modes) all Intel MPX
-	 * instructions use 32-bit operands for bounds and 32 bit
-	 * addressing.
-	 */
-#ifdef CONFIG_X86_64
-	insn->x86_64 = 1;
-	insn->addr_bytes = 8;
-#else
-	insn->x86_64 = 0;
-	insn->addr_bytes = 4;
-#endif
-}
-
-static unsigned long mpx_insn_decode(struct mpx_insn *insn,
+static unsigned long insn_decode(struct insn *insn,
 				     struct pt_regs *regs)
 {
-	mpx_insn_init(insn, regs);
+	int is_64bit = IS_ENABLED(CONFIG_X86_64) && !test_thread_flag(TIF_IA32);
 
+	insn_init(insn, regs, is_64bit);
 	/*
 	 * In this case, we only need decode bndcl/bndcn/bndcu,
 	 * so we can use private diassembly interfaces to get
 	 * prefixes, modrm, sib, displacement, etc..
 	 */
-	mpx_insn_get_prefixes(insn);
+	insn_get_prefixes(insn);
 	insn->next_byte += 2; /* ignore opcode */
-	mpx_insn_get_modrm(insn);
-	mpx_insn_get_sib(insn);
-	mpx_insn_get_displacement(insn);
+	insn_get_modrm(insn);
+	insn_get_sib(insn);
+	insn_get_displacement(insn);
 
 	return get_addr_ref(insn, regs);
 }
@@ -390,11 +231,11 @@ int do_mpx_bt_fault(struct xsave_struct
 void do_mpx_bounds(struct pt_regs *regs, siginfo_t *info,
 		struct xsave_struct *xsave_buf)
 {
-	struct mpx_insn insn;
+	struct insn insn;
 	uint8_t bndregno;
 	unsigned long addr_vio;
 
-	addr_vio = mpx_insn_decode(&insn, regs);
+	addr_vio = insn_decode(&insn, regs);
 
 	bndregno = X86_MODRM_REG(insn.modrm.value);
 	if (bndregno > 3)
diff -puN arch/x86/include/asm/mpx.h~consolidate-instruction-decoding arch/x86/include/asm/mpx.h
--- a/arch/x86/include/asm/mpx.h~consolidate-instruction-decoding	2014-06-19 09:53:53.895441833 -0700
+++ b/arch/x86/include/asm/mpx.h	2014-06-19 09:53:53.909442460 -0700
@@ -53,20 +53,6 @@
 #define MPX_BNDCFG_ENABLE_FLAG	0x1
 #define MPX_BD_ENTRY_VALID_FLAG	0x1
 
-struct mpx_insn {
-	struct insn_field rex_prefix;	/* REX prefix */
-	struct insn_field modrm;
-	struct insn_field sib;
-	struct insn_field displacement;
-
-	unsigned char addr_bytes;	/* effective address size */
-	unsigned char limit;
-	unsigned char x86_64;
-
-	const unsigned char *kaddr;	/* kernel address of insn to analyze */
-	const unsigned char *next_byte;
-};
-
 #define MAX_MPX_INSN_SIZE	15
 
 unsigned long mpx_mmap(unsigned long len);
diff -puN arch/x86/include/asm/insn.h~consolidate-instruction-decoding arch/x86/include/asm/insn.h
--- a/arch/x86/include/asm/insn.h~consolidate-instruction-decoding	2014-06-19 09:53:53.897441922 -0700
+++ b/arch/x86/include/asm/insn.h	2014-06-19 09:53:53.910442505 -0700
@@ -98,6 +98,7 @@ struct insn {
 
 extern void insn_init(struct insn *insn, const void *kaddr, int x86_64);
 extern void insn_get_prefixes(struct insn *insn);
+extern void mpx_insn_get_prefixes(struct insn *insn);
 extern void insn_get_opcode(struct insn *insn);
 extern void insn_get_modrm(struct insn *insn);
 extern void insn_get_sib(struct insn *insn);
diff -puN arch/x86/lib/insn.c~consolidate-instruction-decoding arch/x86/lib/insn.c
--- a/arch/x86/lib/insn.c~consolidate-instruction-decoding	2014-06-19 09:53:53.899442011 -0700
+++ b/arch/x86/lib/insn.c	2014-06-19 09:53:53.910442505 -0700
@@ -63,6 +63,49 @@ void insn_init(struct insn *insn, const
 		insn->addr_bytes = 4;
 }
 
+void mpx_insn_get_prefixes(struct insn *insn)
+{
+	unsigned char b;
+
+	/* Decode legacy prefix and REX prefix */
+	b = peek_next(unsigned char, insn);
+	while (b != 0x0f) {
+		/*
+		 * look for a rex prefix
+		 * a REX prefix cannot be followed by a legacy prefix.
+		 */
+		if (insn->x86_64 && ((b&0xf0) == 0x40)) {
+			insn->prefixes.value = b;
+			insn->prefixes.nbytes = 1;
+			insn->next_byte++;
+			break;
+		}
+
+		/* check the other legacy prefixes */
+		switch (b) {
+		case 0xf2:
+		case 0xf3:
+		case 0xf0:
+		case 0x64:
+		case 0x65:
+		case 0x2e:
+		case 0x3e:
+		case 0x26:
+		case 0x36:
+		case 0x66:
+		case 0x67:
+			insn->next_byte++;
+			break;
+		default: /* everything else is garbage */
+			goto err_out;
+		}
+		b = peek_next(unsigned char, insn);
+	}
+
+err_out:
+	return;
+}
+
 /**
  * insn_get_prefixes - scan x86 instruction prefix bytes
  * @insn:	&struct insn containing instruction
_