| Message ID | 20210401141814.1029036-3-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce KVM_{GET|SET}_SREGS2 and fix PDPTR migration | expand |
On 01/04/21 16:18, Maxim Levitsky wrote: > While KVM's MMU should be fully reset by loading of nested CR0/CR3/CR4 > by KVM_SET_SREGS, we are not in nested mode yet when we do it and therefore > only root_mmu is reset. > > On regular nested entries we call nested_svm_load_cr3 which both updates > the guest's CR3 in the MMU when it is needed, and it also initializes > the mmu again which makes it initialize the walk_mmu as well when nested > paging is enabled in both host and guest. > > Since we don't call nested_svm_load_cr3 on nested state load, > the walk_mmu can be left uninitialized, which can lead to a NULL pointer > dereference while accessing it if we happen to get a nested page fault > right after entering the nested guest first time after the migration and > we decide to emulate it, which leads to the emulator trying to access > walk_mmu->gva_to_gpa which is NULL. > > Therefore we should call this function on nested state load as well. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > arch/x86/kvm/svm/nested.c | 40 +++++++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index 8523f60adb92..ac5e3e17bda4 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -215,24 +215,6 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm) > return true; > } > > -static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu) > -{ > - struct vcpu_svm *svm = to_svm(vcpu); > - > - if (WARN_ON(!is_guest_mode(vcpu))) > - return true; > - > - if (!nested_svm_vmrun_msrpm(svm)) { > - vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > - vcpu->run->internal.suberror = > - KVM_INTERNAL_ERROR_EMULATION; > - vcpu->run->internal.ndata = 0; > - return false; > - } > - > - return true; > -} > - > static bool nested_vmcb_check_controls(struct vmcb_control_area *control) > { > if (CC(!vmcb_is_intercept(control, INTERCEPT_VMRUN))) > @@ -1312,6 +1294,28 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, > return ret; > } > > +static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu) > +{ > + struct vcpu_svm *svm = to_svm(vcpu); > + > + if (WARN_ON(!is_guest_mode(vcpu))) > + return true; > + > + if (nested_svm_load_cr3(&svm->vcpu, vcpu->arch.cr3, > + nested_npt_enabled(svm))) > + return false; > + > + if (!nested_svm_vmrun_msrpm(svm)) { > + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > + vcpu->run->internal.suberror = > + KVM_INTERNAL_ERROR_EMULATION; > + vcpu->run->internal.ndata = 0; > + return false; > + } > + > + return true; > +} > + > struct kvm_x86_nested_ops svm_nested_ops = { > .check_events = svm_check_nested_events, > .triple_fault = nested_svm_triple_fault, > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 8523f60adb92..ac5e3e17bda4 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -215,24 +215,6 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm) return true; } -static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu) -{ - struct vcpu_svm *svm = to_svm(vcpu); - - if (WARN_ON(!is_guest_mode(vcpu))) - return true; - - if (!nested_svm_vmrun_msrpm(svm)) { - vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; - vcpu->run->internal.suberror = - KVM_INTERNAL_ERROR_EMULATION; - vcpu->run->internal.ndata = 0; - return false; - } - - return true; -} - static bool nested_vmcb_check_controls(struct vmcb_control_area *control) { if (CC(!vmcb_is_intercept(control, INTERCEPT_VMRUN))) @@ -1312,6 +1294,28 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, return ret; } +static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu) +{ + struct vcpu_svm *svm = to_svm(vcpu); + + if (WARN_ON(!is_guest_mode(vcpu))) + return true; + + if (nested_svm_load_cr3(&svm->vcpu, vcpu->arch.cr3, + nested_npt_enabled(svm))) + return false; + + if (!nested_svm_vmrun_msrpm(svm)) { + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; + vcpu->run->internal.suberror = + KVM_INTERNAL_ERROR_EMULATION; + vcpu->run->internal.ndata = 0; + return false; + } + + return true; +} + struct kvm_x86_nested_ops svm_nested_ops = { .check_events = svm_check_nested_events, .triple_fault = nested_svm_triple_fault,
While KVM's MMU should be fully reset by loading of nested CR0/CR3/CR4 by KVM_SET_SREGS, we are not in nested mode yet when we do it and therefore only root_mmu is reset. On regular nested entries we call nested_svm_load_cr3 which both updates the guest's CR3 in the MMU when it is needed, and it also initializes the mmu again which makes it initialize the walk_mmu as well when nested paging is enabled in both host and guest. Since we don't call nested_svm_load_cr3 on nested state load, the walk_mmu can be left uninitialized, which can lead to a NULL pointer dereference while accessing it if we happen to get a nested page fault right after entering the nested guest first time after the migration and we decide to emulate it, which leads to the emulator trying to access walk_mmu->gva_to_gpa which is NULL. Therefore we should call this function on nested state load as well. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/svm/nested.c | 40 +++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-)