[PATCH 0/6] KVM: x86: More emulator bugs

[PATCH 0/6] KVM: x86: More emulator bugs

[Nadav Amit]

This patch-set resolves several emulator bugs. Each fix is independent of the
others.  The DR6/7 bug can occur during DR-access exit (regardless to
unrestricted mode, MMIO and SPT).

Thanks for reviewing the patches,
Nadav

Nadav Amit (6):
  KVM: x86: bit-ops emulation ignores offset on 64-bit
  KVM: x86: Wrong emulation on 'xadd X, X'
  KVM: x86: Inter privilage level ret emulation is not implemeneted
  KVM: x86: emulation of dword cmov on long-mode should clear [63:32]
  KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX
  KVM: x86: check DR6/7 high-bits are clear only on long-mode

 arch/x86/kvm/emulate.c | 31 ++++++++++++++++++++-----------
 arch/x86/kvm/x86.c     | 13 +++++++++++--
 2 files changed, 31 insertions(+), 13 deletions(-)

-- 
1.9.1

[PATCH 1/6] KVM: x86: bit-ops emulation ignores offset on 64-bit

[Nadav Amit]

The current emulation of bit operations ignores the offset from the destination
on 64-bit target memory operands. This patch fixes this behavior.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e4e833d..f0b0a10 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1220,12 +1220,14 @@ static void fetch_bit_operand(struct x86_emulate_ctxt *ctxt)
 	long sv = 0, mask;
 
 	if (ctxt->dst.type == OP_MEM && ctxt->src.type == OP_REG) {
-		mask = ~(ctxt->dst.bytes * 8 - 1);
+		mask = ~((long)ctxt->dst.bytes * 8 - 1);
 
 		if (ctxt->src.bytes == 2)
 			sv = (s16)ctxt->src.val & (s16)mask;
 		else if (ctxt->src.bytes == 4)
 			sv = (s32)ctxt->src.val & (s32)mask;
+		else
+			sv = (s64)ctxt->src.val & (s64)mask;
 
 		ctxt->dst.addr.mem.ea += (sv >> 3);
 	}
-- 
1.9.1

[PATCH 2/6] KVM: x86: Wrong emulation on 'xadd X, X'

[Nadav Amit]

The emulator does not emulate the xadd instruction correctly if the two
operands are the same.  In this (unlikely) situation the result should be the
sum of X and X (2X) when it is currently X.  The solution is to first perform
writeback to the source, before writing to the destination.  The only
instruction which should be affected is xadd, as the other instructions that
perform writeback to the source use the extended accumlator (e.g., RAX:RDX).

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f0b0a10..3c8d867 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4711,17 +4711,17 @@ special_insn:
 		goto done;
 
 writeback:
-	if (!(ctxt->d & NoWrite)) {
-		rc = writeback(ctxt, &ctxt->dst);
-		if (rc != X86EMUL_CONTINUE)
-			goto done;
-	}
 	if (ctxt->d & SrcWrite) {
 		BUG_ON(ctxt->src.type == OP_MEM || ctxt->src.type == OP_MEM_STR);
 		rc = writeback(ctxt, &ctxt->src);
 		if (rc != X86EMUL_CONTINUE)
 			goto done;
 	}
+	if (!(ctxt->d & NoWrite)) {
+		rc = writeback(ctxt, &ctxt->dst);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+	}
 
 	/*
 	 * restore dst type in case the decoding will be reused
-- 
1.9.1

Re: [PATCH 2/6] KVM: x86: Wrong emulation on 'xadd X, X'

[Bandan Das]

Nadav Amit <namit@cs.technion.ac.il> writes:

> The emulator does not emulate the xadd instruction correctly if the two
> operands are the same.  In this (unlikely) situation the result should be the
> sum of X and X (2X) when it is currently X.  The solution is to first perform
> writeback to the source, before writing to the destination.  The only
> instruction which should be affected is xadd, as the other instructions that
> perform writeback to the source use the extended accumlator (e.g., RAX:RDX).
>
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f0b0a10..3c8d867 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4711,17 +4711,17 @@ special_insn:
>  		goto done;
>  
>  writeback:
> -	if (!(ctxt->d & NoWrite)) {
> -		rc = writeback(ctxt, &ctxt->dst);
> -		if (rc != X86EMUL_CONTINUE)
> -			goto done;
> -	}
>  	if (ctxt->d & SrcWrite) {
>  		BUG_ON(ctxt->src.type == OP_MEM || ctxt->src.type == OP_MEM_STR);
While we are here, I think we should replace this BUG_ON with a warning
and return X86EMUL_UNHANDLEABLE if the condition is true.

>  		rc = writeback(ctxt, &ctxt->src);
>  		if (rc != X86EMUL_CONTINUE)
>  			goto done;
>  	}
> +	if (!(ctxt->d & NoWrite)) {
> +		rc = writeback(ctxt, &ctxt->dst);
> +		if (rc != X86EMUL_CONTINUE)
> +			goto done;
> +	}
>  
>  	/*
>  	 * restore dst type in case the decoding will be reused

Re: [PATCH 2/6] KVM: x86: Wrong emulation on 'xadd X, X'

[Paolo Bonzini]

Il 16/06/2014 19:38, Bandan Das ha scritto:
> Nadav Amit <namit@cs.technion.ac.il> writes:
>
>> The emulator does not emulate the xadd instruction correctly if the two
>> operands are the same.  In this (unlikely) situation the result should be the
>> sum of X and X (2X) when it is currently X.  The solution is to first perform
>> writeback to the source, before writing to the destination.  The only
>> instruction which should be affected is xadd, as the other instructions that
>> perform writeback to the source use the extended accumlator (e.g., RAX:RDX).
>>
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
>>  arch/x86/kvm/emulate.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index f0b0a10..3c8d867 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -4711,17 +4711,17 @@ special_insn:
>>  		goto done;
>>
>>  writeback:
>> -	if (!(ctxt->d & NoWrite)) {
>> -		rc = writeback(ctxt, &ctxt->dst);
>> -		if (rc != X86EMUL_CONTINUE)
>> -			goto done;
>> -	}
>>  	if (ctxt->d & SrcWrite) {
>>  		BUG_ON(ctxt->src.type == OP_MEM || ctxt->src.type == OP_MEM_STR);
> While we are here, I think we should replace this BUG_ON with a warning
> and return X86EMUL_UNHANDLEABLE if the condition is true.

Sure, please post a patch and I'll apply it right away.

Paolo

Re: [PATCH 2/6] KVM: x86: Wrong emulation on 'xadd X, X'

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 16/06/2014 19:38, Bandan Das ha scritto:
>> Nadav Amit <namit@cs.technion.ac.il> writes:
>>
>>> The emulator does not emulate the xadd instruction correctly if the two
>>> operands are the same.  In this (unlikely) situation the result should be the
>>> sum of X and X (2X) when it is currently X.  The solution is to first perform
>>> writeback to the source, before writing to the destination.  The only
>>> instruction which should be affected is xadd, as the other instructions that
>>> perform writeback to the source use the extended accumlator (e.g., RAX:RDX).
>>>
>>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>>> ---
>>>  arch/x86/kvm/emulate.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>>> index f0b0a10..3c8d867 100644
>>> --- a/arch/x86/kvm/emulate.c
>>> +++ b/arch/x86/kvm/emulate.c
>>> @@ -4711,17 +4711,17 @@ special_insn:
>>>  		goto done;
>>>
>>>  writeback:
>>> -	if (!(ctxt->d & NoWrite)) {
>>> -		rc = writeback(ctxt, &ctxt->dst);
>>> -		if (rc != X86EMUL_CONTINUE)
>>> -			goto done;
>>> -	}
>>>  	if (ctxt->d & SrcWrite) {
>>>  		BUG_ON(ctxt->src.type == OP_MEM || ctxt->src.type == OP_MEM_STR);
>> While we are here, I think we should replace this BUG_ON with a warning
>> and return X86EMUL_UNHANDLEABLE if the condition is true.
>
> Sure, please post a patch and I'll apply it right away.

Well, I meant it more as a review and "suggested changes" to this patchset 
Nadav posted, but yeah, if you prefer, I can post a change myself. I will
make a pass through other uses of BUG() in the code too. 

> Paolo
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] KVM: x86: Wrong emulation on 'xadd X, X'

[Paolo Bonzini]

Il 17/06/2014 17:35, Bandan Das ha scritto:
> Well, I meant it more as a review and "suggested changes" to this patchset
> Nadav posted, but yeah, if you prefer, I can post a change myself. I will
> make a pass through other uses of BUG() in the code too.

I'd prefer that, there's no need to make Nadav do even more work.

Paolo

[PATCH 3/6] KVM: x86: Inter privilage level ret emulation is not implemeneted

[Nadav Amit]

Return unhandlable error on inter-privilage level ret instruction.  This is
since the current emulation does not check the privilage level correctly when
loading the CS, and does not pop RSP/SS as needed.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3c8d867..0183350 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2019,6 +2019,7 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt)
 {
 	int rc;
 	unsigned long cs;
+	int cpl = ctxt->ops->cpl(ctxt);
 
 	rc = emulate_pop(ctxt, &ctxt->_eip, ctxt->op_bytes);
 	if (rc != X86EMUL_CONTINUE)
@@ -2028,6 +2029,9 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt)
 	rc = emulate_pop(ctxt, &cs, ctxt->op_bytes);
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
+	/* Outer-privilage level return is not implemented */
+	if (ctxt->mode >= X86EMUL_MODE_PROT16 && (cs & 3) > cpl)
+		return X86EMUL_UNHANDLEABLE;
 	rc = load_segment_descriptor(ctxt, (u16)cs, VCPU_SREG_CS);
 	return rc;
 }
-- 
1.9.1

[PATCH 4/6] KVM: x86: emulation of dword cmov on long-mode should clear [63:32]

[Nadav Amit]

Even if the condition of cmov is not satisfied, bits[63:32] should be cleared.
This is clearly stated in Intel's CMOVcc documentation.  The solution is to
reassign the destination onto itself if the condition is unsatisfied.  For that
matter the original destination value needs to be read.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 0183350..b354531 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3905,7 +3905,7 @@ static const struct opcode twobyte_table[256] = {
 	N, N,
 	N, N, N, N, N, N, N, N,
 	/* 0x40 - 0x4F */
-	X16(D(DstReg | SrcMem | ModRM | Mov)),
+	X16(D(DstReg | SrcMem | ModRM)),
 	/* 0x50 - 0x5F */
 	N, N, N, N, N, N, N, N, N, N, N, N, N, N, N, N,
 	/* 0x60 - 0x6F */
@@ -4799,8 +4799,10 @@ twobyte_insn:
 		ops->get_dr(ctxt, ctxt->modrm_reg, &ctxt->dst.val);
 		break;
 	case 0x40 ... 0x4f:	/* cmov */
-		ctxt->dst.val = ctxt->dst.orig_val = ctxt->src.val;
-		if (!test_cc(ctxt->b, ctxt->eflags))
+		if (test_cc(ctxt->b, ctxt->eflags))
+			ctxt->dst.val = ctxt->src.val;
+		else if (ctxt->mode != X86EMUL_MODE_PROT64 ||
+			 ctxt->op_bytes != 4)
 			ctxt->dst.type = OP_NONE; /* no writeback */
 		break;
 	case 0x80 ... 0x8f: /* jnz rel, etc*/
-- 
1.9.1

[PATCH 5/6] KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX

[Nadav Amit]

On long-mode the current NOP (0x90) emulation still writes back to RAX.  As a
result, EAX is zero-extended and the high 32-bits of RAX are cleared.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b354531..eb93eb4 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4663,8 +4663,9 @@ special_insn:
 		break;
 	case 0x90 ... 0x97: /* nop / xchg reg, rax */
 		if (ctxt->dst.addr.reg == reg_rmw(ctxt, VCPU_REGS_RAX))
-			break;
-		rc = em_xchg(ctxt);
+			ctxt->dst.type = OP_NONE;
+		else
+			rc = em_xchg(ctxt);
 		break;
 	case 0x98: /* cbw/cwde/cdqe */
 		switch (ctxt->op_bytes) {
-- 
1.9.1

[PATCH 6/6] KVM: x86: check DR6/7 high-bits are clear only on long-mode

[Nadav Amit]

From: Nadav Amit <nadav.amit@gmail.com>

When the guest sets DR6 and DR7, KVM asserts the high 32-bits are clear, and
otherwise injects a #GP exception. This exception should only be injected only
if running in long-mode.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 57eac30..71fe841 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -756,6 +756,15 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu)
 		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED;
 }
 
+static bool is_64_bit_mode(struct kvm_vcpu *vcpu)
+{
+	int cs_db, cs_l;
+	if (!is_long_mode(vcpu))
+		return false;
+	kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
+	return cs_l;
+}
+
 static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
 {
 	switch (dr) {
@@ -769,7 +778,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
 			return 1; /* #UD */
 		/* fall through */
 	case 6:
-		if (val & 0xffffffff00000000ULL)
+		if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
 			return -1; /* #GP */
 		vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
 		kvm_update_dr6(vcpu);
@@ -779,7 +788,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
 			return 1; /* #UD */
 		/* fall through */
 	default: /* 7 */
-		if (val & 0xffffffff00000000ULL)
+		if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
 			return -1; /* #GP */
 		vcpu->arch.dr7 = (val & DR7_VOLATILE) | DR7_FIXED_1;
 		kvm_update_dr7(vcpu);
-- 
1.9.1

Re: [PATCH 6/6] KVM: x86: check DR6/7 high-bits are clear only on long-mode

[Paolo Bonzini]

Il 15/06/2014 15:13, Nadav Amit ha scritto:
> From: Nadav Amit <nadav.amit@gmail.com>
>
> When the guest sets DR6 and DR7, KVM asserts the high 32-bits are clear, and
> otherwise injects a #GP exception. This exception should only be injected only
> if running in long-mode.
>
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/x86.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 57eac30..71fe841 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -756,6 +756,15 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu)
>  		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED;
>  }
>
> +static bool is_64_bit_mode(struct kvm_vcpu *vcpu)
> +{
> +	int cs_db, cs_l;
> +	if (!is_long_mode(vcpu))
> +		return false;
> +	kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
> +	return cs_l;
> +}
> +
>  static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
>  {
>  	switch (dr) {
> @@ -769,7 +778,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
>  			return 1; /* #UD */
>  		/* fall through */
>  	case 6:
> -		if (val & 0xffffffff00000000ULL)
> +		if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
>  			return -1; /* #GP */
>  		vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
>  		kvm_update_dr6(vcpu);
> @@ -779,7 +788,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
>  			return 1; /* #UD */
>  		/* fall through */
>  	default: /* 7 */
> -		if (val & 0xffffffff00000000ULL)
> +		if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
>  			return -1; /* #GP */
>  		vcpu->arch.dr7 = (val & DR7_VOLATILE) | DR7_FIXED_1;
>  		kvm_update_dr7(vcpu);
>

Do you get this if the input register has bit 31 set?

Paolo

Re: [PATCH 6/6] KVM: x86: check DR6/7 high-bits are clear only on long-mode

[Nadav Amit]

On 6/16/14, 1:17 PM, Paolo Bonzini wrote:
> Il 15/06/2014 15:13, Nadav Amit ha scritto:
>> From: Nadav Amit <nadav.amit@gmail.com>
>>
>> When the guest sets DR6 and DR7, KVM asserts the high 32-bits are
>> clear, and
>> otherwise injects a #GP exception. This exception should only be
>> injected only
>> if running in long-mode.
>>
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
>>  arch/x86/kvm/x86.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 57eac30..71fe841 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -756,6 +756,15 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu)
>>          vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED;
>>  }
>>
>> +static bool is_64_bit_mode(struct kvm_vcpu *vcpu)
>> +{
>> +    int cs_db, cs_l;
>> +    if (!is_long_mode(vcpu))
>> +        return false;
>> +    kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
>> +    return cs_l;
>> +}
>> +
>>  static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long
>> val)
>>  {
>>      switch (dr) {
>> @@ -769,7 +778,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int
>> dr, unsigned long val)
>>              return 1; /* #UD */
>>          /* fall through */
>>      case 6:
>> -        if (val & 0xffffffff00000000ULL)
>> +        if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
>>              return -1; /* #GP */
>>          vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
>>          kvm_update_dr6(vcpu);
>> @@ -779,7 +788,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int
>> dr, unsigned long val)
>>              return 1; /* #UD */
>>          /* fall through */
>>      default: /* 7 */
>> -        if (val & 0xffffffff00000000ULL)
>> +        if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu))
>>              return -1; /* #GP */
>>          vcpu->arch.dr7 = (val & DR7_VOLATILE) | DR7_FIXED_1;
>>          kvm_update_dr7(vcpu);
>>
>
> Do you get this if the input register has bit 31 set?
No. To be frank, the scenario may be considered a bit synthetic: the 
guest assigns a value to a general-purpose register in 64-bit mode, 
setting the high 32-bits to some non-zero value. Then, later, in 32-bit 
mode, the guest performs MOV DR instruction. In between the two 
assignments, the general purpose register is unmodified, so the high 
32-bits of the general purpose registers are still set.

Note that this scenario does not occur when MOV DR is emulated, but when 
handle_dr() is called. In this case, the entire 64-bits of the general 
purpose register used for MOV DR are read, regardless to the execution 
mode of the guest.

Nadav

Re: [PATCH 6/6] KVM: x86: check DR6/7 high-bits are clear only on long-mode

[Paolo Bonzini]

Il 16/06/2014 12:33, Nadav Amit ha scritto:
>>
>> Do you get this if the input register has bit 31 set?
> No. To be frank, the scenario may be considered a bit synthetic: the
> guest assigns a value to a general-purpose register in 64-bit mode,
> setting the high 32-bits to some non-zero value. Then, later, in 32-bit
> mode, the guest performs MOV DR instruction. In between the two
> assignments, the general purpose register is unmodified, so the high
> 32-bits of the general purpose registers are still set.
>
> Note that this scenario does not occur when MOV DR is emulated, but when
> handle_dr() is called. In this case, the entire 64-bits of the general
> purpose register used for MOV DR are read, regardless to the execution
> mode of the guest.

I wonder if the same bug happens elsewhere.  For example, 
kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a 
corner case but arguably also a bug.  kvm_hv_hypercall instead does it 
right.

Perhaps we need a variant of kvm_register_read that (on 64-bit hosts) 
checks EFER/CS.L/CS.DB and masks the returned value accordingly.  You 
could call it kvm_register_readl.

Paolo

Re: [PATCH 6/6] KVM: x86: check DR6/7 high-bits are clear only on long-mode

[Nadav Amit]

On 6/16/14, 2:09 PM, Paolo Bonzini wrote:
> Il 16/06/2014 12:33, Nadav Amit ha scritto:
>>>
>>> Do you get this if the input register has bit 31 set?
>> No. To be frank, the scenario may be considered a bit synthetic: the
>> guest assigns a value to a general-purpose register in 64-bit mode,
>> setting the high 32-bits to some non-zero value. Then, later, in 32-bit
>> mode, the guest performs MOV DR instruction. In between the two
>> assignments, the general purpose register is unmodified, so the high
>> 32-bits of the general purpose registers are still set.
>>
>> Note that this scenario does not occur when MOV DR is emulated, but when
>> handle_dr() is called. In this case, the entire 64-bits of the general
>> purpose register used for MOV DR are read, regardless to the execution
>> mode of the guest.
>
> I wonder if the same bug happens elsewhere.  For example,
> kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a
> corner case but arguably also a bug.  kvm_hv_hypercall instead does it
> right.
>
> Perhaps we need a variant of kvm_register_read that (on 64-bit hosts)
> checks EFER/CS.L/CS.DB and masks the returned value accordingly.  You
> could call it kvm_register_readl.

There are two questions that come in mind:
1. Should we ignore CS.DB? It would make it consistent with 
kvm_hv_hypercall and handle_dr. I think this is the proper behavior.

2. Reading CS.L once and masking all the registers (i.e., changing the 
is_long_mode in kvm_emulate_hypercall to is_64_bit_mode) is likely to be 
more efficient.

Regards,
Nadav

Re: [PATCH 6/6] KVM: x86: check DR6/7 high-bits are clear only on long-mode

[Paolo Bonzini]

Il 16/06/2014 13:53, Nadav Amit ha scritto:
> On 6/16/14, 2:09 PM, Paolo Bonzini wrote:
>> Il 16/06/2014 12:33, Nadav Amit ha scritto:
>>>>
>>>> Do you get this if the input register has bit 31 set?
>>> No. To be frank, the scenario may be considered a bit synthetic: the
>>> guest assigns a value to a general-purpose register in 64-bit mode,
>>> setting the high 32-bits to some non-zero value. Then, later, in 32-bit
>>> mode, the guest performs MOV DR instruction. In between the two
>>> assignments, the general purpose register is unmodified, so the high
>>> 32-bits of the general purpose registers are still set.
>>>
>>> Note that this scenario does not occur when MOV DR is emulated, but when
>>> handle_dr() is called. In this case, the entire 64-bits of the general
>>> purpose register used for MOV DR are read, regardless to the execution
>>> mode of the guest.
>>
>> I wonder if the same bug happens elsewhere.  For example,
>> kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a
>> corner case but arguably also a bug.  kvm_hv_hypercall instead does it
>> right.
>>
>> Perhaps we need a variant of kvm_register_read that (on 64-bit hosts)
>> checks EFER/CS.L/CS.DB and masks the returned value accordingly.  You
>> could call it kvm_register_readl.
>
> There are two questions that come in mind:
> 1. Should we ignore CS.DB? It would make it consistent with
> kvm_hv_hypercall and handle_dr. I think this is the proper behavior.

It depends on what you're using it for, but as a start yes.

> 2. Reading CS.L once and masking all the registers (i.e., changing the
> is_long_mode in kvm_emulate_hypercall to is_64_bit_mode) is likely to be
> more efficient.

Yes, for the case of kvm_emulate_hypercall.  Then you can build 
kvm_register_readl on top of is_64bit_mode and fix this bug with that 
function.  Did you check that handle_cr is unaffected?

Paolo

Re: [PATCH 6/6] KVM: x86: check DR6/7 high-bits are clear only on long-mode

[Nadav Amit]

On 6/16/14, 5:56 PM, Paolo Bonzini wrote:
> Il 16/06/2014 13:53, Nadav Amit ha scritto:
>> On 6/16/14, 2:09 PM, Paolo Bonzini wrote:
>>> Il 16/06/2014 12:33, Nadav Amit ha scritto:
>>>>>
>>>>> Do you get this if the input register has bit 31 set?
>>>> No. To be frank, the scenario may be considered a bit synthetic: the
>>>> guest assigns a value to a general-purpose register in 64-bit mode,
>>>> setting the high 32-bits to some non-zero value. Then, later, in 32-bit
>>>> mode, the guest performs MOV DR instruction. In between the two
>>>> assignments, the general purpose register is unmodified, so the high
>>>> 32-bits of the general purpose registers are still set.
>>>>
>>>> Note that this scenario does not occur when MOV DR is emulated, but
>>>> when
>>>> handle_dr() is called. In this case, the entire 64-bits of the general
>>>> purpose register used for MOV DR are read, regardless to the execution
>>>> mode of the guest.
>>>
>>> I wonder if the same bug happens elsewhere.  For example,
>>> kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a
>>> corner case but arguably also a bug.  kvm_hv_hypercall instead does it
>>> right.
>>>
>>> Perhaps we need a variant of kvm_register_read that (on 64-bit hosts)
>>> checks EFER/CS.L/CS.DB and masks the returned value accordingly.  You
>>> could call it kvm_register_readl.
>>
>> There are two questions that come in mind:
>> 1. Should we ignore CS.DB? It would make it consistent with
>> kvm_hv_hypercall and handle_dr. I think this is the proper behavior.
>
> It depends on what you're using it for, but as a start yes.
>
>> 2. Reading CS.L once and masking all the registers (i.e., changing the
>> is_long_mode in kvm_emulate_hypercall to is_64_bit_mode) is likely to be
>> more efficient.
>
> Yes, for the case of kvm_emulate_hypercall.  Then you can build
> kvm_register_readl on top of is_64bit_mode and fix this bug with that
> function.  Did you check that handle_cr is unaffected?

handle_cr is affected, and there are some other instances of affected 
register reads. Actually, most of the vmx instruction exit handling 
routines ignore long-mode and cs.l when considering the register operand 
size.

I'll make the necessary changes and resubmit.

Nadav

[PATCH v2 0/9] KVM: x86: More emulator bugs

[Nadav Amit]

This patch-set resolves several emulator bugs. Each fix is independent of the
others.  The DR6 bug can occur during DR-access exit (regardless to 
unrestricted mode, MMIO and SPT).


Changes in v2:

Introduced kvm_register_readl and kvm_register_writel which consider long-mode
and cs.l when reading the registers.

Fixing the register read to respect 32/64 bit in hypercall handling, CR exit
handling and VMX instructions handling.

Thanks for re-reviewing the patch

Nadav Amit (9):
  KVM: x86: bit-ops emulation ignores offset on 64-bit
  KVM: x86: Wrong emulation on 'xadd X, X'
  KVM: x86: Inter privilage level ret emulation is not implemeneted
  KVM: x86: emulation of dword cmov on long-mode should clear [63:32]
  KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX
  KVM: x86: check DR6/7 high-bits are clear only on long-mode
  KVM: x86: Hypercall handling does not considers opsize correctly
  KVM: vmx: handle_cr ignores 32/64-bit mode
  KVM: vmx: vmx instructions handling does not consider cs.l

 arch/x86/kvm/emulate.c | 31 ++++++++++++++++++++-----------
 arch/x86/kvm/vmx.c     | 16 ++++++++--------
 arch/x86/kvm/x86.c     | 11 ++++++-----
 arch/x86/kvm/x86.h     | 27 +++++++++++++++++++++++++++
 4 files changed, 61 insertions(+), 24 deletions(-)

-- 
1.9.1

[PATCH v2 1/9] KVM: x86: bit-ops emulation ignores offset on 64-bit

[Nadav Amit]

The current emulation of bit operations ignores the offset from the destination
on 64-bit target memory operands. This patch fixes this behavior.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e4e833d..f0b0a10 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1220,12 +1220,14 @@ static void fetch_bit_operand(struct x86_emulate_ctxt *ctxt)
 	long sv = 0, mask;
 
 	if (ctxt->dst.type == OP_MEM && ctxt->src.type == OP_REG) {
-		mask = ~(ctxt->dst.bytes * 8 - 1);
+		mask = ~((long)ctxt->dst.bytes * 8 - 1);
 
 		if (ctxt->src.bytes == 2)
 			sv = (s16)ctxt->src.val & (s16)mask;
 		else if (ctxt->src.bytes == 4)
 			sv = (s32)ctxt->src.val & (s32)mask;
+		else
+			sv = (s64)ctxt->src.val & (s64)mask;
 
 		ctxt->dst.addr.mem.ea += (sv >> 3);
 	}
-- 
1.9.1

[PATCH v2 2/9] KVM: x86: Wrong emulation on 'xadd X, X'

[Nadav Amit]

The emulator does not emulate the xadd instruction correctly if the two
operands are the same.  In this (unlikely) situation the result should be the
sum of X and X (2X) when it is currently X.  The solution is to first perform
writeback to the source, before writing to the destination.  The only
instruction which should be affected is xadd, as the other instructions that
perform writeback to the source use the extended accumlator (e.g., RAX:RDX).

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f0b0a10..3c8d867 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4711,17 +4711,17 @@ special_insn:
 		goto done;
 
 writeback:
-	if (!(ctxt->d & NoWrite)) {
-		rc = writeback(ctxt, &ctxt->dst);
-		if (rc != X86EMUL_CONTINUE)
-			goto done;
-	}
 	if (ctxt->d & SrcWrite) {
 		BUG_ON(ctxt->src.type == OP_MEM || ctxt->src.type == OP_MEM_STR);
 		rc = writeback(ctxt, &ctxt->src);
 		if (rc != X86EMUL_CONTINUE)
 			goto done;
 	}
+	if (!(ctxt->d & NoWrite)) {
+		rc = writeback(ctxt, &ctxt->dst);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
+	}
 
 	/*
 	 * restore dst type in case the decoding will be reused
-- 
1.9.1

[PATCH v2 3/9] KVM: x86: Inter privilage level ret emulation is not implemeneted

[Nadav Amit]

Return unhandlable error on inter-privilage level ret instruction.  This is
since the current emulation does not check the privilage level correctly when
loading the CS, and does not pop RSP/SS as needed.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3c8d867..0183350 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2019,6 +2019,7 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt)
 {
 	int rc;
 	unsigned long cs;
+	int cpl = ctxt->ops->cpl(ctxt);
 
 	rc = emulate_pop(ctxt, &ctxt->_eip, ctxt->op_bytes);
 	if (rc != X86EMUL_CONTINUE)
@@ -2028,6 +2029,9 @@ static int em_ret_far(struct x86_emulate_ctxt *ctxt)
 	rc = emulate_pop(ctxt, &cs, ctxt->op_bytes);
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
+	/* Outer-privilage level return is not implemented */
+	if (ctxt->mode >= X86EMUL_MODE_PROT16 && (cs & 3) > cpl)
+		return X86EMUL_UNHANDLEABLE;
 	rc = load_segment_descriptor(ctxt, (u16)cs, VCPU_SREG_CS);
 	return rc;
 }
-- 
1.9.1

[PATCH v2 4/9] KVM: x86: emulation of dword cmov on long-mode should clear [63:32]

[Nadav Amit]

Even if the condition of cmov is not satisfied, bits[63:32] should be cleared.
This is clearly stated in Intel's CMOVcc documentation.  The solution is to
reassign the destination onto itself if the condition is unsatisfied.  For that
matter the original destination value needs to be read.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 0183350..b354531 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3905,7 +3905,7 @@ static const struct opcode twobyte_table[256] = {
 	N, N,
 	N, N, N, N, N, N, N, N,
 	/* 0x40 - 0x4F */
-	X16(D(DstReg | SrcMem | ModRM | Mov)),
+	X16(D(DstReg | SrcMem | ModRM)),
 	/* 0x50 - 0x5F */
 	N, N, N, N, N, N, N, N, N, N, N, N, N, N, N, N,
 	/* 0x60 - 0x6F */
@@ -4799,8 +4799,10 @@ twobyte_insn:
 		ops->get_dr(ctxt, ctxt->modrm_reg, &ctxt->dst.val);
 		break;
 	case 0x40 ... 0x4f:	/* cmov */
-		ctxt->dst.val = ctxt->dst.orig_val = ctxt->src.val;
-		if (!test_cc(ctxt->b, ctxt->eflags))
+		if (test_cc(ctxt->b, ctxt->eflags))
+			ctxt->dst.val = ctxt->src.val;
+		else if (ctxt->mode != X86EMUL_MODE_PROT64 ||
+			 ctxt->op_bytes != 4)
 			ctxt->dst.type = OP_NONE; /* no writeback */
 		break;
 	case 0x80 ... 0x8f: /* jnz rel, etc*/
-- 
1.9.1

[PATCH v2 5/9] KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX

[Nadav Amit]

On long-mode the current NOP (0x90) emulation still writes back to RAX.  As a
result, EAX is zero-extended and the high 32-bits of RAX are cleared.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b354531..eb93eb4 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4663,8 +4663,9 @@ special_insn:
 		break;
 	case 0x90 ... 0x97: /* nop / xchg reg, rax */
 		if (ctxt->dst.addr.reg == reg_rmw(ctxt, VCPU_REGS_RAX))
-			break;
-		rc = em_xchg(ctxt);
+			ctxt->dst.type = OP_NONE;
+		else
+			rc = em_xchg(ctxt);
 		break;
 	case 0x98: /* cbw/cwde/cdqe */
 		switch (ctxt->op_bytes) {
-- 
1.9.1

[PATCH v2 6/9] KVM: x86: check DR6/7 high-bits are clear only on long-mode

[Nadav Amit]

From: Nadav Amit <nadav.amit@gmail.com>

When the guest sets DR6 and DR7, KVM asserts the high 32-bits are clear, and
otherwise injects a #GP exception. This exception should only be injected only
if running in long-mode.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/vmx.c |  2 +-
 arch/x86/kvm/x86.h | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 801332e..c0f53a0 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5165,7 +5165,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 			return 1;
 		kvm_register_write(vcpu, reg, val);
 	} else
-		if (kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg)))
+		if (kvm_set_dr(vcpu, dr, kvm_register_readl(vcpu, reg)))
 			return 1;
 
 	skip_emulated_instruction(vcpu);
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 8c97bac..c5b61a7 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -47,6 +47,16 @@ static inline int is_long_mode(struct kvm_vcpu *vcpu)
 #endif
 }
 
+static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)
+{
+	int cs_db, cs_l;
+
+	if (!is_long_mode(vcpu))
+		return false;
+	kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
+	return cs_l;
+}
+
 static inline bool mmu_is_nested(struct kvm_vcpu *vcpu)
 {
 	return vcpu->arch.walk_mmu == &vcpu->arch.nested_mmu;
@@ -108,6 +118,14 @@ static inline bool vcpu_match_mmio_gpa(struct kvm_vcpu *vcpu, gpa_t gpa)
 	return false;
 }
 
+static inline unsigned long kvm_register_readl(struct kvm_vcpu *vcpu,
+					       enum kvm_reg reg)
+{
+	unsigned long val = kvm_register_read(vcpu, reg);
+
+	return is_64_bit_mode(vcpu) ? val : (u32)val;
+}
+
 void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
 void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
 int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
-- 
1.9.1

[PATCH v2 7/9] KVM: x86: Hypercall handling does not considers opsize correctly

[Nadav Amit]

Currently, the hypercall handling routine only considers LME as an indication
to whether the guest uses 32/64-bit mode. This is incosistent with hyperv
hypercalls handling and against the common sense of considering cs.l as well.
This patch uses is_64_bit_mode instead of is_long_mode for that matter. In
addition, the result is masked in respect to the guest execution mode. Last, it
changes kvm_hv_hypercall to use is_64_bit_mode as well to simplify the code.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f32a025..c6dfe47 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5662,7 +5662,6 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
 	u64 param, ingpa, outgpa, ret;
 	uint16_t code, rep_idx, rep_cnt, res = HV_STATUS_SUCCESS, rep_done = 0;
 	bool fast, longmode;
-	int cs_db, cs_l;
 
 	/*
 	 * hypercall generates UD from non zero cpl and real mode
@@ -5673,8 +5672,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
 		return 0;
 	}
 
-	kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
-	longmode = is_long_mode(vcpu) && cs_l == 1;
+	longmode = is_64_bit_mode(vcpu);
 
 	if (!longmode) {
 		param = ((u64)kvm_register_read(vcpu, VCPU_REGS_RDX) << 32) |
@@ -5739,7 +5737,7 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, unsigned long flags, int apicid)
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 {
 	unsigned long nr, a0, a1, a2, a3, ret;
-	int r = 1;
+	int op_64_bit, r = 1;
 
 	if (kvm_hv_hypercall_enabled(vcpu->kvm))
 		return kvm_hv_hypercall(vcpu);
@@ -5752,7 +5750,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 
 	trace_kvm_hypercall(nr, a0, a1, a2, a3);
 
-	if (!is_long_mode(vcpu)) {
+	op_64_bit = is_64_bit_mode(vcpu);
+	if (!op_64_bit) {
 		nr &= 0xFFFFFFFF;
 		a0 &= 0xFFFFFFFF;
 		a1 &= 0xFFFFFFFF;
@@ -5778,6 +5777,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 		break;
 	}
 out:
+	if (!op_64_bit)
+		ret = (u32)ret;
 	kvm_register_write(vcpu, VCPU_REGS_RAX, ret);
 	++vcpu->stat.hypercalls;
 	return r;
-- 
1.9.1

[PATCH v2 8/9] KVM: vmx: handle_cr ignores 32/64-bit mode

[Nadav Amit]

On 32-bit mode only bits [31:0] of the CR should be used for setting the CR
value.  Otherwise, the host may incorrectly assume the value is invalid if bits
[63:32] are not zero.  Moreover, the CR is currently being read twice when CR8
is used.  Last, nested mov-cr exiting is modified to handle the CR value
correctly as well.

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

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c0f53a0..cbfbb8b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5039,7 +5039,7 @@ static int handle_cr(struct kvm_vcpu *vcpu)
 	reg = (exit_qualification >> 8) & 15;
 	switch ((exit_qualification >> 4) & 3) {
 	case 0: /* mov to cr */
-		val = kvm_register_read(vcpu, reg);
+		val = kvm_register_readl(vcpu, reg);
 		trace_kvm_cr_write(cr, val);
 		switch (cr) {
 		case 0:
@@ -5056,7 +5056,7 @@ static int handle_cr(struct kvm_vcpu *vcpu)
 			return 1;
 		case 8: {
 				u8 cr8_prev = kvm_get_cr8(vcpu);
-				u8 cr8 = kvm_register_read(vcpu, reg);
+				u8 cr8 = (u8)val;
 				err = kvm_set_cr8(vcpu, cr8);
 				kvm_complete_insn_gp(vcpu, err);
 				if (irqchip_in_kernel(vcpu->kvm))
@@ -6751,7 +6751,7 @@ static bool nested_vmx_exit_handled_cr(struct kvm_vcpu *vcpu,
 	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
 	int cr = exit_qualification & 15;
 	int reg = (exit_qualification >> 8) & 15;
-	unsigned long val = kvm_register_read(vcpu, reg);
+	unsigned long val = kvm_register_readl(vcpu, reg);
 
 	switch ((exit_qualification >> 4) & 3) {
 	case 0: /* mov to cr */
-- 
1.9.1

[PATCH v2 9/9] KVM: vmx: vmx instructions handling does not consider cs.l

[Nadav Amit]

VMX instructions use 32-bit operands in 32-bit mode, and 64-bit operands in
64-bit mode.  The current implementation is broken since it does not use the
register operands correctly, and always uses 64-bit for reads and writes.
Moreover, write to memory in vmwrite only considers long-mode, so it ignores
cs.l. This patch fixes this behavior.  The field of vmread/vmwrite is kept
intentionally as 64-bit read since if bits [63:32] are not cleared the
instruction should fail, according to Intel SDM.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/vmx.c | 8 ++++----
 arch/x86/kvm/x86.h | 9 +++++++++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index cbfbb8b..75dc888 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6397,7 +6397,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
 	 * on the guest's mode (32 or 64 bit), not on the given field's length.
 	 */
 	if (vmx_instruction_info & (1u << 10)) {
-		kvm_register_write(vcpu, (((vmx_instruction_info) >> 3) & 0xf),
+		kvm_register_writel(vcpu, (((vmx_instruction_info) >> 3) & 0xf),
 			field_value);
 	} else {
 		if (get_vmx_mem_address(vcpu, exit_qualification,
@@ -6434,14 +6434,14 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
 		return 1;
 
 	if (vmx_instruction_info & (1u << 10))
-		field_value = kvm_register_read(vcpu,
+		field_value = kvm_register_readl(vcpu,
 			(((vmx_instruction_info) >> 3) & 0xf));
 	else {
 		if (get_vmx_mem_address(vcpu, exit_qualification,
 				vmx_instruction_info, &gva))
 			return 1;
 		if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva,
-			   &field_value, (is_long_mode(vcpu) ? 8 : 4), &e)) {
+			   &field_value, (is_64_bit_mode(vcpu) ? 8 : 4), &e)) {
 			kvm_inject_page_fault(vcpu, &e);
 			return 1;
 		}
@@ -6571,7 +6571,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 	}
 
 	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-	type = kvm_register_read(vcpu, (vmx_instruction_info >> 28) & 0xf);
+	type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf);
 
 	types = (nested_vmx_ept_caps >> VMX_EPT_EXTENT_SHIFT) & 6;
 
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index c5b61a7..306a1b7 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -126,6 +126,15 @@ static inline unsigned long kvm_register_readl(struct kvm_vcpu *vcpu,
 	return is_64_bit_mode(vcpu) ? val : (u32)val;
 }
 
+static inline void kvm_register_writel(struct kvm_vcpu *vcpu,
+				       enum kvm_reg reg,
+				       unsigned long val)
+{
+	if (!is_64_bit_mode(vcpu))
+		val = (u32)val;
+	return kvm_register_write(vcpu, reg, val);
+}
+
 void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
 void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
 int kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
-- 
1.9.1

Re: [PATCH v2 9/9] KVM: vmx: vmx instructions handling does not consider cs.l

[Paolo Bonzini]

Il 18/06/2014 16:19, Nadav Amit ha scritto:
> VMX instructions use 32-bit operands in 32-bit mode, and 64-bit operands in
> 64-bit mode.  The current implementation is broken since it does not use the
> register operands correctly, and always uses 64-bit for reads and writes.
> Moreover, write to memory in vmwrite only considers long-mode, so it ignores
> cs.l. This patch fixes this behavior.  The field of vmread/vmwrite is kept
> intentionally as 64-bit read since if bits [63:32] are not cleared the
> instruction should fail, according to Intel SDM.

This is not how I read the SDM:

"These instructions fail if given, in 64-bit mode, an operand that sets 
an encoding bit beyond bit 32." (Section 24.11.1.2)

"Outside IA-32e mode, the source operand has 32 bits, regardless of the 
value of CS.D. In 64-bit mode, the source operand has 64 bits; however, 
if bits 63:32 of the source operand are not zero, VMREAD will fail due 
to an attempt to access an unsupported VMCS component (see operation 
section)." (Description of VMREAD in Chapter 30).

I'll fix up the patch myself.

Paolo

Re: [PATCH v2 9/9] KVM: vmx: vmx instructions handling does not consider cs.l

[Nadav Amit]

On 6/18/14, 6:41 PM, Paolo Bonzini wrote:
> Il 18/06/2014 16:19, Nadav Amit ha scritto:
>> VMX instructions use 32-bit operands in 32-bit mode, and 64-bit
>> operands in
>> 64-bit mode.  The current implementation is broken since it does not
>> use the
>> register operands correctly, and always uses 64-bit for reads and writes.
>> Moreover, write to memory in vmwrite only considers long-mode, so it
>> ignores
>> cs.l. This patch fixes this behavior.  The field of vmread/vmwrite is
>> kept
>> intentionally as 64-bit read since if bits [63:32] are not cleared the
>> instruction should fail, according to Intel SDM.
>
> This is not how I read the SDM:
>
> "These instructions fail if given, in 64-bit mode, an operand that sets
> an encoding bit beyond bit 32." (Section 24.11.1.2)
>
> "Outside IA-32e mode, the source operand has 32 bits, regardless of the
> value of CS.D. In 64-bit mode, the source operand has 64 bits; however,
> if bits 63:32 of the source operand are not zero, VMREAD will fail due
> to an attempt to access an unsupported VMCS component (see operation
> section)." (Description of VMREAD in Chapter 30).
>
> I'll fix up the patch myself.
>

Perhaps I am missing something, but I don't see where my mistake is.
The VMREAD source operand is always read as 64-bits and I made no 
changes there. Therefore, if bits 63:32 are not zero, the instruction 
should fail when attempting to access the field.

The value in the source operand of VMWRITE which represents the value 
which should be written is zero-extended outside 64-bit mode.
Quoting: "The effective size of the primary source operand, which may be 
a register or in memory, is always 32 bits outside IA-32e mode (the 
setting of CS.D is ignored with respect to operand size) and 64 bits in 
64-bit mode." (Description of VMWRITE in chapter 30).

Regards,
Nadav

Re: [PATCH v2 9/9] KVM: vmx: vmx instructions handling does not consider cs.l

[Paolo Bonzini]

Il 18/06/2014 18:01, Nadav Amit ha scritto:
>>
>
> Perhaps I am missing something, but I don't see where my mistake is.
> The VMREAD source operand is always read as 64-bits and I made no
> changes there. Therefore, if bits 63:32 are not zero, the instruction
> should fail when attempting to access the field.

In 32-bit mode, it should be read as 32 bits: "Outside IA-32e mode, the 
source operand has 32 bits, regardless of the value of CS.D" (so it's 
never 16 bits, but it's also never 64 bits).

Paolo

Re: [PATCH v2 9/9] KVM: vmx: vmx instructions handling does not consider cs.l

[Nadav Amit]

On Jun 18, 2014, at 7:06 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 18/06/2014 18:01, Nadav Amit ha scritto:
>>> 
>> 
>> Perhaps I am missing something, but I don't see where my mistake is.
>> The VMREAD source operand is always read as 64-bits and I made no
>> changes there. Therefore, if bits 63:32 are not zero, the instruction
>> should fail when attempting to access the field.
> 
> In 32-bit mode, it should be read as 32 bits: "Outside IA-32e mode, the source operand has 32 bits, regardless of the value of CS.D" (so it's never 16 bits, but it's also never 64 bits).

Oh. I now get it. I misunderstood what the SDM said, as I was thinking that 62:32 will lead to failure also on 32-bit mode.
If you fix it, please fix both VMREAD and VMWRITE. If not, I would resubmit.

Thanks,
Nadav

Re: [PATCH v2 9/9] KVM: vmx: vmx instructions handling does not consider cs.l

[Paolo Bonzini]

Il 18/06/2014 19:51, Nadav Amit ha scritto:
> If you fix it, please fix both VMREAD and VMWRITE. If not, I would resubmit.

Yes, I'm fixing it myself.

Paolo

Re: [PATCH v2 0/9] KVM: x86: More emulator bugs

[Paolo Bonzini]

Il 18/06/2014 16:19, Nadav Amit ha scritto:
> This patch-set resolves several emulator bugs. Each fix is independent of the
> others.  The DR6 bug can occur during DR-access exit (regardless to
> unrestricted mode, MMIO and SPT).
>
>
> Changes in v2:
>
> Introduced kvm_register_readl and kvm_register_writel which consider long-mode
> and cs.l when reading the registers.
>
> Fixing the register read to respect 32/64 bit in hypercall handling, CR exit
> handling and VMX instructions handling.
>
> Thanks for re-reviewing the patch
>
> Nadav Amit (9):
>   KVM: x86: bit-ops emulation ignores offset on 64-bit
>   KVM: x86: Wrong emulation on 'xadd X, X'
>   KVM: x86: Inter privilage level ret emulation is not implemeneted
>   KVM: x86: emulation of dword cmov on long-mode should clear [63:32]
>   KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX
>   KVM: x86: check DR6/7 high-bits are clear only on long-mode
>   KVM: x86: Hypercall handling does not considers opsize correctly
>   KVM: vmx: handle_cr ignores 32/64-bit mode
>   KVM: vmx: vmx instructions handling does not consider cs.l
>
>  arch/x86/kvm/emulate.c | 31 ++++++++++++++++++++-----------
>  arch/x86/kvm/vmx.c     | 16 ++++++++--------
>  arch/x86/kvm/x86.c     | 11 ++++++-----
>  arch/x86/kvm/x86.h     | 27 +++++++++++++++++++++++++++
>  4 files changed, 61 insertions(+), 24 deletions(-)
>

Thanks, looks good.

Paolo

Re: [PATCH 0/6] KVM: x86: More emulator bugs

[Paolo Bonzini]

Il 15/06/2014 15:12, Nadav Amit ha scritto:
> This patch-set resolves several emulator bugs. Each fix is independent of the
> others.  The DR6/7 bug can occur during DR-access exit (regardless to
> unrestricted mode, MMIO and SPT).
>
> Thanks for reviewing the patches,
> Nadav
>
> Nadav Amit (6):
>   KVM: x86: bit-ops emulation ignores offset on 64-bit
>   KVM: x86: Wrong emulation on 'xadd X, X'
>   KVM: x86: Inter privilage level ret emulation is not implemeneted
>   KVM: x86: emulation of dword cmov on long-mode should clear [63:32]
>   KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX
>   KVM: x86: check DR6/7 high-bits are clear only on long-mode
>
>  arch/x86/kvm/emulate.c | 31 ++++++++++++++++++++-----------
>  arch/x86/kvm/x86.c     | 13 +++++++++++--
>  2 files changed, 31 insertions(+), 13 deletions(-)
>

I applied these locally.  Can you prepare testcases for patches 1, 2, 4 
and 5?  Perhaps patch 6 too if it's easy to reproduce.

Paolo