Re: Query about calling kvm_vcpu_gfn_to_memslot() with a GVA (Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Sean Christopherson <seanjc@google.com>]

On Mon, Jan 17, 2022, Liam Merwick wrote:
> On 13/01/2022 16:57, Sean Christopherson wrote:
> > On Thu, Jan 13, 2022, Liam Merwick wrote:
> > > When looking into an SEV issue it was noted that the second arg to
> > > kvm_vcpu_gfn_to_memslot() is a gfn_t but kvm_rip_read() will return guest
> > > RIP which is a guest virtual address and memslots hold guest physical
> > > addresses. How is KVM supposed to translate it to a memslot
> > > and indicate if the guest RIP is valid?
> > 
> > Ugh, magic?  That code is complete garbage.  It worked to fix the selftest issue
> > because the selftest identity maps the relevant guest code.
> > 
> > The entire idea is a hack.  If KVM gets into an infinite loop because the guest
> > is attempting to fetch from MMIO, then the #NPF/#PF should have the FETCH bit set
> > in the error code.  I.e. I believe the below change should fix the original issue,
> > at which point we can revert the above.  I'll test today and hopefully get a patch
> > sent out.
> 
> Thanks Sean.
> 
> I have been running with this patch along with reverting commit
> e72436bc3a52 ("KVM: SVM: avoid infinite loop on NPF from bad address")
> with over 150 hours runtime on multiple machines and it resolves an SEV
> guest crash I was encountering where if there were no decode assist bytes
> available, it then continued on and hit the invalid RIP check.

Nice!  Thanks for the update.  The below patch doesn't fully remedy KVM's woes,
and it's not the best place to handle PFERR_FETCH_MASK for other reasons, so what
I'll officially post (hopefully soon) will be different, but the basic gist will
be the same.

> Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
> Tested-by: Liam Merwick <liam.merwick@oracle.com>
> 
> > 
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index c3d9006478a4..e1d2a46e06bf 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -1995,6 +1995,17 @@ static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
> >          vmcb_mark_dirty(svm->vmcb, VMCB_DR);
> >   }
> > 
> > +static char *svm_get_pf_insn_bytes(struct vcpu_svm *svm)
> > +{
> > +       if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
> > +               return NULL;
> > +
> > +       if (svm->vmcb->control.exit_info_1 & PFERR_FETCH_MASK)
> > +               return NULL;
> > +
> > +       return svm->vmcb->control.insn_bytes;
> > +}
> > +
> >   static int pf_interception(struct kvm_vcpu *vcpu)
> >   {
> >          struct vcpu_svm *svm = to_svm(vcpu);
> > @@ -2003,9 +2014,8 @@ static int pf_interception(struct kvm_vcpu *vcpu)
> >          u64 error_code = svm->vmcb->control.exit_info_1;
> > 
> >          return kvm_handle_page_fault(vcpu, error_code, fault_address,
> > -                       static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> > -                       svm->vmcb->control.insn_bytes : NULL,
> > -                       svm->vmcb->control.insn_len);
> > +                                    svm_get_pf_insn_bytes(svm),
> > +                                    svm->vmcb->control.insn_len);
> >   }
> > 
> >   static int npf_interception(struct kvm_vcpu *vcpu)
> > @@ -2017,9 +2027,8 @@ static int npf_interception(struct kvm_vcpu *vcpu)
> > 
> >          trace_kvm_page_fault(fault_address, error_code);
> >          return kvm_mmu_page_fault(vcpu, fault_address, error_code,
> > -                       static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> > -                       svm->vmcb->control.insn_bytes : NULL,
> > -                       svm->vmcb->control.insn_len);
> > +                                 svm_get_pf_insn_bytes(svm),
> > +                                 svm->vmcb->control.insn_len);
> >   }
> > 
> >   static int db_interception(struct kvm_vcpu *vcpu)
>