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

[Jim Mattson]

On Thu, Jun 4, 2020 at 9:43 AM Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
>
> Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:
>
> > On Thu, Jun 04, 2020 at 05:33:25PM +0200, Vitaly Kuznetsov wrote:
> >> Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:
> >>
> >> > 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.
> >> >> >
> >>
> >> ...
> >>
> >> >>
> >> >> 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
> >> >
> >>
> >> Oh I see...
> >>
> >> [...]
> >>
> >> Let's get back to 'vm_bugged' idea then?
> >>
> >> https://lore.kernel.org/kvm/87muadnn1t.fsf@xxxxxxxxxxxxxxxxxxxx/
> >
> > Hmm, I don't think we need to go that far.  The 'vm_bugged' idea was more
> > to handle cases where KVM itself (or hardware) screwed something up and
> > detects an issue deep in a call stack with no recourse for reporting the
> > error up the stack.
> >
> > That isn't the case here.  Unless I'm mistaken, the end result is simliar
> > to this patch, except that KVM would exit to userspace with
> > KVM_INTERNAL_ERROR_EMULATION instead of injecting a #GP.  E.g.
>
> I just wanted to resurrect that 'vm_bugged' idea but was waiting for a
> good opportunity :-)
>
> The advantage of KVM_EXIT_INTERNAL_ERROR is that we're not trying to
> invent some behavior which is not in SDM and making it a bit more likely
> that we get a bug report from an angry user.

If KVM can't handle the emulation, KVM_EXIT_INTERNAL_ERROR is far
better than cooking up fictional faults to deliver to the guest.

> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 9c74a732b08d..e13d2c0014e2 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -4624,6 +4624,20 @@ void nested_vmx_pmu_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
> >         }
> >  }
> >
> > +static int nested_vmx_handle_memory_failure(struct kvm_vcpu *vcpu, int ret,
> > +                                           struct x86_exception *e)
> > +{
> > +       if (r == X86EMUL_PROPAGATE_FAULT) {
> > +               kvm_inject_emulated_page_fault(vcpu, &e);
> > +               return 1;
> > +       }
> > +
> > +       vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> > +       vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> > +       vcpu->run->internal.ndata = 0;
> > +       return 0;
> > +}
> > +
> >  static int nested_vmx_get_vmptr(struct kvm_vcpu *vcpu, gpa_t *vmpointer)
> >  {
> >         gva_t gva;
> > @@ -4634,11 +4648,9 @@ static int nested_vmx_get_vmptr(struct kvm_vcpu *vcpu, gpa_t *vmpointer)
> >                                 sizeof(*vmpointer), &gva))
> >                 return 1;
> >
> > -       if (kvm_read_guest_virt(vcpu, gva, vmpointer, sizeof(*vmpointer), &e)) {
> > -               kvm_inject_emulated_page_fault(vcpu, &e);
> > -               return 1;
> > -       }
> > -
> > +       r kvm_read_guest_virt(vcpu, gva, vmpointer, sizeof(*vmpointer), &e);
> > +       if (r)
> > +               return nested_vmx_handle_memory_failure(r, &e);
> >         return 0;
> >  }
> >
>
> ... and the same for handle_vmread, handle_vmwrite, handle_invept and
> handle_invvpid as suggested by Paolo. I'll be sending this as v2 with
> your Suggested-by: shortly.
>
> >
> >
> > Side topic, I have some preliminary patches for the 'vm_bugged' idea.  I'll
> > try to whip them into something that can be posted upstream in the next few
> > weeks.
> >
>
> Sounds great!
>
> --
> Vitaly
>