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

[Paolo Bonzini <pbonzini@redhat.com>]

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