linux-kernel - Re: [RESEND PATCH 02/13] KVM: x86: Clean up handle_emulation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 23 Aug 2019 15:58:28 +0300
From:   Liran Alon <liran.alon@...cle.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     Sean Christopherson <sean.j.christopherson@...el.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>
Subject: Re: [RESEND PATCH 02/13] KVM: x86: Clean up
 handle_emulation_failure()

> On 23 Aug 2019, at 12:23, Vitaly Kuznetsov <vkuznets@...hat.com> wrote:
> 
> Sean Christopherson <sean.j.christopherson@...el.com> writes:
> 
>> When handling emulation failure, return the emulation result directly
>> instead of capturing it in a local variable.  Future patches will move
>> additional cases into handle_emulation_failure(), clean up the cruft
>> before so there isn't an ugly mix of setting a local variable and
>> returning directly.
>> 
>> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
>> ---
>> arch/x86/kvm/x86.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index cd425f54096a..c6de5bc4fa5e 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6207,24 +6207,22 @@ EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
>> 
>> static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
>> {
>> -	int r = EMULATE_DONE;
>> -
>> 	++vcpu->stat.insn_emulation_fail;
>> 	trace_kvm_emulate_insn_failed(vcpu);
>> 
>> 	if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
>> 		return EMULATE_FAIL;
>> 
>> +	kvm_queue_exception(vcpu, UD_VECTOR);
>> +
>> 	if (!is_guest_mode(vcpu) && kvm_x86_ops->get_cpl(vcpu) == 0) {
>> 		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>> 		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
>> 		vcpu->run->internal.ndata = 0;
>> -		r = EMULATE_USER_EXIT;
>> +		return EMULATE_USER_EXIT;
>> 	}
>> 
>> -	kvm_queue_exception(vcpu, UD_VECTOR);
>> -
>> -	return r;
>> +	return EMULATE_DONE;
>> }
>> 
>> static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t cr2,
> 
> No functional change,
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> 
> Just for self-education, what sane userspace is supposed to do when it
> sees KVM_EXIT_INTERNAL_ERROR other than kill the guest? Why does it make
> sense to still prepare to inject '#UD’
> 
> -- 
> Vitaly

The commit which introduced this behaviour seems to be
6d77dbfc88e3 ("KVM: inject #UD if instruction emulation fails and exit to userspace")

I actually agree with Vitaly. It made more sense that the ABI would be that
on internal emulation failure, we just return to userspace and allow it to handle
the scenario however it likes. If it wishes to queue #UD on vCPU and resume
guest in case CPL==3 then it made sense that this logic would only be in userspace.
Thus, there is no need for KVM to queue a #UD from kernel on this scenario...

What’s even weirder is that this ABI was then further broken by 2 later commits:
First, fc3a9157d314 ("KVM: X86: Don't report L2 emulation failures to user-space")
changed behaviour to avoid reporting emulation error in case vCPU in guest-mode.
Then, a2b9e6c1a35a ("KVM: x86: Don't report guest userspace emulation error to userspace")
Changed behaviour similarly to avoid reporting emulation error in case vCPU CPL!=0.
In both cases, only #UD is injected to guest without userspace being aware of it.

Problem is that if we would change this ABI to not queue #UD on emulation error,
we will definitely break userspace VMMs that rely on it when they re-enter into guest
in this scenario and expect #UD to be injected.
Therefore, the only way to change this behaviour is to introduce a new KVM_CAP
that needs to be explicitly enabled from userspace.
But because most likely most userspace VMMs just terminate guest in case
of emulation-failure, it’s probably not worth it and Sean’s commit is good enough.

For the commit itself:
Reviewed-by: Liran Alon <liran.alon@...cle.com>

-Liran