[PATCH] KVM: x86: Generate #UD when memory operand is required

[PATCH] KVM: x86: Generate #UD when memory operand is required

[Nadav Amit]

Certain x86 instructions that use modrm operands only allow memory operand
(i.e., mod012), and cause a #UD exception otherwise. KVM ignores this fact.
Currently, the instructions that are such and are emulated by KVM are MOVBE,
MOVNTPS, MOVNTPD and MOVNTI.  MOVBE is the most blunt example, since it may be
emulated by the host regardless of MMIO.

The fix introduces a new group for handling such instructions, marking mod3 as
illegal instruction.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 6a57157..3817334 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -123,6 +123,7 @@
 #define Prefix      (3<<15)     /* Instruction varies with 66/f2/f3 prefix */
 #define RMExt       (4<<15)     /* Opcode extension in ModRM r/m if mod == 3 */
 #define Escape      (5<<15)     /* Escape to coprocessor instruction */
+#define InstrDual   (6<<15)     /* Alternate instruction decoding of mod == 3 */
 #define Sse         (1<<18)     /* SSE Vector instruction */
 /* Generic ModRM decode. */
 #define ModRM       (1<<19)
@@ -211,6 +212,7 @@ struct opcode {
 		const struct group_dual *gdual;
 		const struct gprefix *gprefix;
 		const struct escape *esc;
+		const struct instr_dual *idual;
 		void (*fastop)(struct fastop *fake);
 	} u;
 	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
@@ -233,6 +235,11 @@ struct escape {
 	struct opcode high[64];
 };
 
+struct instr_dual {
+	struct opcode mod012;
+	struct opcode mod3;
+};
+
 /* EFLAGS bit definitions. */
 #define EFLG_ID (1<<21)
 #define EFLG_VIP (1<<20)
@@ -3679,6 +3686,7 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
 #define EXT(_f, _e) { .flags = ((_f) | RMExt), .u.group = (_e) }
 #define G(_f, _g) { .flags = ((_f) | Group | ModRM), .u.group = (_g) }
 #define GD(_f, _g) { .flags = ((_f) | GroupDual | ModRM), .u.gdual = (_g) }
+#define ID(_f, _i) { .flags = ((_f) | InstrDual | ModRM), .u.idual = (_i) }
 #define E(_f, _e) { .flags = ((_f) | Escape | ModRM), .u.esc = (_e) }
 #define I(_f, _e) { .flags = (_f), .u.execute = (_e) }
 #define F(_f, _e) { .flags = (_f) | Fastop, .u.fastop = (_e) }
@@ -3840,8 +3848,12 @@ static const struct gprefix pfx_0f_6f_0f_7f = {
 	I(Mmx, em_mov), I(Sse | Aligned, em_mov), N, I(Sse | Unaligned, em_mov),
 };
 
+static const struct instr_dual instr_dual_0f_2b = {
+	I(0, em_mov), N
+};
+
 static const struct gprefix pfx_0f_2b = {
-	I(0, em_mov), I(0, em_mov), N, N,
+	ID(0, &instr_dual_0f_2b), ID(0, &instr_dual_0f_2b), N, N,
 };
 
 static const struct gprefix pfx_0f_28_0f_29 = {
@@ -3915,6 +3927,10 @@ static const struct escape escape_dd = { {
 	N, N, N, N, N, N, N, N,
 } };
 
+static const struct instr_dual instr_dual_0f_c3 = {
+	I(DstMem | SrcReg | ModRM | No16 | Mov, em_mov), N
+};
+
 static const struct opcode opcode_table[256] = {
 	/* 0x00 - 0x07 */
 	F6ALU(Lock, em_add),
@@ -4117,7 +4133,7 @@ static const struct opcode twobyte_table[256] = {
 	D(DstReg | SrcMem8 | ModRM | Mov), D(DstReg | SrcMem16 | ModRM | Mov),
 	/* 0xC0 - 0xC7 */
 	F2bv(DstMem | SrcReg | ModRM | SrcWrite | Lock, em_xadd),
-	N, I(DstMem | SrcReg | ModRM | No16 | Mov, em_mov),
+	N, ID(0, &instr_dual_0f_c3),
 	N, N, N, GD(0, &group9),
 	/* 0xC8 - 0xCF */
 	X8(I(DstReg, em_bswap)),
@@ -4130,12 +4146,20 @@ static const struct opcode twobyte_table[256] = {
 	N, N, N, N, N, N, N, N, N, N, N, N, N, N, N, N
 };
 
+static const struct instr_dual instr_dual_0f_38_f0 = {
+	I(DstReg | SrcMem | Mov, em_movbe), N
+};
+
+static const struct instr_dual instr_dual_0f_38_f1 = {
+	I(DstMem | SrcReg | Mov, em_movbe), N
+};
+
 static const struct gprefix three_byte_0f_38_f0 = {
-	I(DstReg | SrcMem | Mov, em_movbe), N, N, N
+	ID(0, &instr_dual_0f_38_f0), N, N, N
 };
 
 static const struct gprefix three_byte_0f_38_f1 = {
-	I(DstMem | SrcReg | Mov, em_movbe), N, N, N
+	ID(0, &instr_dual_0f_38_f1), N, N, N
 };
 
 /*
@@ -4536,6 +4560,12 @@ done_prefixes:
 			else
 				opcode = opcode.u.esc->op[(ctxt->modrm >> 3) & 7];
 			break;
+		case InstrDual:
+			if ((ctxt->modrm >> 6) == 3)
+				opcode = opcode.u.idual->mod3;
+			else
+				opcode = opcode.u.idual->mod012;
+			break;
 		default:
 			return EMULATION_FAILED;
 		}
-- 
1.9.1

[PATCH kvm-unit-tests] x86: Test illegal movbe

[Nadav Amit]

Previously KVM ignored the mod field of MOVBE instruction, so MOVBE from
register to register succeeds, although it should fail (cause a #UD exception).
This test check that a #UD is indeed delivered upon such MOVBE.

The test would not work if MOVBE is unsupported.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 x86/emulator.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/x86/emulator.c b/x86/emulator.c
index 5aa4dbf..709978b 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -1051,6 +1051,27 @@ static void test_simplealu(u32 *mem)
     report("test", *mem == 0x8400);
 }
 
+static void illegal_movbe_handler(struct ex_regs *regs)
+{
+	extern char bad_movbe_cont;
+
+	++exceptions;
+	regs->rip = (ulong)&bad_movbe_cont;
+}
+
+static void test_illegal_movbe(void)
+{
+	if (!(cpuid(1).c & (1 << 22)))
+		printf("SKIP: illegal movbe\n");
+
+	exceptions = 0;
+	handle_exception(UD_VECTOR, illegal_movbe_handler);
+	asm volatile(".byte 0x0f; .byte 0x38; .byte 0xf0; .byte 0xc0;\n\t"
+		     " bad_movbe_cont:" : : : "rax");
+	report("illegal movbe", exceptions == 1);
+	handle_exception(UD_VECTOR, 0);
+}
+
 int main()
 {
 	void *mem;
@@ -1119,6 +1140,7 @@ int main()
 	test_string_io_mmio(mem);
 
 	test_jmp_noncanonical(mem);
+	test_illegal_movbe();
 
 	return report_summary();
 }
-- 
1.9.1

Re: [PATCH] KVM: x86: Generate #UD when memory operand is required

[Paolo Bonzini]

On 26/11/2014 14:47, Nadav Amit wrote:
> Certain x86 instructions that use modrm operands only allow memory operand
> (i.e., mod012), and cause a #UD exception otherwise. KVM ignores this fact.
> Currently, the instructions that are such and are emulated by KVM are MOVBE,
> MOVNTPS, MOVNTPD and MOVNTI.  MOVBE is the most blunt example, since it may be
> emulated by the host regardless of MMIO.
> 
> The fix introduces a new group for handling such instructions, marking mod3 as
> illegal instruction.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 38 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 6a57157..3817334 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -123,6 +123,7 @@
>  #define Prefix      (3<<15)     /* Instruction varies with 66/f2/f3 prefix */
>  #define RMExt       (4<<15)     /* Opcode extension in ModRM r/m if mod == 3 */
>  #define Escape      (5<<15)     /* Escape to coprocessor instruction */
> +#define InstrDual   (6<<15)     /* Alternate instruction decoding of mod == 3 */
>  #define Sse         (1<<18)     /* SSE Vector instruction */
>  /* Generic ModRM decode. */
>  #define ModRM       (1<<19)
> @@ -211,6 +212,7 @@ struct opcode {
>  		const struct group_dual *gdual;
>  		const struct gprefix *gprefix;
>  		const struct escape *esc;
> +		const struct instr_dual *idual;
>  		void (*fastop)(struct fastop *fake);
>  	} u;
>  	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
> @@ -233,6 +235,11 @@ struct escape {
>  	struct opcode high[64];
>  };
>  
> +struct instr_dual {
> +	struct opcode mod012;
> +	struct opcode mod3;
> +};
> +
>  /* EFLAGS bit definitions. */
>  #define EFLG_ID (1<<21)
>  #define EFLG_VIP (1<<20)
> @@ -3679,6 +3686,7 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
>  #define EXT(_f, _e) { .flags = ((_f) | RMExt), .u.group = (_e) }
>  #define G(_f, _g) { .flags = ((_f) | Group | ModRM), .u.group = (_g) }
>  #define GD(_f, _g) { .flags = ((_f) | GroupDual | ModRM), .u.gdual = (_g) }
> +#define ID(_f, _i) { .flags = ((_f) | InstrDual | ModRM), .u.idual = (_i) }
>  #define E(_f, _e) { .flags = ((_f) | Escape | ModRM), .u.esc = (_e) }
>  #define I(_f, _e) { .flags = (_f), .u.execute = (_e) }
>  #define F(_f, _e) { .flags = (_f) | Fastop, .u.fastop = (_e) }
> @@ -3840,8 +3848,12 @@ static const struct gprefix pfx_0f_6f_0f_7f = {
>  	I(Mmx, em_mov), I(Sse | Aligned, em_mov), N, I(Sse | Unaligned, em_mov),
>  };
>  
> +static const struct instr_dual instr_dual_0f_2b = {
> +	I(0, em_mov), N
> +};
> +
>  static const struct gprefix pfx_0f_2b = {
> -	I(0, em_mov), I(0, em_mov), N, N,
> +	ID(0, &instr_dual_0f_2b), ID(0, &instr_dual_0f_2b), N, N,
>  };
>  
>  static const struct gprefix pfx_0f_28_0f_29 = {
> @@ -3915,6 +3927,10 @@ static const struct escape escape_dd = { {
>  	N, N, N, N, N, N, N, N,
>  } };
>  
> +static const struct instr_dual instr_dual_0f_c3 = {
> +	I(DstMem | SrcReg | ModRM | No16 | Mov, em_mov), N
> +};
> +
>  static const struct opcode opcode_table[256] = {
>  	/* 0x00 - 0x07 */
>  	F6ALU(Lock, em_add),
> @@ -4117,7 +4133,7 @@ static const struct opcode twobyte_table[256] = {
>  	D(DstReg | SrcMem8 | ModRM | Mov), D(DstReg | SrcMem16 | ModRM | Mov),
>  	/* 0xC0 - 0xC7 */
>  	F2bv(DstMem | SrcReg | ModRM | SrcWrite | Lock, em_xadd),
> -	N, I(DstMem | SrcReg | ModRM | No16 | Mov, em_mov),
> +	N, ID(0, &instr_dual_0f_c3),
>  	N, N, N, GD(0, &group9),
>  	/* 0xC8 - 0xCF */
>  	X8(I(DstReg, em_bswap)),
> @@ -4130,12 +4146,20 @@ static const struct opcode twobyte_table[256] = {
>  	N, N, N, N, N, N, N, N, N, N, N, N, N, N, N, N
>  };
>  
> +static const struct instr_dual instr_dual_0f_38_f0 = {
> +	I(DstReg | SrcMem | Mov, em_movbe), N
> +};
> +
> +static const struct instr_dual instr_dual_0f_38_f1 = {
> +	I(DstMem | SrcReg | Mov, em_movbe), N
> +};
> +
>  static const struct gprefix three_byte_0f_38_f0 = {
> -	I(DstReg | SrcMem | Mov, em_movbe), N, N, N
> +	ID(0, &instr_dual_0f_38_f0), N, N, N
>  };
>  
>  static const struct gprefix three_byte_0f_38_f1 = {
> -	I(DstMem | SrcReg | Mov, em_movbe), N, N, N
> +	ID(0, &instr_dual_0f_38_f1), N, N, N
>  };
>  
>  /*
> @@ -4536,6 +4560,12 @@ done_prefixes:
>  			else
>  				opcode = opcode.u.esc->op[(ctxt->modrm >> 3) & 7];
>  			break;
> +		case InstrDual:
> +			if ((ctxt->modrm >> 6) == 3)
> +				opcode = opcode.u.idual->mod3;
> +			else
> +				opcode = opcode.u.idual->mod012;
> +			break;
>  		default:
>  			return EMULATION_FAILED;
>  		}
> 

Thanks, looks good (not applied yet).

Paolo

Re: [PATCH kvm-unit-tests] x86: Test illegal movbe

[Paolo Bonzini]

On 26/11/2014 14:48, Nadav Amit wrote:
> Previously KVM ignored the mod field of MOVBE instruction, so MOVBE from
> register to register succeeds, although it should fail (cause a #UD exception).
> This test check that a #UD is indeed delivered upon such MOVBE.
> 
> The test would not work if MOVBE is unsupported.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  x86/emulator.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/x86/emulator.c b/x86/emulator.c
> index 5aa4dbf..709978b 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -1051,6 +1051,27 @@ static void test_simplealu(u32 *mem)
>      report("test", *mem == 0x8400);
>  }
>  
> +static void illegal_movbe_handler(struct ex_regs *regs)
> +{
> +	extern char bad_movbe_cont;
> +
> +	++exceptions;
> +	regs->rip = (ulong)&bad_movbe_cont;
> +}
> +
> +static void test_illegal_movbe(void)
> +{
> +	if (!(cpuid(1).c & (1 << 22)))
> +		printf("SKIP: illegal movbe\n");
> +
> +	exceptions = 0;
> +	handle_exception(UD_VECTOR, illegal_movbe_handler);
> +	asm volatile(".byte 0x0f; .byte 0x38; .byte 0xf0; .byte 0xc0;\n\t"
> +		     " bad_movbe_cont:" : : : "rax");
> +	report("illegal movbe", exceptions == 1);
> +	handle_exception(UD_VECTOR, 0);
> +}
> +
>  int main()
>  {
>  	void *mem;
> @@ -1119,6 +1140,7 @@ int main()
>  	test_string_io_mmio(mem);
>  
>  	test_jmp_noncanonical(mem);
> +	test_illegal_movbe();
>  
>  	return report_summary();
>  }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks,

Paolo

[PATCH kvm-unit-tests v2] x86: Test illegal movbe

[Nadav Amit]

Previously KVM ignored the mod field of MOVBE instruction, so MOVBE from
register to register succeeds, although it should fail (cause a #UD exception).
This test check that a #UD is indeed delivered upon such MOVBE.

The test would not work if MOVBE is unsupported.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
v1->v2: Add missing return upon no support for MOVBE
---
 x86/emulator.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/x86/emulator.c b/x86/emulator.c
index 5aa4dbf..567e421 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -1051,6 +1051,29 @@ static void test_simplealu(u32 *mem)
     report("test", *mem == 0x8400);
 }
 
+static void illegal_movbe_handler(struct ex_regs *regs)
+{
+	extern char bad_movbe_cont;
+
+	++exceptions;
+	regs->rip = (ulong)&bad_movbe_cont;
+}
+
+static void test_illegal_movbe(void)
+{
+	if (!(cpuid(1).c & (1 << 22))) {
+		printf("SKIP: illegal movbe\n");
+		return;
+	}
+
+	exceptions = 0;
+	handle_exception(UD_VECTOR, illegal_movbe_handler);
+	asm volatile(".byte 0x0f; .byte 0x38; .byte 0xf0; .byte 0xc0;\n\t"
+		     " bad_movbe_cont:" : : : "rax");
+	report("illegal movbe", exceptions == 1);
+	handle_exception(UD_VECTOR, 0);
+}
+
 int main()
 {
 	void *mem;
@@ -1119,6 +1142,7 @@ int main()
 	test_string_io_mmio(mem);
 
 	test_jmp_noncanonical(mem);
+	test_illegal_movbe();
 
 	return report_summary();
 }
-- 
1.9.1

Re: [PATCH] KVM: x86: Generate #UD when memory operand is required

[Radim Krčmář]

2014-11-26 15:47+0200, Nadav Amit:
> Certain x86 instructions that use modrm operands only allow memory operand
> (i.e., mod012), and cause a #UD exception otherwise. KVM ignores this fact.
> Currently, the instructions that are such and are emulated by KVM are MOVBE,
> MOVNTPS, MOVNTPD and MOVNTI.  MOVBE is the most blunt example, since it may be
> emulated by the host regardless of MMIO.
> 
> The fix introduces a new group for handling such instructions, marking mod3 as
> illegal instruction.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

(We could remove GroupDual in the future.)