[PATCH] KVM: x86: Avoid emulating instructions on #UD mistakenly

[PATCH] KVM: x86: Avoid emulating instructions on #UD mistakenly

[Nadav Amit]

Commit d40a6898e5 mistakenly caused instructions which are not marked as
EmulateOnUD to be emulated upon #UD exception. The commit caused the check of
whether the instruction flags include EmulateOnUD to never be evaluated. As a
result instructions whose emulation is broken may be emulated.  This fix moves
the evaluation of EmulateOnUD so it would be evaluated.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 56657b0..37a83b2 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4394,6 +4394,9 @@ done_prefixes:
 
 	ctxt->execute = opcode.u.execute;
 
+	if (!(ctxt->d & EmulateOnUD) && ctxt->ud)
+		return EMULATION_FAILED;
+
 	if (unlikely(ctxt->d &
 		     (NotImpl|EmulateOnUD|Stack|Op3264|Sse|Mmx|Intercept|CheckPerm))) {
 		/*
@@ -4406,9 +4409,6 @@ done_prefixes:
 		if (ctxt->d & NotImpl)
 			return EMULATION_FAILED;
 
-		if (!(ctxt->d & EmulateOnUD) && ctxt->ud)
-			return EMULATION_FAILED;
-
 		if (mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack))
 			ctxt->op_bytes = 8;
 
-- 
1.9.1

Re: [PATCH] KVM: x86: Avoid emulating instructions on #UD mistakenly

[Nadav Amit]

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

Correction: the word “never” in the message is too harsh.
Nonetheless, there is a regression bug. I encountered it with “wrfsbase” instruction.

Nadav

On Aug 13, 2014, at 4:50 PM, Nadav Amit <namit@cs.technion.ac.il> wrote:

> Commit d40a6898e5 mistakenly caused instructions which are not marked as
> EmulateOnUD to be emulated upon #UD exception. The commit caused the check of
> whether the instruction flags include EmulateOnUD to never be evaluated. As a
> result instructions whose emulation is broken may be emulated.  This fix moves
> the evaluation of EmulateOnUD so it would be evaluated.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
> arch/x86/kvm/emulate.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 56657b0..37a83b2 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4394,6 +4394,9 @@ done_prefixes:
> 
>  	ctxt->execute = opcode.u.execute;
> 
> +	if (!(ctxt->d & EmulateOnUD) && ctxt->ud)
> +		return EMULATION_FAILED;
> +
>  	if (unlikely(ctxt->d &
>  		     (NotImpl|EmulateOnUD|Stack|Op3264|Sse|Mmx|Intercept|CheckPerm))) {
>  		/*
> @@ -4406,9 +4409,6 @@ done_prefixes:
>  		if (ctxt->d & NotImpl)
>  			return EMULATION_FAILED;
> 
> -		if (!(ctxt->d & EmulateOnUD) && ctxt->ud)
> -			return EMULATION_FAILED;
> -
>  		if (mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack))
>  			ctxt->op_bytes = 8;
> 
> -- 
> 1.9.1
> 


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] KVM: x86: Avoid emulating instructions on #UD mistakenly

[Paolo Bonzini]

Il 13/08/2014 16:21, Nadav Amit ha scritto:
> Correction: the word “never” in the message is too harsh.
> Nonetheless, there is a regression bug. I encountered it with “wrfsbase” instruction.

So KVM is emulating wrfsbase even if the host doesn't support it?

I'm swapping the order of the two operands of &&, since the first one will almost
always be true and the second one will almost always be false.

Also, there's now no need to test EmulateOnUD in the condition below.  Does the
below look good to you?

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 37a83b24e040..ef117b842334 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4394,11 +4394,11 @@ done_prefixes:
 
 	ctxt->execute = opcode.u.execute;
 
-	if (!(ctxt->d & EmulateOnUD) && ctxt->ud)
+	if (unlikely(ctxt->ud) && likely(!(ctxt->d & EmulateOnUD)))
 		return EMULATION_FAILED;
 
 	if (unlikely(ctxt->d &
-		     (NotImpl|EmulateOnUD|Stack|Op3264|Sse|Mmx|Intercept|CheckPerm))) {
+		     (NotImpl|Stack|Op3264|Sse|Mmx|Intercept|CheckPerm))) {
 		/*
 		 * These are copied unconditionally here, and checked unconditionally
 		 * in x86_emulate_insn.

 

Paolo

> Nadav
> 
> On Aug 13, 2014, at 4:50 PM, Nadav Amit <namit@cs.technion.ac.il> wrote:
> 
>> Commit d40a6898e5 mistakenly caused instructions which are not marked as
>> EmulateOnUD to be emulated upon #UD exception. The commit caused the check of
>> whether the instruction flags include EmulateOnUD to never be evaluated. As a
>> result instructions whose emulation is broken may be emulated.  This fix moves
>> the evaluation of EmulateOnUD so it would be evaluated.
>>
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
>> arch/x86/kvm/emulate.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index 56657b0..37a83b2 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -4394,6 +4394,9 @@ done_prefixes:
>>
>>  	ctxt->execute = opcode.u.execute;
>>
>> +	if (!(ctxt->d & EmulateOnUD) && ctxt->ud)
>> +		return EMULATION_FAILED;
>> +
>>  	if (unlikely(ctxt->d &
>>  		     (NotImpl|EmulateOnUD|Stack|Op3264|Sse|Mmx|Intercept|CheckPerm))) {
>>  		/*
>> @@ -4406,9 +4409,6 @@ done_prefixes:
>>  		if (ctxt->d & NotImpl)
>>  			return EMULATION_FAILED;
>>
>> -		if (!(ctxt->d & EmulateOnUD) && ctxt->ud)
>> -			return EMULATION_FAILED;
>> -
>>  		if (mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack))
>>  			ctxt->op_bytes = 8;
>>
>> -- 
>> 1.9.1
>>
> 

Re: [PATCH] KVM: x86: Avoid emulating instructions on #UD mistakenly

[Nadav Amit]

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


On Aug 18, 2014, at 11:31 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 13/08/2014 16:21, Nadav Amit ha scritto:
>> Correction: the word “never” in the message is too harsh.
>> Nonetheless, there is a regression bug. I encountered it with “wrfsbase” instruction.
> 
> So KVM is emulating wrfsbase even if the host doesn't support it?
KVM doesn’t know wrfsbase - wrfsbase is encoded like clflush with rep-prefix.
Therefore, the emulator thinks it can emulate it as clflush, and eliminates the #UD.

> 
> I'm swapping the order of the two operands of &&, since the first one will almost
> always be true and the second one will almost always be false.
> 
> Also, there's now no need to test EmulateOnUD in the condition below.  Does the
> below look good to you?
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 37a83b24e040..ef117b842334 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4394,11 +4394,11 @@ done_prefixes:
> 
>  	ctxt->execute = opcode.u.execute;
> 
> -	if (!(ctxt->d & EmulateOnUD) && ctxt->ud)
> +	if (unlikely(ctxt->ud) && likely(!(ctxt->d & EmulateOnUD)))
>  		return EMULATION_FAILED;
> 
>  	if (unlikely(ctxt->d &
> -		     (NotImpl|EmulateOnUD|Stack|Op3264|Sse|Mmx|Intercept|CheckPerm))) {
> +		     (NotImpl|Stack|Op3264|Sse|Mmx|Intercept|CheckPerm))) {
>  		/*
>  		 * These are copied unconditionally here, and checked unconditionally
>  		 * in x86_emulate_insn.
> 

Sure. Until I find some bugs in it. ;-)

Thanks,
Nadav

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]