Re: [PATCH] KVM: nVMX: Inject #GP when nested_vmx_get_vmptr() fails to read guest memory

[Sean Christopherson]

On Thu, Jun 04, 2020 at 04:40:52PM +0200, Paolo Bonzini wrote:
> On 04/06/20 16:31, Vitaly Kuznetsov wrote:

...

> > KVM could've handled the request correctly by going to userspace and
> > performing I/O but there doesn't seem to be a good need for such requests
> > in the first place. Sane guests should not call VMXON/VMPTRLD/VMCLEAR with
> > anything but normal memory. Just inject #GP to find insane ones.
> > 
> > Reported-by: syzbot+2a7156e11dc199bdbd8a@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 9c74a732b08d..05d57c3cb1ce 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -4628,14 +4628,29 @@ static int nested_vmx_get_vmptr(struct kvm_vcpu *vcpu, gpa_t *vmpointer)
> >  {
> >  	gva_t gva;
> >  	struct x86_exception e;
> > +	int r;
> >  
> >  	if (get_vmx_mem_address(vcpu, vmx_get_exit_qual(vcpu),
> >  				vmcs_read32(VMX_INSTRUCTION_INFO), false,
> >  				sizeof(*vmpointer), &gva))
> >  		return 1;
> >  
> > -	if (kvm_read_guest_virt(vcpu, gva, vmpointer, sizeof(*vmpointer), &e)) {
> > -		kvm_inject_emulated_page_fault(vcpu, &e);
> > +	r = kvm_read_guest_virt(vcpu, gva, vmpointer, sizeof(*vmpointer), &e);
> > +	if (r != X86EMUL_CONTINUE) {
> > +		if (r == X86EMUL_PROPAGATE_FAULT) {
> > +			kvm_inject_emulated_page_fault(vcpu, &e);
> > +		} else {
> > +			/*
> > +			 * X86EMUL_IO_NEEDED is returned when kvm_vcpu_read_guest_page()
> > +			 * fails to read guest's memory (e.g. when 'gva' points to MMIO
> > +			 * space). While KVM could've handled the request correctly by
> > +			 * exiting to userspace and performing I/O, there doesn't seem
> > +			 * to be a real use-case behind such requests, just inject #GP
> > +			 * for now.
> > +			 */
> > +			kvm_inject_gp(vcpu, 0);
> > +		}
> > +
> >  		return 1;
> >  	}
> >  
> > 
> 
> Hi Vitaly,
> 
> looks good but we need to do the same in handle_vmread, handle_vmwrite,
> handle_invept and handle_invvpid.  Which probably means adding something
> like nested_inject_emulation_fault to commonize the inner "if".

Can we just kill the guest already instead of throwing more hacks at this
and hoping something sticks?  We already have one in
kvm_write_guest_virt_system...

  commit 541ab2aeb28251bf7135c7961f3a6080eebcc705
  Author: Fuqian Huang <huangfq.daxian@xxxxxxxxx>
  Date:   Thu Sep 12 12:18:17 2019 +0800

    KVM: x86: work around leak of uninitialized stack contents

    Emulation of VMPTRST can incorrectly inject a page fault
    when passed an operand that points to an MMIO address.
    The page fault will use uninitialized kernel stack memory
    as the CR2 and error code.

    The right behavior would be to abort the VM with a KVM_EXIT_INTERNAL_ERROR
    exit to userspace; however, it is not an easy fix, so for now just ensure
    that the error code and CR2 are zero.