Skip to content
  • Vitaly Kuznetsov's avatar
    KVM: VMX: Properly handle kvm_read/write_guest_virt*() result · 7a35e515
    Vitaly Kuznetsov authored
    Syzbot reports the following issue:
    
    WARNING: CPU: 0 PID: 6819 at arch/x86/kvm/x86.c:618
     kvm_inject_emulated_page_fault+0x210/0x290 arch/x86/kvm/x86.c:618
    ...
    Call Trace:
    ...
    RIP: 0010:kvm_inject_emulated_page_fault+0x210/0x290 arch/x86/kvm/x86.c:618
    ...
     nested_vmx_get_vmptr+0x1f9/0x2a0 arch/x86/kvm/vmx/nested.c:4638
     handle_vmon arch/x86/kvm/vmx/nested.c:4767 [inline]
     handle_vmon+0x168/0x3a0 arch/x86/kvm/vmx/nested.c:4728
     vmx_handle_exit+0x29c/0x1260 arch/x86/kvm/vmx/vmx.c:6067
    
    'exception' we're trying to inject with kvm_inject_emulated_page_fault()
    comes from:
    
      nested_vmx_get_vmptr()
       kvm_read_guest_virt()
         kvm_read_guest_virt_helper()
           vcpu->arch.walk_mmu->gva_to_gpa()
    
    but it is only set when GVA to GPA conversion fails. In case it doesn't but
    we still fail kvm_vcpu_read_guest_page(), X86EMUL_IO_NEEDED is returned and
    nested_vmx_get_vmptr() calls kvm_inject_emulated_page_fault() with zeroed
    'exception'. This happen when the argument is MMIO.
    
    Paolo also noticed that nested_vmx_get_vmptr() is not the only place in
    KVM code where kvm_read/write_guest_virt*() return result is mishandled.
    VMX instructions along with INVPCID have the same issue. This was already
    noticed before, e.g. see commit 541ab2ae
    
     ("KVM: x86: work around
    leak of uninitialized stack contents") but was never fully fixed.
    
    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.
    
    Introduce vmx_handle_memory_failure() as an interim solution.
    
    Note, nested_vmx_get_vmptr() now has three possible outcomes: OK, PF,
    KVM_EXIT_INTERNAL_ERROR and callers need to know if userspace exit is
    needed (for KVM_EXIT_INTERNAL_ERROR) in case of failure. We don't seem
    to have a good enum describing this tristate, just add "int *ret" to
    nested_vmx_get_vmptr() interface to pass the information.
    
    Reported-by: default avatar <syzbot+2a7156e11dc199bdbd8a@syzkaller.appspotmail.com>
    Suggested-by: default avatarSean Christopherson <sean.j.christopherson@intel.com>
    Signed-off-by: default avatarVitaly Kuznetsov <vkuznets@redhat.com>
    Message-Id: <20200605115906.532682-1-vkuznets@redhat.com>
    Signed-off-by: default avatarPaolo Bonzini <pbonzini@redhat.com>
    7a35e515