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

[Liran Alon <LIRAN.ALON@ORACLE.COM>]

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
>