Re: [PATCH RFC v7 44/64] KVM: SVM: Remove the long-lived GHCB host map

[Alper Gun]

On Wed, Jan 18, 2023 at 7:27 AM Jeremi Piotrowski
<jpiotrowski@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Dec 14, 2022 at 01:40:36PM -0600, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@xxxxxxx>
> >
> > On VMGEXIT, sev_handle_vmgexit() creates a host mapping for the GHCB GPA,
> > and unmaps it just before VM-entry. This long-lived GHCB map is used by
> > the VMGEXIT handler through accessors such as ghcb_{set_get}_xxx().
> >
> > A long-lived GHCB map can cause issue when SEV-SNP is enabled. When
> > SEV-SNP is enabled the mapped GPA needs to be protected against a page
> > state change.
> >
> > To eliminate the long-lived GHCB mapping, update the GHCB sync operations
> > to explicitly map the GHCB before access and unmap it after access is
> > complete. This requires that the setting of the GHCBs sw_exit_info_{1,2}
> > fields be done during sev_es_sync_to_ghcb(), so create two new fields in
> > the vcpu_svm struct to hold these values when required to be set outside
> > of the GHCB mapping.
> >
> > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> > Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > [mdr: defer per_cpu() assignment and order it with barrier() to fix case
> >       where kvm_vcpu_map() causes reschedule on different CPU]
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> >  arch/x86/kvm/svm/sev.c | 131 ++++++++++++++++++++++++++---------------
> >  arch/x86/kvm/svm/svm.c |  18 +++---
> >  arch/x86/kvm/svm/svm.h |  24 +++++++-
> >  3 files changed, 116 insertions(+), 57 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index d5c6e48055fb..6ac0cb6e3484 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -2921,15 +2921,40 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu)
> >       kvfree(svm->sev_es.ghcb_sa);
> >  }
> >
> > +static inline int svm_map_ghcb(struct vcpu_svm *svm, struct kvm_host_map *map)
> > +{
> > +     struct vmcb_control_area *control = &svm->vmcb->control;
> > +     u64 gfn = gpa_to_gfn(control->ghcb_gpa);
> > +
> > +     if (kvm_vcpu_map(&svm->vcpu, gfn, map)) {
> > +             /* Unable to map GHCB from guest */
> > +             pr_err("error mapping GHCB GFN [%#llx] from guest\n", gfn);
> > +             return -EFAULT;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static inline void svm_unmap_ghcb(struct vcpu_svm *svm, struct kvm_host_map *map)
> > +{
> > +     kvm_vcpu_unmap(&svm->vcpu, map, true);
> > +}
> > +
> >  static void dump_ghcb(struct vcpu_svm *svm)
> >  {
> > -     struct ghcb *ghcb = svm->sev_es.ghcb;
> > +     struct kvm_host_map map;
> >       unsigned int nbits;
> > +     struct ghcb *ghcb;
> > +
> > +     if (svm_map_ghcb(svm, &map))
> > +             return;
> > +
> > +     ghcb = map.hva;
>
> dump_ghcb() is called from sev_es_validate_vmgexit() with the ghcb already
> mapped. How about passing 'struct kvm_host_map *' (or struct ghcb *) as a
> param to avoid double mapping?

This also causes a soft lockup, PSC spin lock is already acquired in
sev_es_validate_vmgexit. dump_ghcb will try to acquire the same lock
again. So a guest can send an invalid ghcb page  and cause a host soft
lockup.

>
> >
> >       /* Re-use the dump_invalid_vmcb module parameter */
> >       if (!dump_invalid_vmcb) {
> >               pr_warn_ratelimited("set kvm_amd.dump_invalid_vmcb=1 to dump internal KVM state.\n");
> > -             return;
> > +             goto e_unmap;
> >       }
> >
> >       nbits = sizeof(ghcb->save.valid_bitmap) * 8;
> > @@ -2944,12 +2969,21 @@ static void dump_ghcb(struct vcpu_svm *svm)
> >       pr_err("%-20s%016llx is_valid: %u\n", "sw_scratch",
> >              ghcb->save.sw_scratch, ghcb_sw_scratch_is_valid(ghcb));
> >       pr_err("%-20s%*pb\n", "valid_bitmap", nbits, ghcb->save.valid_bitmap);
> > +
> > +e_unmap:
> > +     svm_unmap_ghcb(svm, &map);
> >  }
> >
> > -static void sev_es_sync_to_ghcb(struct vcpu_svm *svm)
> > +static bool sev_es_sync_to_ghcb(struct vcpu_svm *svm)
> >  {
> >       struct kvm_vcpu *vcpu = &svm->vcpu;
> > -     struct ghcb *ghcb = svm->sev_es.ghcb;
> > +     struct kvm_host_map map;
> > +     struct ghcb *ghcb;
> > +
> > +     if (svm_map_ghcb(svm, &map))
> > +             return false;
> > +
> > +     ghcb = map.hva;
> >
> >       /*
> >        * The GHCB protocol so far allows for the following data
> > @@ -2963,13 +2997,24 @@ static void sev_es_sync_to_ghcb(struct vcpu_svm *svm)
> >       ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]);
> >       ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]);
> >       ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]);
> > +
> > +     /*
> > +      * Copy the return values from the exit_info_{1,2}.
> > +      */
> > +     ghcb_set_sw_exit_info_1(ghcb, svm->sev_es.ghcb_sw_exit_info_1);
> > +     ghcb_set_sw_exit_info_2(ghcb, svm->sev_es.ghcb_sw_exit_info_2);
> > +
> > +     trace_kvm_vmgexit_exit(svm->vcpu.vcpu_id, ghcb);
> > +
> > +     svm_unmap_ghcb(svm, &map);
> > +
> > +     return true;
> >  }
> >
> > -static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
> > +static void sev_es_sync_from_ghcb(struct vcpu_svm *svm, struct ghcb *ghcb)
> >  {
> >       struct vmcb_control_area *control = &svm->vmcb->control;
> >       struct kvm_vcpu *vcpu = &svm->vcpu;
> > -     struct ghcb *ghcb = svm->sev_es.ghcb;
> >       u64 exit_code;
> >
> >       /*
> > @@ -3013,20 +3058,25 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
> >       memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
> >  }
> >
> > -static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> > +static int sev_es_validate_vmgexit(struct vcpu_svm *svm, u64 *exit_code)
> >  {
> > -     struct kvm_vcpu *vcpu;
> > +     struct kvm_vcpu *vcpu = &svm->vcpu;
> > +     struct kvm_host_map map;
> >       struct ghcb *ghcb;
> > -     u64 exit_code;
> >       u64 reason;
> >
> > -     ghcb = svm->sev_es.ghcb;
> > +     if (svm_map_ghcb(svm, &map))
> > +             return -EFAULT;
> > +
> > +     ghcb = map.hva;
> > +
> > +     trace_kvm_vmgexit_enter(vcpu->vcpu_id, ghcb);
> >
> >       /*
> >        * Retrieve the exit code now even though it may not be marked valid
> >        * as it could help with debugging.
> >        */
> > -     exit_code = ghcb_get_sw_exit_code(ghcb);
> > +     *exit_code = ghcb_get_sw_exit_code(ghcb);
> >
> >       /* Only GHCB Usage code 0 is supported */
> >       if (ghcb->ghcb_usage) {
> > @@ -3119,6 +3169,9 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> >               goto vmgexit_err;
> >       }
> >
> > +     sev_es_sync_from_ghcb(svm, ghcb);
> > +
> > +     svm_unmap_ghcb(svm, &map);
> >       return 0;
> >
> >  vmgexit_err:
> > @@ -3129,10 +3182,10 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> >                           ghcb->ghcb_usage);
> >       } else if (reason == GHCB_ERR_INVALID_EVENT) {
> >               vcpu_unimpl(vcpu, "vmgexit: exit code %#llx is not valid\n",
> > -                         exit_code);
> > +                         *exit_code);
> >       } else {
> >               vcpu_unimpl(vcpu, "vmgexit: exit code %#llx input is not valid\n",
> > -                         exit_code);
> > +                         *exit_code);
> >               dump_ghcb(svm);
> >       }
> >
> > @@ -3142,6 +3195,8 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> >       ghcb_set_sw_exit_info_1(ghcb, 2);
> >       ghcb_set_sw_exit_info_2(ghcb, reason);
> >
> > +     svm_unmap_ghcb(svm, &map);
> > +
> >       /* Resume the guest to "return" the error code. */
> >       return 1;
> >  }
>