Re: [PATCH 07/21] KVM: x86: Emulator considers imm as memory operand

[Paolo Bonzini <pbonzini@redhat.com>]

On 02/11/2014 10:54, Nadav Amit wrote:
> The emulator mistakenly considers some of the immediate operands as memory
> operands, performs memory read and uses the wrong data.  By default, every
> operand is marked as OP_MEM, so if it is not changed, memory read may be
> wrongly emulated and the wrong value would be used.  Consider for instance the
> ROR instruction - src2 (the number of times) would be read from memory instead
> of being used as immediate.
> 
> Mark every immediate operand as such to avoid this problem.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f456783..e624d62 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4269,6 +4269,7 @@ static int decode_operand(struct x86_emulate_ctxt *ctxt, struct operand *op,
>  		fetch_register_operand(op);
>  		break;
>  	case OpCL:
> +		op->type = OP_IMM;
>  		op->bytes = 1;
>  		op->val = reg_read(ctxt, VCPU_REGS_RCX) & 0xff;
>  		break;
> @@ -4276,6 +4277,7 @@ static int decode_operand(struct x86_emulate_ctxt *ctxt, struct operand *op,
>  		rc = decode_imm(ctxt, op, 1, true);
>  		break;
>  	case OpOne:
> +		op->type = OP_IMM;
>  		op->bytes = 1;
>  		op->val = 1;
>  		break;
> @@ -4334,21 +4336,27 @@ static int decode_operand(struct x86_emulate_ctxt *ctxt, struct operand *op,
>  		ctxt->memop.bytes = ctxt->op_bytes + 2;
>  		goto mem_common;
>  	case OpES:
> +		op->type = OP_IMM;
>  		op->val = VCPU_SREG_ES;
>  		break;
>  	case OpCS:
> +		op->type = OP_IMM;
>  		op->val = VCPU_SREG_CS;
>  		break;
>  	case OpSS:
> +		op->type = OP_IMM;
>  		op->val = VCPU_SREG_SS;
>  		break;
>  	case OpDS:
> +		op->type = OP_IMM;
>  		op->val = VCPU_SREG_DS;
>  		break;
>  	case OpFS:
> +		op->type = OP_IMM;
>  		op->val = VCPU_SREG_FS;
>  		break;
>  	case OpGS:
> +		op->type = OP_IMM;
>  		op->val = VCPU_SREG_GS;
>  		break;
>  	case OpImplicit:
> 

I'm including this for stable@ because, until commit c44b4c6ab80e (KVM:
emulate: clean up initializations in init_decode_cache, 2014-04-16),
this would be harmless.  The type would be set to OP_REG instead of
OP_IMM, but it would not matter because no writeback is done for Src2
operands.

Now, the uninitialized op->type may be an OP_MEM from a previous
instruction.

Paolo