diff mbox

[2/3] KVM: x86: emulator: Return to user-mode on L1 CPL=0 emulation failure

Message ID 1509891703-9735-3-git-send-email-liran.alon@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Liran Alon Nov. 5, 2017, 2:21 p.m. UTC 
From: Liran Alon <liran.alon@ravellosystems.com>

On this case, handle_emulation_failure() fills kvm_run with
internal-error information which it expects to be delivered
to user-mode for further processing.
However, the code reports a wrong return-value which makes KVM to never
return to user-mode on this scenario.

Fixes: 6d77dbfc88e3 ("KVM: inject #UD if instruction emulation fails and exit to
userspace")

Signed-off-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paolo Bonzini Nov. 6, 2017, 9:19 a.m. UTC | #1 
On 05/11/2017 15:21, Liran Alon wrote:
> From: Liran Alon <liran.alon@ravellosystems.com>
> 
> On this case, handle_emulation_failure() fills kvm_run with
> internal-error information which it expects to be delivered
> to user-mode for further processing.
> However, the code reports a wrong return-value which makes KVM to never
> return to user-mode on this scenario.
> 
> Fixes: 6d77dbfc88e3 ("KVM: inject #UD if instruction emulation fails and exit to
> userspace")
> 
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  arch/x86/kvm/x86.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 03869eb7fcd6..f4edb4baf441 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5413,7 +5413,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)
>  		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>  		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
>  		vcpu->run->internal.ndata = 0;
> -		r = EMULATE_FAIL;
> +		r = EMULATE_USER_EXIT;
>  	}
>  	kvm_queue_exception(vcpu, UD_VECTOR);
>  
> 

You should still get an emulation failure when the emulator is invoked
by mmu.c:

        case EMULATE_USER_EXIT:
                ++vcpu->stat.mmio_exits;
                /* fall through */
        case EMULATE_FAIL:
                return 0;

What is the caller of x86_emulate_instruction that you are interested
in?  #UD is not affected, because emulation_type has EMULTYPE_TRAP_UD
set and therefore x86_emulate_instruction exits before invoking
handle_emulation_failure.

Thanks,

Paolo
Liran Alon Nov. 6, 2017, 1:25 p.m. UTC | #2 
On 06/11/17 11:19, Paolo Bonzini wrote:
> On 05/11/2017 15:21, Liran Alon wrote:
>> From: Liran Alon <liran.alon@ravellosystems.com>
>>
>> On this case, handle_emulation_failure() fills kvm_run with
>> internal-error information which it expects to be delivered
>> to user-mode for further processing.
>> However, the code reports a wrong return-value which makes KVM to never
>> return to user-mode on this scenario.
>>
>> Fixes: 6d77dbfc88e3 ("KVM: inject #UD if instruction emulation fails and exit to
>> userspace")
>>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> ---
>>   arch/x86/kvm/x86.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 03869eb7fcd6..f4edb4baf441 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5413,7 +5413,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu)
>>   		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>   		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
>>   		vcpu->run->internal.ndata = 0;
>> -		r = EMULATE_FAIL;
>> +		r = EMULATE_USER_EXIT;
>>   	}
>>   	kvm_queue_exception(vcpu, UD_VECTOR);
>>
>>
>
> You should still get an emulation failure when the emulator is invoked
> by mmu.c:
>
>          case EMULATE_USER_EXIT:
>                  ++vcpu->stat.mmio_exits;
>                  /* fall through */
>          case EMULATE_FAIL:
>                  return 0;
>
> What is the caller of x86_emulate_instruction that you are interested
> in?  #UD is not affected, because emulation_type has EMULTYPE_TRAP_UD
> set and therefore x86_emulate_instruction exits before invoking
> handle_emulation_failure.
I think this patch is still correct from a number of reasons:

1) If I understand the code correctly, semantically it doesn't make 
sense to fill kvm_run struct without exiting to user-mode. Therefore, if 
emulator filled kvm_run, it makes sense that it needs to return 
EMULATE_USER_EXIT.

2) EMULTYPE_TRAP_UD only causes the emulator to return EMULATE_FAIL in 
case emulation fails on instruction decoding. However, consider a case 
where #UD intercept happens on a valid instruction (such as VMMCALL AMD 
opcode on physical Intel CPU). In that case, instruction decoding 
doesn't fail but we could still fail on instruction emulation at 
x86_emulate_insn(). In this case, EMULTYPE_TRAP_UD flag is not 
considered anymore and failure will reach handle_emulation_failure().

3) We have another KVM commits series (not upstream yet) that adds 
intercept on #GP which calls the x86 emulator. This is done to allow 
access to I/O ports even though they aren't allowed via guest's TSS I/O 
permissions bitmap.
>
> Thanks,
>
> Paolo
>
Paolo Bonzini Nov. 6, 2017, 1:50 p.m. UTC | #3 
On 06/11/2017 14:25, Liran Alon wrote:
>> What is the caller of x86_emulate_instruction that you are interested
>> in?  #UD is not affected, because emulation_type has EMULTYPE_TRAP_UD
>> set and therefore x86_emulate_instruction exits before invoking
>> handle_emulation_failure. 
>>
> I think this patch is still correct from a number of reasons:
> 
> 1) If I understand the code correctly, semantically it doesn't make
> sense to fill kvm_run struct without exiting to user-mode. Therefore, if
> emulator filled kvm_run, it makes sense that it needs to return
> EMULATE_USER_EXIT.
> 
> 2) EMULTYPE_TRAP_UD only causes the emulator to return EMULATE_FAIL in
> case emulation fails on instruction decoding. However, consider a case
> where #UD intercept happens on a valid instruction (such as VMMCALL AMD
> opcode on physical Intel CPU). In that case, instruction decoding
> doesn't fail but we could still fail on instruction emulation at
> x86_emulate_insn(). In this case, EMULTYPE_TRAP_UD flag is not
> considered anymore and failure will reach handle_emulation_failure().

(1) is true, but (2) more or less answers my question.  So

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

> 3) We have another KVM commits series (not upstream yet) that adds
> intercept on #GP which calls the x86 emulator. This is done to allow
> access to I/O ports even though they aren't allowed via guest's TSS I/O
> permissions bitmap.

vmport?... :)

Paolo
Liran Alon Nov. 6, 2017, 2:08 p.m. UTC | #4 
On 06/11/17 15:50, Paolo Bonzini wrote:
> On 06/11/2017 14:25, Liran Alon wrote:
>>> What is the caller of x86_emulate_instruction that you are interested
>>> in?  #UD is not affected, because emulation_type has EMULTYPE_TRAP_UD
>>> set and therefore x86_emulate_instruction exits before invoking
>>> handle_emulation_failure.
>>>
>> I think this patch is still correct from a number of reasons:
>>
>> 1) If I understand the code correctly, semantically it doesn't make
>> sense to fill kvm_run struct without exiting to user-mode. Therefore, if
>> emulator filled kvm_run, it makes sense that it needs to return
>> EMULATE_USER_EXIT.
>>
>> 2) EMULTYPE_TRAP_UD only causes the emulator to return EMULATE_FAIL in
>> case emulation fails on instruction decoding. However, consider a case
>> where #UD intercept happens on a valid instruction (such as VMMCALL AMD
>> opcode on physical Intel CPU). In that case, instruction decoding
>> doesn't fail but we could still fail on instruction emulation at
>> x86_emulate_insn(). In this case, EMULTYPE_TRAP_UD flag is not
>> considered anymore and failure will reach handle_emulation_failure().
>
> (1) is true, but (2) more or less answers my question.  So
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
>> 3) We have another KVM commits series (not upstream yet) that adds
>> intercept on #GP which calls the x86 emulator. This is done to allow
>> access to I/O ports even though they aren't allowed via guest's TSS I/O
>> permissions bitmap.
>
> vmport?... :)
Yep. Stay tuned. :)
>
> Paolo
>
Paolo Bonzini Nov. 6, 2017, 2:13 p.m. UTC | #5 
On 06/11/2017 15:08, Liran Alon wrote:
>>> 3) We have another KVM commits series (not upstream yet) that adds
>>> intercept on #GP which calls the x86 emulator. This is done to allow
>>> access to I/O ports even though they aren't allowed via guest's TSS I/O
>>> permissions bitmap.
>>
>> vmport?... :)
>
> Yep. Stay tuned. :)

Is that a warning? ;)

Paolo
diff mbox

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 03869eb7fcd6..f4edb4baf441 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5413,7 +5413,7 @@  static int handle_emulation_failure(struct kvm_vcpu *vcpu)
 		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
 		vcpu->run->internal.ndata = 0;
-		r = EMULATE_FAIL;
+		r = EMULATE_USER_EXIT;
 	}
 	kvm_queue_exception(vcpu, UD_VECTOR);